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

Pavel Reichl preichl at redhat.com
Wed Mar 11 22:35:16 UTC 2015



On 03/11/2015 06:07 PM, Jakub Hrozek wrote: 



On Wed, Mar 11, 2015 at 06:02:01PM +0100, Pavel Reichl wrote: 

<blockquote>

On 03/11/2015 05:04 PM, Lukas Slebodnik wrote: 

<blockquote>

On (11/03/15 16:48), Jakub Hrozek wrote: 

<blockquote>

On Wed, Mar 11, 2015 at 04:30:52PM +0100, Pavel Reichl wrote: 

<blockquote>

On 03/11/2015 03:46 PM, Jakub Hrozek wrote: 

<blockquote>

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

</blockquote>

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. 

</blockquote>

</blockquote>

I think this version looks a little better, but I'm fine with both version.
So ACK from me. 

<blockquote>

>From c0daf41e1c9ca1e8331dc9e5e7098e7862fd8b7e Mon Sep 17 00:00:00 2001 

<blockquote>

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 

</blockquote>

This patch should be applied as the last one (and not 3rd)
otherwise the test nss-srv-tests fails and our CI would complain. 

</blockquote>

How would CI complain? It seems OK to me: http://sssd-ci.duckdns.org/logs/job/9/47/summary.html 

</blockquote>

I think that's because the CI that runs post-commit runs the 'rigorous'
set of tests which applies the commits one by one. Developers usually
run the 'essential' set of tests.

The patch order was actually intentional, I was first trying to show the
bug and fix in the later patch. But I didn't realize the CI point. I
think Lukas is right.

Attached is the patchset I'd like to push. 

</blockquote>

I quickly retested the patchset. ACK 
ci passed: http://sssd-ci.duckdns.org/logs/job/9/49/summary.html 

<blockquote>



_______________________________________________
sssd-devel mailing list sssd-devel at lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel 

</blockquote>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150311/09bae42c/attachment.html>


More information about the sssd-devel mailing list