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

Lukas Slebodnik lslebodn at redhat.com
Wed Mar 11 17:04:20 UTC 2015


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.

LS



More information about the sssd-devel mailing list