On 12/12/2014 04:08 PM, Sumit Bose wrote:
>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
Thanks for noticing. Updated patch attached.