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

Jakub Hrozek jhrozek at redhat.com
Thu Mar 12 09:19:19 UTC 2015


On Wed, Mar 11, 2015 at 06:35:16PM -0400, Pavel Reichl wrote:
> 
> 
> 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 

Thanks, pushed upstream:

* master:
    * aa648535f445e7a95bf6dedc7c43bb5f94ab7354
    * ff19b24a93a50c8a62b5c2621e45d101e3a00781
    * 84a4c4fcc93b3dcc70604817a05f7943606ff596
    * 9cc2223e0bc0478c1b47a47fd71bba7e7129492d 
* sssd-1-12:
    * d8006abd55f2ce0698e09213b8374e9071e70016

I only pushed the fix without the unit tests to upstream, since the unit
tests were already converted to cmocka-1.0



More information about the sssd-devel mailing list