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(a)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(a)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(a)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