[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