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?= <[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 > >> >> > >> >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?= <[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 > > > >[..] > > > >> + /* 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. CI: http://sssd-ci.duckdns.org/logs/job/52/09/summary.html _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
