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(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
>
>> From d221c60070d310d82fd5753bab5c14ec0ed64c93 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
>
>[..]
>
>> + /* 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