On 12/11/2014 06:47 PM, Sumit Bose wrote:
>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(a)rdustv1911.test //ipa admin
>>>>>id aduser1(a)ipaad2012r2.test // aduser
>>>>>id aduser2(a)ipaad2012r2.test
>>>>>sss_cache -E
>>>>>id aduser1(a)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(a)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
Thank you for patience. Updated patch attached.
Thank you for the new version, it is working as expected and I didn't
see a crash neither in IPA clients nor on a server.
I think all of Lukas' comments are addressed as well. I just have a
minor comment but imo it does not has to be fixed.
bye,
Sumit
+
+ if (sidstr != NULL) {
+ sids[n] = talloc_steal(sids, sidstr);
Currently talloc_steal() only returns NULL if the 2nd argument is NULL
and since we know that sidstr != NULL the following check will never be
true. But since it is quite cheap it can be left here.
+ if (sids[n] == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ dns[n] = talloc_steal(dns,
+ ldb_dn_get_linearized(members[i]->dn));
In this case the check is valid because ldb_dn_get_linearized() can
return NULL.
+
+ if (dns[n] == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ n++;
+ }
+ }
+
+ if (n == 0) {
+ ret = ENOENT;
+ goto done;
+ }
+
+ *_n = n;
+ *_sids = talloc_steal(mem_ctx, sids);
+ *_dns = talloc_steal(mem_ctx, dns);
+
+ ret = EOK;
+
+done:
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_FUNC, "No such entry\n");
+ } else if (ret) {
+ DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret));
+ }
+ talloc_free(tmp_ctx);
+ return ret;
+}