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

Pavel Reichl preichl at redhat.com
Wed Mar 11 17:02:01 UTC 2015



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

>
> 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