On Wed, Mar 11, 2015 at 06:02:01PM +0100, 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.0Thanks 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.htmlI'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 2001From: Jakub Hrozek <jhrozek@redhat.com> Date: Mon, 9 Mar 2015 16:59:39 +0100 Subject: [PATCH 3/4] Add unit tests for initgroupsThis 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.htmlI 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.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel