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
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for review. Please see updated patch.
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for review. Please see updated patch.
Thank you, ACK.
bye, Sumit
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@ipaad2012r2.test // aduser
==14009== Use of uninitialised value of size 8 ==14009== at 0x13ABE6B7: retain_extern_members (sdap_async_groups.c:864) ==14009== by 0x13ABE6B7: sdap_save_grpmem (sdap_async_groups.c:929) ==14009== by 0x13ABE6B7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== ==14009== Use of uninitialised value of size 8 ==14009== at 0x4C2CB62: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==14009== by 0x13ABEEF7: has_member (sdap_async_groups.c:200) ==14009== by 0x13ABEEF7: link_pgroup_members (sdap_async_groups.c:222) ==14009== by 0x13ABEEF7: sdap_fill_memberships (sdap_async_groups.c:331) ==14009== by 0x13ABEEF7: sdap_save_grpmem (sdap_async_groups.c:961) ==14009== by 0x13ABEEF7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== ==14009== Invalid read of size 1 ==14009== at 0x4C2CB62: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==14009== by 0x13ABEEF7: has_member (sdap_async_groups.c:200) ==14009== by 0x13ABEEF7: link_pgroup_members (sdap_async_groups.c:222) ==14009== by 0x13ABEEF7: sdap_fill_memberships (sdap_async_groups.c:331) ==14009== by 0x13ABEEF7: sdap_save_grpmem (sdap_async_groups.c:961) ==14009== by 0x13ABEEF7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== Address 0xdededede is not stack'd, malloc'd or (recently) free'd
LS
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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?
Maybe this should be better written with local variables to avoid this kind of issues? Additionally there is a NULL check missing after talloc_realloc().
bye, Sumit
==14009== Use of uninitialised value of size 8 ==14009== at 0x13ABE6B7: retain_extern_members (sdap_async_groups.c:864) ==14009== by 0x13ABE6B7: sdap_save_grpmem (sdap_async_groups.c:929) ==14009== by 0x13ABE6B7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== ==14009== Use of uninitialised value of size 8 ==14009== at 0x4C2CB62: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==14009== by 0x13ABEEF7: has_member (sdap_async_groups.c:200) ==14009== by 0x13ABEEF7: link_pgroup_members (sdap_async_groups.c:222) ==14009== by 0x13ABEEF7: sdap_fill_memberships (sdap_async_groups.c:331) ==14009== by 0x13ABEEF7: sdap_save_grpmem (sdap_async_groups.c:961) ==14009== by 0x13ABEEF7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== ==14009== Invalid read of size 1 ==14009== at 0x4C2CB62: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==14009== by 0x13ABEEF7: has_member (sdap_async_groups.c:200) ==14009== by 0x13ABEEF7: link_pgroup_members (sdap_async_groups.c:222) ==14009== by 0x13ABEEF7: sdap_fill_memberships (sdap_async_groups.c:331) ==14009== by 0x13ABEEF7: sdap_save_grpmem (sdap_async_groups.c:961) ==14009== by 0x13ABEEF7: sdap_save_groups (sdap_async_groups.c:1094) ==14009== by 0x13AC09CC: sdap_nested_done (sdap_async_groups.c:2305) ==14009== by 0x13AC3E35: sdap_nested_group_process_done (sdap_async_nested_groups.c:994) ==14009== by 0x54BB7E3: tevent_common_loop_immediate (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54C00CD: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE7D6: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BAFBC: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BB15A: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x54BE776: ??? (in /usr/lib64/libtevent.so.0.9.21) ==14009== by 0x52904A2: server_loop (server.c:668) ==14009== by 0x10E971: main (data_provider_be.c:2915) ==14009== Address 0xdededede is not stack'd, malloc'd or (recently) free'd
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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@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!
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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@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;
+}
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@rdustv1911.test //ipa admin id aduser1@ipaad2012r2.test // aduser id aduser2@ipaad2012r2.test sss_cache -E id aduser1@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@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.
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;
+}
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@rdustv1911.test //ipa admin > id aduser1@ipaad2012r2.test // aduser > id aduser2@ipaad2012r2.test > sss_cache -E > id aduser1@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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for noticing. Updated patch attached.
On Fri, Dec 12, 2014 at 04:45:13PM +0100, Pavel Reichl wrote:
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@rdustv1911.test //ipa admin >>id aduser1@ipaad2012r2.test // aduser >>id aduser2@ipaad2012r2.test >>sss_cache -E >>id aduser1@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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for noticing. Updated patch attached.
ACK
bye, Sumit
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote:
ACK
bye, Sumit
master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote:
ACK
bye, Sumit
master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Dec 15, 2014 at 04:36:46PM +0100, Sumit Bose wrote:
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote:
ACK
bye, Sumit
master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
OK.
Pavel, can you then prepare the sssd-1-11 backport? The patch couldn't be cherry-picked easily..
On 01/05/2015 04:55 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 04:36:46PM +0100, Sumit Bose wrote:
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote:
ACK
bye, Sumit
master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
OK.
Pavel, can you then prepare the sssd-1-11 backport? The patch couldn't be cherry-picked easily.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure, patch for 1-11 is attached. Although necessary changes were minimal I would like if the patch was tested before it's pushed. Unfortunately my testing environment is completely broken right now. So unless Sumit has his testing environment still handy and has time to test it I would prefer if we could wait for my testing then.
On Mon, Jan 05, 2015 at 07:18:13PM +0100, Pavel Reichl wrote:
On 01/05/2015 04:55 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 04:36:46PM +0100, Sumit Bose wrote:
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote:
ACK
bye, Sumit
master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
OK.
Pavel, can you then prepare the sssd-1-11 backport? The patch couldn't be cherry-picked easily.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure, patch for 1-11 is attached. Although necessary changes were minimal I would like if the patch was tested before it's pushed. Unfortunately my testing environment is completely broken right now. So unless Sumit has his testing environment still handy and has time to test it I would prefer if we could wait for my testing then.
np, I'm not in a hurry. I would prefer if this patch was part of the next 1.11 release, but we still have a known nested group segfault to fix before we do that release..
On 01/06/2015 09:12 AM, Jakub Hrozek wrote:
On Mon, Jan 05, 2015 at 07:18:13PM +0100, Pavel Reichl wrote:
On 01/05/2015 04:55 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 04:36:46PM +0100, Sumit Bose wrote:
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote:
On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote: > ACK > > bye, > Sumit master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920
reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
OK.
Pavel, can you then prepare the sssd-1-11 backport? The patch couldn't be cherry-picked easily.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure, patch for 1-11 is attached. Although necessary changes were minimal I would like if the patch was tested before it's pushed. Unfortunately my testing environment is completely broken right now. So unless Sumit has his testing environment still handy and has time to test it I would prefer if we could wait for my testing then.
np, I'm not in a hurry. I would prefer if this patch was part of the next 1.11 release, but we still have a known nested group segfault to fix before we do that release.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I tested the patch - works fine. Thanks for patience.
On Mon, Jan 12, 2015 at 03:41:16PM +0100, Pavel Reichl wrote:
On 01/06/2015 09:12 AM, Jakub Hrozek wrote:
On Mon, Jan 05, 2015 at 07:18:13PM +0100, Pavel Reichl wrote:
On 01/05/2015 04:55 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 04:36:46PM +0100, Sumit Bose wrote:
On Sat, Dec 13, 2014 at 10:25:55PM +0100, Jakub Hrozek wrote:
On Sat, Dec 13, 2014 at 10:17:09PM +0100, Jakub Hrozek wrote: >On Fri, Dec 12, 2014 at 05:19:00PM +0100, Sumit Bose wrote: >>ACK >> >>bye, >>Sumit >master: 6fac5e5f0c54a0f92872ce1450606cfcb577a920 reviewers, if you think it makes sense to push this patch to sssd-1-11 as well, please holler.
I personally don't -- this bug seems mostly be related to the server mode and for IPA server side SSSD it just makes sense to go with sssd-1-12.
I think the patch is valid on 1.11 IPA clients as well because it makes sure that members of IPA groups which are added from a different source, e.g. the PAC responder, are not removed if the group expires and is updated with data from the IPA LDAP group object, which does not contain the external users as members.
So if there are issue on 1.11 where IPA group memberships of AD users get lost this patch might help here as well.
bye, Sumit
OK.
Pavel, can you then prepare the sssd-1-11 backport? The patch couldn't be cherry-picked easily.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure, patch for 1-11 is attached. Although necessary changes were minimal I would like if the patch was tested before it's pushed. Unfortunately my testing environment is completely broken right now. So unless Sumit has his testing environment still handy and has time to test it I would prefer if we could wait for my testing then.
np, I'm not in a hurry. I would prefer if this patch was part of the next 1.11 release, but we still have a known nested group segfault to fix before we do that release.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I tested the patch - works fine. Thanks for patience.
* sssd-1-11: dcb16038ac779186aebbd7d37caab3736cb01466
On (10/12/14 10:44), 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.
//snip
- /* This is a temporal solution until the IPA provider is able to
* resolve external group membership.
* https://fedorahosted.org/sssd/ticket/2522
*/
- 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",
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I was testing whether your patch fix some issues and this message looks very dangerous in log files. It is ignored by default. So it would be good to change debug level because it is not kind of error which we are intrested in. (You can also change debug message or treat ENOENT differently)
[sdap_save_grpmem] (0x0080): retain_extern_members failed: 2:[No such file or directory].
LS
On (10/12/14 10:44), 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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for review. Please see updated patch.
From a2b941b6f03b77e92604e2709bc18d891c2365f3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 20 Nov 2014 18:27:04 +0000 Subject: [PATCH] LDAP: retain external members
When processing group membership check sysdb for group members from extern domain and include them in newly processed group membership as extern members are curently found only when initgroups() is called.
Resolves: https://fedorahosted.org/sssd/ticket/2492
src/db/sysdb.h | 6 +++ src/db/sysdb_ops.c | 90 ++++++++++++++++++++++++++++++++ src/providers/ldap/sdap_async_groups.c | 93 ++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5bd7f90acb685bbaff5c98f433c7dce8175c33ca..a1254bb556533245ca3a02342b775cda7f0f6f79 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -1103,4 +1103,10 @@ errno_t sysdb_gpo_get_gpo_result_setting(TALLOC_CTX *mem_ctx, const char *policy_setting_key, const char **policy_setting_value);
+errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom,
const char *group_name,
char ***_sids,
char ***_dns,
size_t *_n);
#endif /* __SYS_DB_H__ */ diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 998046a2ca1c746b2032f430e5f9c4a7151e1dbc..7ad8816ba93e45e998ce277ca9dc672d5fe5c39e 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -3630,3 +3630,93 @@ errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx, return sysdb_search_object_by_str_attr(mem_ctx, domain, SYSDB_UUID_FILTER, uuid_str, attrs, res); }
+errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom,
const char *group_name,
char ***_sids,
char ***_dns,
size_t *_n)
+{
- errno_t ret;
- size_t i, m_count;
- TALLOC_CTX *tmp_ctx;
- struct ldb_message *msg;
- struct ldb_message **members;
- const char *attrs[] = {SYSDB_SID_STR, NULL };
^^ Please add space here
- char **sids = NULL, **dns = NULL;
- size_t n = 0;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
return ENOMEM;
- }
- ret = sysdb_search_group_by_name(tmp_ctx, dom, group_name, NULL, &msg);
- if (ret != EOK) {
goto done;
- }
- /* Get sid_str attribute of all elemets pointed to by group members */
- ret = sysdb_asq_search(tmp_ctx, dom, msg->dn, NULL, SYSDB_MEMBER, attrs,
&m_count, &members);
- if (ret != EOK) {
goto done;
- }
- sids = talloc_array(tmp_ctx, char*, m_count);
- if (sids == NULL) {
ret = ENOMEM;
goto done;
- }
- dns = talloc_array(tmp_ctx, char*, m_count);
- if (dns == NULL) {
ret = ENOMEM;
goto done;
- }
- for(i=0; i < m_count; i++) {
const char *sidstr;
sidstr = ldb_msg_find_attr_as_string(members[i], SYSDB_SID_STR, NULL);
if (sidstr != NULL) {
sids[n] = talloc_strdup(sids, sidstr);
result of sysdb_search_group_by_name is allocated under temporary context and it is not used anywhere else. If there aren't objections from other developers we can use talloc_steal in this case.
if (sids[n] == NULL) {
ret = ENOMEM;
goto done;
}
dns[n] = talloc_strdup(dns,
ldb_dn_get_linearized(members[i]->dn));
If there aren't objections from other developers we can use talloc_steal here as well.
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) {
^^^^^^^^^ } else if http://www.freeipa.org/page/Coding_Style#IF_Statements
DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret));
- }
- talloc_free(tmp_ctx);
- return ret;
+} diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 8cf7f7ff1d414049f0694c7d2873556fc9dad741..e8c408253a0cc311bb84e749208180fd92b379a9 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -801,6 +801,76 @@ done: return ret; }
+static errno_t +are_sids_from_same_dom(const char *sid1, const char *sid2, bool *_result) +{
- char *rid1, *rid2;
- bool result;
- rid1 = strrchr(sid1, '-');
- if (rid1 == NULL) {
return EINVAL;
- }
- rid2 = strrchr(sid2, '-');
- if (rid2 == NULL) {
return EINVAL;
- }
- result = ((rid1 - sid1) == (rid2 - sid2)) &&
(strncmp(sid1, sid2, rid1 - sid1) == 0);
(rid1 - sid1) is the lenght of "SID header" "SID header" is probably not the right terminology. I was able to find just this definition of sid.
The structure of the SID looks like this: S-[Revision]-[IdentifierAuthority]-[SubAuthority0]-[SubAuthority1]-...-[SubAuthority[SubAuthorityCount]](-RID).
But let's back to main point It would improve readability if you use constants for length of "SID header" (maybe slength{1,2} lenght{1,2} ...
- *_result = result;
- return EOK;
+}
+static errno_t +retain_extern_members(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom,
const char *group_name,
const char *group_sid,
char ***_userdns,
size_t *_nuserdns)
+{
- TALLOC_CTX *tmp_ctx;
- char **sids, **dns;
- bool same_domain;
- errno_t ret;
- size_t i, n;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
return ENOMEM;
- }
- ret = sysdb_get_sids_of_members(tmp_ctx, dom, group_name, &sids, &dns, &n);
- if (ret != EOK ) {
^ remove extra space
if (ret != ENOENT) {
DEBUG(SSSDBG_TRACE_ALL,
"get_sids_of_members failed: %d [%s]\n",
ret, sss_strerror(ret));
}
goto done;
- }
- 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, char*, *_nuserdns);
*_userdns[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]);
^^^^^^^^^ @see Sumit's hint in different mail.
}
- }
- ret = EOK;
+done:
- talloc_free(tmp_ctx);
- return ret;
+}
/* ==Save-Group-Memebrs=================================================== */
@@ -817,6 +887,7 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx, { struct ldb_message_element *el; struct sysdb_attrs *group_attrs = NULL;
- const char *group_sid; const char *group_name; char **userdns = NULL; size_t nuserdns = 0;
@@ -843,6 +914,28 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx, } }
- /* This is a temporal solution until the IPA provider is able to
* resolve external group membership.
* https://fedorahosted.org/sssd/ticket/2522
*/
- 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));
}
}
- }
- ret = sysdb_attrs_get_el(attrs, opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name, &el); if (ret != EOK) {
-- 1.9.3
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org