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.
LS