Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 932745aaadd7c2a73e817bfd685fbe1aaa462cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 16:40:45 +0200 Subject: [PATCH 1/2] CI: Test extra attributes duplicate
Regresion test for ticket #3120
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/tests/intg/ldap_test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 84f7f2d..b8315e5 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -292,6 +292,28 @@ def sanity_rfc2307_bis(request, ldap_conn): return None
+@pytest.fixture +def just_start_ldap(request, ldap_conn):
- ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
- create_ldap_fixture(request, ldap_conn, ent_list)
- conf = \
format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \unindent("""\[domain/LDAP]ldap_user_extra_attrs = mail""").format(**locals())- create_conf_fixture(request, conf)
- create_sssd_cleanup(request)
- return None
+def test_extra_attribute_already_exists(ldap_conn, just_start_ldap):
- if subprocess.call(["sssd", "-D", "-f"]) != 0:
raise Exception("sssd start failed")def test_regression_ticket2163(ldap_conn, simple_rfc2307): ent.assert_passwd_by_name( 'usr\001', -- 2.5.0
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 932745aaadd7c2a73e817bfd685fbe1aaa462cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 16:40:45 +0200 Subject: [PATCH 1/2] CI: Test extra attributes duplicate
Regresion test for ticket #3120
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/tests/intg/ldap_test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 84f7f2d..b8315e5 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -292,6 +292,28 @@ def sanity_rfc2307_bis(request, ldap_conn): return None
+@pytest.fixture +def just_start_ldap(request, ldap_conn):
- ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
- create_ldap_fixture(request, ldap_conn, ent_list)
- conf = \
format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \unindent("""\[domain/LDAP]ldap_user_extra_attrs = mail""").format(**locals())- create_conf_fixture(request, conf)
- create_sssd_cleanup(request)
- return None
+def test_extra_attribute_already_exists(ldap_conn, just_start_ldap):
- if subprocess.call(["sssd", "-D", "-f"]) != 0:
raise Exception("sssd start failed")def test_regression_ticket2163(ldap_conn, simple_rfc2307): ent.assert_passwd_by_name( 'usr\001', -- 2.5.0
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
LS
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
LS
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote:
Hi,
see the attached patch.
I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr).
When we try to add entry to the map that already exists in the map, then without this patch we will fail.
With this patch, we only fail if the newly added extension would redefine already existing entry in the map.
Otherwise it is just skipped without a failure (we just skip adding what is already there).
I created simple CI test for this (first patch).
Michal
From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+/* _already_in_map is set to true if the attribute
- already exists in the map and is used for the same
- LDAP attribute.
- _conflicts_with_map is set to true if the attribute
- already exists in map, but is used for different
- LDAP attribute.
- */
+static void check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr,bool *_already_in_map,bool *_conflicts_with_map){
This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
Here you are
LS
On (16/08/16 15:20), Jakub Hrozek wrote:
On Tue, Aug 16, 2016 at 11:50:05AM +0200, Lukas Slebodnik wrote:
Here you are
LS
The code looks good, but can you rebase the patches?
BTW, You could not apply patches because integration test depends on patches from thread "[PATCH] intg: test nested membership"
LS
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote:
On (10/08/16 17:41), Michal Židek wrote: > Hi, > > see the attached patch. > > I modified the detection of duplicates when > extending the maps (sysdb_attr:ldap_attr). > > When we try to add entry to the map > that already exists in the map, then > without this patch we will fail. > > With this patch, we only fail if the > newly added extension would redefine > already existing entry in the map. > > Otherwise it is just skipped without > a failure (we just skip adding what > is already there). > > I created simple CI test for this (first > patch). > > Michal
> From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com > Date: Wed, 10 Aug 2016 15:41:34 +0200 > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps > > When extending map with entry that already > exists in the map in the exacty same form, > then there is no need to fail. > > We should only fail if we try to > change purpose of already used sysdb > attribute. > > Resolves: > https://fedorahosted.org/sssd/ticket/3120 > --- > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c > index 97b8f12..e1cf70f 100644 > --- a/src/providers/ldap/sdap.c > +++ b/src/providers/ldap/sdap.c > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, > return EOK; > } > > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, > - int num_entries, > - const char *sysdb_attr) > +/* _already_in_map is set to true if the attribute > + * already exists in the map and is used for the same > + * LDAP attribute. > + * > + * _conflicts_with_map is set to true if the attribute > + * already exists in map, but is used for different > + * LDAP attribute. > + * */ > +static void check_duplicate(struct sdap_attr_map *map, > + int num_entries, > + const char *sysdb_attr, > + const char *ldap_attr, > + bool *_already_in_map, > + bool *_conflicts_with_map) > { This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs.
LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;
Hmm, it crahsed even after adding this line.
Self-NACK
LS
On (16/08/16 15:22), Lukas Slebodnik wrote:
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote:
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: > On (10/08/16 17:41), Michal Židek wrote: > > Hi, > > > > see the attached patch. > > > > I modified the detection of duplicates when > > extending the maps (sysdb_attr:ldap_attr). > > > > When we try to add entry to the map > > that already exists in the map, then > > without this patch we will fail. > > > > With this patch, we only fail if the > > newly added extension would redefine > > already existing entry in the map. > > > > Otherwise it is just skipped without > > a failure (we just skip adding what > > is already there). > > > > I created simple CI test for this (first > > patch). > > > > Michal > > > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 > > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com > > Date: Wed, 10 Aug 2016 15:41:34 +0200 > > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps > > > > When extending map with entry that already > > exists in the map in the exacty same form, > > then there is no need to fail. > > > > We should only fail if we try to > > change purpose of already used sysdb > > attribute. > > > > Resolves: > > https://fedorahosted.org/sssd/ticket/3120 > > --- > > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ > > 1 file changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c > > index 97b8f12..e1cf70f 100644 > > --- a/src/providers/ldap/sdap.c > > +++ b/src/providers/ldap/sdap.c > > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, > > return EOK; > > } > > > > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, > > - int num_entries, > > - const char *sysdb_attr) > > +/* _already_in_map is set to true if the attribute > > + * already exists in the map and is used for the same > > + * LDAP attribute. > > + * > > + * _conflicts_with_map is set to true if the attribute > > + * already exists in map, but is used for different > > + * LDAP attribute. > > + * */ > > +static void check_duplicate(struct sdap_attr_map *map, > > + int num_entries, > > + const char *sysdb_attr, > > + const char *ldap_attr, > > + bool *_already_in_map, > > + bool *_conflicts_with_map) > > { > This function has 3 output boolean argumets: > It would be better to return enum instead of > adding new parametrs. > > LS
Ok, attached is version with enum.
Michal
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;Hmm, it crahsed even after adding this line.
Self-NACK
BTW the crash was caused by empty entry in map which was caused by skipped existing mapping.
If you do not like interrrating over array with pointers then I can use two indeces.
Updated patch is attched.
LS
On (17/08/16 15:39), Lukas Slebodnik wrote:
On (16/08/16 15:22), Lukas Slebodnik wrote:
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote:
On (10/08/16 20:59), Michal Židek wrote: >On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: >> On (10/08/16 17:41), Michal Židek wrote: >> > Hi, >> > >> > see the attached patch. >> > >> > I modified the detection of duplicates when >> > extending the maps (sysdb_attr:ldap_attr). >> > >> > When we try to add entry to the map >> > that already exists in the map, then >> > without this patch we will fail. >> > >> > With this patch, we only fail if the >> > newly added extension would redefine >> > already existing entry in the map. >> > >> > Otherwise it is just skipped without >> > a failure (we just skip adding what >> > is already there). >> > >> > I created simple CI test for this (first >> > patch). >> > >> > Michal >> >> > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 >> > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com >> > Date: Wed, 10 Aug 2016 15:41:34 +0200 >> > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps >> > >> > When extending map with entry that already >> > exists in the map in the exacty same form, >> > then there is no need to fail. >> > >> > We should only fail if we try to >> > change purpose of already used sysdb >> > attribute. >> > >> > Resolves: >> > https://fedorahosted.org/sssd/ticket/3120 >> > --- >> > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ >> > 1 file changed, 35 insertions(+), 6 deletions(-) >> > >> > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c >> > index 97b8f12..e1cf70f 100644 >> > --- a/src/providers/ldap/sdap.c >> > +++ b/src/providers/ldap/sdap.c >> > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, >> > return EOK; >> > } >> > >> > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, >> > - int num_entries, >> > - const char *sysdb_attr) >> > +/* _already_in_map is set to true if the attribute >> > + * already exists in the map and is used for the same >> > + * LDAP attribute. >> > + * >> > + * _conflicts_with_map is set to true if the attribute >> > + * already exists in map, but is used for different >> > + * LDAP attribute. >> > + * */ >> > +static void check_duplicate(struct sdap_attr_map *map, >> > + int num_entries, >> > + const char *sysdb_attr, >> > + const char *ldap_attr, >> > + bool *_already_in_map, >> > + bool *_conflicts_with_map) >> > { >> This function has 3 output boolean argumets: >> It would be better to return enum instead of >> adding new parametrs. >> >> LS > >Ok, attached is version with enum. > >Michal >
I tried to rest use-case from ticket #3120 http://www.freeipa.org/page/Web_App_Authentication/Example_setup
but sssd_be crashed (gdb) bt #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 #14 0x00007fc2a1a4bd20 in ?? () #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587
it crashed because one agruments from strcasecmp was NULL (dereference of NULL pointer)
I guess that we hit the last value in user_map (zeroed structure) In other words, opts->user_map_cnt does not match reallity.
(gdb) l 482 477 } 478 } 479 } 480 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, 483 NULL, user_name, user_attrs); 484 if (ret) { 485 goto done; 486 }
NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;Hmm, it crahsed even after adding this line.
Self-NACK
BTW the crash was caused by empty entry in map which was caused by skipped existing mapping.
If you do not like interrrating over array with pointers then I can use two indeces.
Updated patch is attched.
and one more time with modified test. The test also check values of extra attributes in cache
The integration tests depens on patches from thread "intg: test nested membership"
LS
On Wed, Aug 17, 2016 at 04:37:43PM +0200, Lukas Slebodnik wrote:
On (17/08/16 15:39), Lukas Slebodnik wrote:
On (16/08/16 15:22), Lukas Slebodnik wrote:
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote:
On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote: > On (10/08/16 20:59), Michal Židek wrote: > >On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: > >> On (10/08/16 17:41), Michal Židek wrote: > >> > Hi, > >> > > >> > see the attached patch. > >> > > >> > I modified the detection of duplicates when > >> > extending the maps (sysdb_attr:ldap_attr). > >> > > >> > When we try to add entry to the map > >> > that already exists in the map, then > >> > without this patch we will fail. > >> > > >> > With this patch, we only fail if the > >> > newly added extension would redefine > >> > already existing entry in the map. > >> > > >> > Otherwise it is just skipped without > >> > a failure (we just skip adding what > >> > is already there). > >> > > >> > I created simple CI test for this (first > >> > patch). > >> > > >> > Michal > >> > >> > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 > >> > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com > >> > Date: Wed, 10 Aug 2016 15:41:34 +0200 > >> > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps > >> > > >> > When extending map with entry that already > >> > exists in the map in the exacty same form, > >> > then there is no need to fail. > >> > > >> > We should only fail if we try to > >> > change purpose of already used sysdb > >> > attribute. > >> > > >> > Resolves: > >> > https://fedorahosted.org/sssd/ticket/3120 > >> > --- > >> > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ > >> > 1 file changed, 35 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c > >> > index 97b8f12..e1cf70f 100644 > >> > --- a/src/providers/ldap/sdap.c > >> > +++ b/src/providers/ldap/sdap.c > >> > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, > >> > return EOK; > >> > } > >> > > >> > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, > >> > - int num_entries, > >> > - const char *sysdb_attr) > >> > +/* _already_in_map is set to true if the attribute > >> > + * already exists in the map and is used for the same > >> > + * LDAP attribute. > >> > + * > >> > + * _conflicts_with_map is set to true if the attribute > >> > + * already exists in map, but is used for different > >> > + * LDAP attribute. > >> > + * */ > >> > +static void check_duplicate(struct sdap_attr_map *map, > >> > + int num_entries, > >> > + const char *sysdb_attr, > >> > + const char *ldap_attr, > >> > + bool *_already_in_map, > >> > + bool *_conflicts_with_map) > >> > { > >> This function has 3 output boolean argumets: > >> It would be better to return enum instead of > >> adding new parametrs. > >> > >> LS > > > >Ok, attached is version with enum. > > > >Michal > > > > I tried to rest use-case from ticket #3120 > http://www.freeipa.org/page/Web_App_Authentication/Example_setup > > but sssd_be crashed > (gdb) bt > #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 > #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 > #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 > #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, > name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 > #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, > now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 > #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 > #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 > #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 > #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 > #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 > #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 > #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 > #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 > #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 > #14 0x00007fc2a1a4bd20 in ?? () > #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so > #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 > #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 > #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587 > > > it crashed because one agruments from strcasecmp was NULL > (dereference of NULL pointer) > > I guess that we hit the last value in user_map (zeroed structure) > In other words, opts->user_map_cnt does not match reallity. > > (gdb) l 482 > 477 } > 478 } > 479 } > 480 > 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { > 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, > 483 NULL, user_name, user_attrs); > 484 if (ret) { > 485 goto done; > 486 } > > NACK
Do you think you can fix the patch with additional one given that this is a pretty bad regression and Michal is out for a couple of weeks?
I will try to look
Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;Hmm, it crahsed even after adding this line.
Self-NACK
BTW the crash was caused by empty entry in map which was caused by skipped existing mapping.
If you do not like interrrating over array with pointers then I can use two indeces.
Updated patch is attched.
and one more time with modified test. The test also check values of extra attributes in cache
The integration tests depens on patches from thread "intg: test nested membership"
LS
From d221c60070d310d82fd5753bab5c14ec0ed64c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
[..]
/* index must be incremented only for appended entry. */++i;
My only comment is that I prefer to use post-increment rather than pre-increment style-wise.
Are you OK with me changing this before pushing?
On (18/08/16 11:41), Jakub Hrozek wrote:
On Wed, Aug 17, 2016 at 04:37:43PM +0200, Lukas Slebodnik wrote:
On (17/08/16 15:39), Lukas Slebodnik wrote:
On (16/08/16 15:22), Lukas Slebodnik wrote:
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote:
On (12/08/16 16:14), Jakub Hrozek wrote: >On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote: >> On (10/08/16 20:59), Michal Židek wrote: >> >On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: >> >> On (10/08/16 17:41), Michal Židek wrote: >> >> > Hi, >> >> > >> >> > see the attached patch. >> >> > >> >> > I modified the detection of duplicates when >> >> > extending the maps (sysdb_attr:ldap_attr). >> >> > >> >> > When we try to add entry to the map >> >> > that already exists in the map, then >> >> > without this patch we will fail. >> >> > >> >> > With this patch, we only fail if the >> >> > newly added extension would redefine >> >> > already existing entry in the map. >> >> > >> >> > Otherwise it is just skipped without >> >> > a failure (we just skip adding what >> >> > is already there). >> >> > >> >> > I created simple CI test for this (first >> >> > patch). >> >> > >> >> > Michal >> >> >> >> > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 >> >> > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com >> >> > Date: Wed, 10 Aug 2016 15:41:34 +0200 >> >> > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps >> >> > >> >> > When extending map with entry that already >> >> > exists in the map in the exacty same form, >> >> > then there is no need to fail. >> >> > >> >> > We should only fail if we try to >> >> > change purpose of already used sysdb >> >> > attribute. >> >> > >> >> > Resolves: >> >> > https://fedorahosted.org/sssd/ticket/3120 >> >> > --- >> >> > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ >> >> > 1 file changed, 35 insertions(+), 6 deletions(-) >> >> > >> >> > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c >> >> > index 97b8f12..e1cf70f 100644 >> >> > --- a/src/providers/ldap/sdap.c >> >> > +++ b/src/providers/ldap/sdap.c >> >> > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, >> >> > return EOK; >> >> > } >> >> > >> >> > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, >> >> > - int num_entries, >> >> > - const char *sysdb_attr) >> >> > +/* _already_in_map is set to true if the attribute >> >> > + * already exists in the map and is used for the same >> >> > + * LDAP attribute. >> >> > + * >> >> > + * _conflicts_with_map is set to true if the attribute >> >> > + * already exists in map, but is used for different >> >> > + * LDAP attribute. >> >> > + * */ >> >> > +static void check_duplicate(struct sdap_attr_map *map, >> >> > + int num_entries, >> >> > + const char *sysdb_attr, >> >> > + const char *ldap_attr, >> >> > + bool *_already_in_map, >> >> > + bool *_conflicts_with_map) >> >> > { >> >> This function has 3 output boolean argumets: >> >> It would be better to return enum instead of >> >> adding new parametrs. >> >> >> >> LS >> > >> >Ok, attached is version with enum. >> > >> >Michal >> > >> >> I tried to rest use-case from ticket #3120 >> http://www.freeipa.org/page/Web_App_Authentication/Example_setup >> >> but sssd_be crashed >> (gdb) bt >> #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 >> #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 >> #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 >> #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, >> name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 >> #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, >> now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 >> #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 >> #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 >> #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 >> #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 >> #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 >> #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 >> #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 >> #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 >> #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 >> #14 0x00007fc2a1a4bd20 in ?? () >> #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so >> #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 >> #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 >> #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587 >> >> >> it crashed because one agruments from strcasecmp was NULL >> (dereference of NULL pointer) >> >> I guess that we hit the last value in user_map (zeroed structure) >> In other words, opts->user_map_cnt does not match reallity. >> >> (gdb) l 482 >> 477 } >> 478 } >> 479 } >> 480 >> 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { >> 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, >> 483 NULL, user_name, user_attrs); >> 484 if (ret) { >> 485 goto done; >> 486 } >> >> NACK > >Do you think you can fix the patch with additional one given that this is >a pretty bad regression and Michal is out for a couple of weeks? I will try to look
Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;Hmm, it crahsed even after adding this line.
Self-NACK
BTW the crash was caused by empty entry in map which was caused by skipped existing mapping.
If you do not like interrrating over array with pointers then I can use two indeces.
Updated patch is attched.
and one more time with modified test. The test also check values of extra attributes in cache
The integration tests depens on patches from thread "intg: test nested membership"
LS
From d221c60070d310d82fd5753bab5c14ec0ed64c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
[..]
/* index must be incremented only for appended entry. */++i;My only comment is that I prefer to use post-increment rather than pre-increment style-wise.
Are you OK with me changing this before pushing?
Sure
LS
On Thu, Aug 18, 2016 at 12:04:46PM +0200, Lukas Slebodnik wrote:
On (18/08/16 11:41), Jakub Hrozek wrote:
On Wed, Aug 17, 2016 at 04:37:43PM +0200, Lukas Slebodnik wrote:
On (17/08/16 15:39), Lukas Slebodnik wrote:
On (16/08/16 15:22), Lukas Slebodnik wrote:
On (16/08/16 11:50), Lukas Slebodnik wrote:
On (12/08/16 16:30), Lukas Slebodnik wrote: >On (12/08/16 16:14), Jakub Hrozek wrote: >>On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote: >>> On (10/08/16 20:59), Michal Židek wrote: >>> >On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: >>> >> On (10/08/16 17:41), Michal Židek wrote: >>> >> > Hi, >>> >> > >>> >> > see the attached patch. >>> >> > >>> >> > I modified the detection of duplicates when >>> >> > extending the maps (sysdb_attr:ldap_attr). >>> >> > >>> >> > When we try to add entry to the map >>> >> > that already exists in the map, then >>> >> > without this patch we will fail. >>> >> > >>> >> > With this patch, we only fail if the >>> >> > newly added extension would redefine >>> >> > already existing entry in the map. >>> >> > >>> >> > Otherwise it is just skipped without >>> >> > a failure (we just skip adding what >>> >> > is already there). >>> >> > >>> >> > I created simple CI test for this (first >>> >> > patch). >>> >> > >>> >> > Michal >>> >> >>> >> > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 >>> >> > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com >>> >> > Date: Wed, 10 Aug 2016 15:41:34 +0200 >>> >> > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps >>> >> > >>> >> > When extending map with entry that already >>> >> > exists in the map in the exacty same form, >>> >> > then there is no need to fail. >>> >> > >>> >> > We should only fail if we try to >>> >> > change purpose of already used sysdb >>> >> > attribute. >>> >> > >>> >> > Resolves: >>> >> > https://fedorahosted.org/sssd/ticket/3120 >>> >> > --- >>> >> > src/providers/ldap/sdap.c | 41 +++++++++++++++++++++++++++++++++++------ >>> >> > 1 file changed, 35 insertions(+), 6 deletions(-) >>> >> > >>> >> > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c >>> >> > index 97b8f12..e1cf70f 100644 >>> >> > --- a/src/providers/ldap/sdap.c >>> >> > +++ b/src/providers/ldap/sdap.c >>> >> > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, >>> >> > return EOK; >>> >> > } >>> >> > >>> >> > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, >>> >> > - int num_entries, >>> >> > - const char *sysdb_attr) >>> >> > +/* _already_in_map is set to true if the attribute >>> >> > + * already exists in the map and is used for the same >>> >> > + * LDAP attribute. >>> >> > + * >>> >> > + * _conflicts_with_map is set to true if the attribute >>> >> > + * already exists in map, but is used for different >>> >> > + * LDAP attribute. >>> >> > + * */ >>> >> > +static void check_duplicate(struct sdap_attr_map *map, >>> >> > + int num_entries, >>> >> > + const char *sysdb_attr, >>> >> > + const char *ldap_attr, >>> >> > + bool *_already_in_map, >>> >> > + bool *_conflicts_with_map) >>> >> > { >>> >> This function has 3 output boolean argumets: >>> >> It would be better to return enum instead of >>> >> adding new parametrs. >>> >> >>> >> LS >>> > >>> >Ok, attached is version with enum. >>> > >>> >Michal >>> > >>> >>> I tried to rest use-case from ticket #3120 >>> http://www.freeipa.org/page/Web_App_Authentication/Example_setup >>> >>> but sssd_be crashed >>> (gdb) bt >>> #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 >>> #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 >>> #2 0x00007fc29f199fad in sysdb_attrs_get_el (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 >>> #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, >>> name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at src/providers/ldap/sdap_utils.c:40 >>> #4 0x00007fc28fe1a2c7 in sdap_save_user (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, dom=0x7fc2a1a54ae0, attrs=<optimized out>, _usn_value=_usn_value@entry=0x0, >>> now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 >>> #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at src/providers/ldap/sdap_async_initgroups.c:2961 >>> #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 >>> #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:1578 >>> #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, sh=<optimized out>, msg=0x7fc2a1aba9f0) at src/providers/ldap/sdap_async.c:353 >>> #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at src/providers/ldap/sdap_async.c:197 >>> #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 >>> #11 0x00007fc29b860b5a in epoll_event_loop_once () from /lib64/libtevent.so.0 >>> #12 0x00007fc29b85f257 in std_event_context_init () from /lib64/libtevent.so.0 >>> #13 0x00007fc29b85b40d in _tevent_loop_until () from /lib64/libtevent.so.0 >>> #14 0x00007fc2a1a4bd20 in ?? () >>> #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so >>> #16 0x00007fc29b85f1f7 in std_event_loop_once () from /lib64/libtevent.so.0 >>> #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at src/util/server.c:702 >>> #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:587 >>> >>> >>> it crashed because one agruments from strcasecmp was NULL >>> (dereference of NULL pointer) >>> >>> I guess that we hit the last value in user_map (zeroed structure) >>> In other words, opts->user_map_cnt does not match reallity. >>> >>> (gdb) l 482 >>> 477 } >>> 478 } >>> 479 } >>> 480 >>> 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; i++) { >>> 482 ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, >>> 483 NULL, user_name, user_attrs); >>> 484 if (ret) { >>> 485 goto done; >>> 486 } >>> >>> NACK >> >>Do you think you can fix the patch with additional one given that this is >>a pretty bad regression and Michal is out for a couple of weeks? >I will try to look > Here you are
LS
From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
Resolves: https://fedorahosted.org/sssd/ticket/3120
Signed-off-by: Lukas Slebodnik lslebodn@redhat.com
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; }
-static bool is_sysdb_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr)+enum duplicate_t {
- NOT_FOUND = 0,
- ALREADY_IN_MAP, /* nothing to add */
- CONFLICT_WITH_MAP /* attempt to redefine attribute */
+};
+static enum duplicate_t check_duplicate(struct sdap_attr_map *map,
int num_entries,const char *sysdb_attr,const char *ldap_attr){ int i;
for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) {
return true;
if (strcmp(map[i].name, ldap_attr) == 0) {return ALREADY_IN_MAP;} else {return CONFLICT_WITH_MAP;} }
- return false;
- return NOT_FOUND;
}
int sdap_extend_map(TALLOC_CTX *memctx, @@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; }
if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) {
ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr);if (ret == ALREADY_IN_MAP) {DEBUG(SSSDBG_TRACE_FUNC,"Attribute %s (%s in LDAP) is already in map.\n",sysdb_attr, ldap_attr);--nextra;Hmm, it crahsed even after adding this line.
Self-NACK
BTW the crash was caused by empty entry in map which was caused by skipped existing mapping.
If you do not like interrrating over array with pointers then I can use two indeces.
Updated patch is attched.
and one more time with modified test. The test also check values of extra attributes in cache
The integration tests depens on patches from thread "intg: test nested membership"
LS
From d221c60070d310d82fd5753bab5c14ec0ed64c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps
When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail.
We should only fail if we try to change purpose of already used sysdb attribute.
[..]
/* index must be incremented only for appended entry. */++i;My only comment is that I prefer to use post-increment rather than pre-increment style-wise.
Are you OK with me changing this before pushing?
Sure
ACK, then.
On Fri, Aug 19, 2016 at 10:36:01AM +0200, Jakub Hrozek wrote:
ACK, then.
* master: * 133647422d6e276a597494002873b2afce1d12a7 * b6bc67f3272d8a45fb6b5c01c8a3f8e74010eb71
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek