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?= <[email protected]> >>>> >> > 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?= <[email protected]> >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 <[email protected]> >--- > 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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
