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
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}