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.
> +done:
> + talloc_free(tmp_ctx);
> + return ret;
> +}
>
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel