On Fri, Dec 12, 2014 at 01:16:29PM +0100, Pavel Reichl wrote:
> 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.
I think that the check was
there forgotten from previous version when
tallo_strdup was used.
> + 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;
> +}
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel