[SSSD] [PATCH] LDAP: retain external members

Lukas Slebodnik lslebodn at redhat.com
Wed Dec 10 22:28:26 UTC 2014


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 at rdustv1911.test  //ipa admin
>> id aduser1 at ipaad2012r2.test // aduser
>> id aduser2 at ipaad2012r2.test
>> sss_cache -E
>> id aduser1 at 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



More information about the sssd-devel mailing list