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.