[SSSD] [PATCH] NSS: Handle ENOENT when doing initgroups by UPN

Pavel Reichl preichl at redhat.com
Wed Mar 11 17:10:33 UTC 2015



On 03/11/2015 06:04 PM, Lukas Slebodnik wrote:
> On (11/03/15 18:02), Pavel Reichl wrote:
>>
>> On 03/11/2015 05:04 PM, Lukas Slebodnik wrote:
>>> On (11/03/15 16:48), Jakub Hrozek wrote:
>>>> On Wed, Mar 11, 2015 at 04:30:52PM +0100, Pavel Reichl wrote:
>>>>> On 03/11/2015 03:46 PM, Jakub Hrozek wrote:
>>>>>
>>>>>
>>>>>> @@ -4062,29 +4062,39 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
>>>>>>           if (cmdctx->name_is_upn) {
>>>>>>               ret = sysdb_search_user_by_upn(cmdctx, dom, name, user_attrs, &msg);
>>>>>> -            if (ret != EOK && ret != ENOENT) {
>>>>>> +            if (ret == EOK) {
>>>>>> +                sysdb_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
>>>>>> +                if (sysdb_name == NULL) {
>>>>>> +                    DEBUG(SSSDBG_OP_FAILURE,
>>>>>> +                        "Sysdb entry does not have a name.\n");
>>>>>> +                    return EINVAL;
>>>>>> +                }
>>>>>> +
>>>>>> +                ret = sysdb_initgroups(cmdctx, dom, sysdb_name, &dctx->res);
>>>>>> +                if (ret == EOK && DOM_HAS_VIEWS(dom)) {
>>>>>> +                    for (c = 0; c < dctx->res->count; c++) {
>>>>>> +                        ret = sysdb_add_overrides_to_object(dom, dctx->res->msgs[c],
>>>>>> +                                                            NULL, NULL);
>>>>>> +                        if (ret != EOK) {
>>>>>> +                            DEBUG(SSSDBG_OP_FAILURE,
>>>>>> +                                "sysdb_add_overrides_to_object failed.\n");
>>>>>> +                            return ret;
>>>>>> +                        }
>>>>>> +                    }
>>>>>> +                }
>>>>>> +            } else if (ret != ENOENT) {
>>>>>>                   DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn failed.\n");
>>>>>>                   return ret;
>>>>>> -            }
>>>>>> -
>>>>>> -            sysdb_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
>>>>>> -            if (sysdb_name == NULL) {
>>>>>> -                DEBUG(SSSDBG_OP_FAILURE,
>>>>>> -                      "Sysdb entry does not have a name.\n");
>>>>>> -                return EINVAL;
>>>>>> -            }
>>>>>> -
>>>>>> -            ret = sysdb_initgroups(cmdctx, dom, sysdb_name, &dctx->res);
>>>>>> -            if (ret == EOK && DOM_HAS_VIEWS(dom)) {
>>>>>> -                for (c = 0; c < dctx->res->count; c++) {
>>>>>> -                    ret = sysdb_add_overrides_to_object(dom, dctx->res->msgs[c],
>>>>>> -                                                        NULL, NULL);
>>>>>> -                    if (ret != EOK) {
>>>>>> -                        DEBUG(SSSDBG_OP_FAILURE,
>>>>>> -                              "sysdb_add_overrides_to_object failed.\n");
>>>>>> -                        return ret;
>>>>>> -                    }
>>>>>> +            } else {
>>>>>> +                dctx->res = talloc_zero(cmdctx, struct ldb_result);
>>>>>> +                if (dctx->res == NULL) {
>>>>>> +                    DEBUG(SSSDBG_OP_FAILURE, "talloc_zero failed.\n");
>>>>>> +                    return ENOMEM;
>>>>>>                   }
>>>>>> +
>>>>>> +                dctx->res->count = 0;
>>>>>> +                dctx->res->msgs = NULL;
>>>>>> +                ret = EOK;
>>>>>>               }
>>>>>>           } else {
>>>>>>               ret = sysdb_initgroups_with_views(cmdctx, dom, name, &dctx->res);
>>>>>> -- 2.1.0
>>>>> Thanks for the patches.
>>>>> All patches LGTM.
>>>>>
>>>>> I've tested that tests fail if the forth patch is not applied. I also wrote
>>>>> simple program to reproduce the segfault and after applying the 4th patch
>>>>> I'm no longer able to reproduce the bug.
>>>>>
>>>>> I have only one style nitpick - would it make more sense to test explicitly
>>>>> for 'ret == ENOENT' and handle the other cases in else branch?
>>>>>
>>>>> If you don't agree then ignore then comment and push the patches (ACK to
>>>>> all).
>>>>>
>>>>> Thanks!
>>>>>
>>>>> ci passed:
>>>>>
>>>>> http://sssd-ci.duckdns.org/logs/job/9/46/summary.html
>>>> I'm not sure if I understood what you meant, but check the attached
>>>> patches.
>>>>
>>>> Normally I prefer:
>>>>     if (ret == ENOENT) {
>>>>         /* handle ENOENT */
>>>>     } else if (ret != EOK) {
>>>>         /* Error! *//
>>>>     }
>>>>
>>>>     /* Let's not put EOK into else and avoid extra indentation */
>>>>
>>>> But here the EOK must be in an else branch, I think.
>> I think this version looks a little better, but I'm fine with both version.
>> So ACK from me.
>>
>>
>>> >From c0daf41e1c9ca1e8331dc9e5e7098e7862fd8b7e Mon Sep 17 00:00:00 2001
>>>> From: Jakub Hrozek <jhrozek at redhat.com>
>>>> Date: Mon, 9 Mar 2015 16:59:39 +0100
>>>> Subject: [PATCH 3/4] Add unit tests for initgroups
>>>>
>>> This patch should be applied as the last one (and not 3rd)
>>> otherwise the test nss-srv-tests fails and our CI would complain.
>> How would CI complain? It seems OK to me:
>>
>> http://sssd-ci.duckdns.org/logs/job/9/47/summary.html
>>
> You did not test each patch separatelly.
> You tested only a whole patchset.
Oh, Lukas just remained me, that the test failure would be a problem for 
git bisect. Then I guess we should move the 4th patch.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list