[SSSD] [PATCH] LDAP: retain external members

Sumit Bose sbose at redhat.com
Thu Dec 11 17:47:02 UTC 2014


On Thu, Dec 11, 2014 at 06:07:07PM +0100, Pavel Reichl wrote:
> 
> On 12/10/2014 11:28 PM, Lukas Slebodnik wrote:
> >On (10/12/14 21:32), Sumit Bose wrote:
> >>On Wed, Dec 10, 2014 at 05:43:37PM +0100, Lukas Slebodnik wrote:
> >>>On (10/12/14 11:26), Sumit Bose wrote:
> >>>>On Wed, Dec 10, 2014 at 10:44:32AM +0100, Pavel Reichl wrote:
> >>>>>On 12/09/2014 01:36 PM, Sumit Bose wrote:
> >>>>>>On Tue, Nov 25, 2014 at 03:42:14PM +0100, Pavel Reichl wrote:
> >>>>>>>Hello,
> >>>>>>>
> >>>>>>>please see attached patch for https://fedorahosted.org/sssd/ticket/2492
> >>>>>>>
> >>>>>>>Thanks!
> >>>>>>Hi Pavel,
> >>>>>>
> >>>>>>thank you for the patch, it works well in my tests and I didn't see any
> >>>>>>regressions in IPA setup with and without trsut to AD, so ACK.
> >>>>>>
> >>>>>>I would just like to ask you to add a comment to
> >>>>>>
> >>>>>>>@@ -842,6 +913,23 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx,
> >>>>>>>              goto fail;
> >>>>>>>          }
> >>>>>>>      }
> >>>>>>>+    if (opts->schema_type == SDAP_SCHEMA_IPA_V1) {
> >>>>>>>+        ret = sysdb_attrs_get_string(attrs, SYSDB_SID_STR, &group_sid);
> >>>>>>>+        if (ret != EOK) {
> >>>>>>>+            DEBUG(SSSDBG_TRACE_FUNC, "Failed to get group sid\n");
> >>>>>>>+            group_sid = NULL;
> >>>>>>>+        }
> >>>>>>>+
> >>>>>>>+        if (group_sid != NULL) {
> >>>>>>>+            ret = retain_extern_members(memctx, dom, group_name, group_sid,
> >>>>>>>+                                        &userdns, &nuserdns);
> >>>>>>>+            if (ret != EOK) {
> >>>>>>>+                DEBUG(SSSDBG_MINOR_FAILURE,
> >>>>>>>+                      "retain_extern_members failed: %d:[%s].\n",
> >>>>>>>+                      ret, sss_strerror(ret));
> >>>>>>>+            }
> >>>>>>>+        }
> >>>>>>>+    }
> >>>>>>which explains that this is a temporary solution until the IPA provider
> >>>>>>can resolve external group membership. I have created
> >>>>>>https://fedorahosted.org/sssd/ticket/2522 for this. Feel free to
> >>>>>>explicitly add the ticket URL into the comment.
> >>>>>>
> >>>>>>bye,
> >>>>>>Sumit
> >>>>>Thanks for review.
> >>>>>Please see updated patch.
> >>>>>
> >>>>Thank you, ACK.
> >>>NACK, sssd crashed with this patch.
> >>>How to reproduce:
> >>>sssd with ipa server mode:
> >>>
> >>>id admin at rdustv1911.test  //ipa admin
> >>>id aduser1 at ipaad2012r2.test // aduser
> >>>id aduser2 at ipaad2012r2.test
> >>>sss_cache -E
> >>>id aduser1 at ipaad2012r2.test // aduser
> >>good catch, thank you. Looks like I always only tested with one external
> >>member. Does
> >>
> >>diff --git a/src/providers/ldap/sdap_async_groups.c
> >>b/src/providers/ldap/sdap_async_groups.c
> >>index 43744ac..4bc2aa7 100644
> >>--- a/src/providers/ldap/sdap_async_groups.c
> >>+++ b/src/providers/ldap/sdap_async_groups.c
> >>@@ -861,7 +861,7 @@ retain_extern_members(TALLOC_CTX *mem_ctx,
> >>
> >>             (*_nuserdns)++;
> >>             *_userdns = talloc_realloc(mem_ctx, *_userdns, char*, *_nuserdns);
> >>-            *_userdns[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]);
> >>+            (*_userdns)[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]);
> >>         }
> >>     }
> >>
> >>
> >>fix the issue for you?
> >Yet, it fixed.
> >I could see the mistake in code and I was looking at the code at least fo 10
> >minutes. Someone should write 100 times I will learn C operator precedence
> >http://en.cppreference.com/w/c/language/operator_precedence
> >
> >>Maybe this should be better written with local variables to avoid this
> >>kind of issues?
> >+1
> >
> >>Additionally there is a NULL check missing after talloc_realloc().
> >+1
> >
> >LS
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> I'm really sorry about the crash.
> Please see if the new patch is crash free.
> 
> Thanks!

Hi Pavel,

thank you for the new version, I haven't had a close look yet, but I hav
a nitpick about using the memory contexts ...

...

With the if (userdns == NULL), there is a chance to leak memory. I would
suggest to following:

> +
> +    for (i=0; i < n; i++) {
> +        ret = are_sids_from_same_dom(group_sid, sids[i], &same_domain);
> +        if (ret == EOK && !same_domain) {
> +            DEBUG(SSSDBG_TRACE_ALL, "extern member: %s\n", dns[i]);
> +            nuserdns++;
> +            userdns = talloc_realloc(mem_ctx, userdns, const char*, nuserdns);
               userdns = talloc_realloc(tmp_ctx, userdns, const char*, nuserdns);
> +            if (userdns == NULL) {
> +                return ENOMEM;
> +            }
> +            userdns[nuserdns-1] = talloc_steal(mem_ctx, dns[i]);
               userdns[nuserdns-1] = talloc_steal(userdns, dns[i]);
> +        }
> +    }
> +    *_nuserdns = nuserdns;
       *_nuserdns = talloc_steal(mem_ctx, nuserdns);
> +    *_userdns = discard_const(userdns);
> +    ret = EOK;
> +

talloc_steal() is not recursive, but with the scheme above the items of
userdns are children of userdns talloc-wise and stealing userdns on
mem_ctx does not change it and a talloc_free(userdns) will free userdns
and all its elements.

bye,
Sumit

> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
>  



More information about the sssd-devel mailing list