On 01/06/2016 04:34 PM, Sumit Bose wrote:
On Mon, Jan 04, 2016 at 03:40:05PM +0100, Pavel Reichl wrote:
> Hello Sumit, thanks for comments. I attached rebased patch with fixes as you
proposed.
>
> On 12/10/2015 01:23 PM, Sumit Bose wrote:
>
>> Some unit-test where failing for me because of an issue in cleaning up
>> the secondary slices. 'it = it->next' does not work because it was
just
>> freed in the body of the loop. I fixed it with the change below, but
>> maybe a do-while loop would be even nicer here?
>
> Sorry to hear that. I fixed that in attached patch set.
> Which test failed for you? make check is always passing in my environment.
This is what I see:
$ ./sss_idmap-tests
Running suite(s): IDMAP
88%: Checks: 25, Failures: 0, Errors: 3
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ss:0: (after this point) Received
signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_uid2sid_ss:0: (after this point) Received
signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ext_sec_slices:0: (after this point)
Received signal 11 (Segmentation fault)
and here is what valgrind has to say:
$ valgrind .libs/lt-sss_idmap-tests
==17919== Memcheck, a memory error detector
==17919== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==17919== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==17919== Command: .libs/lt-sss_idmap-tests
==17919==
Running suite(s): IDMAP
==17919== Invalid read of size 4
==17919== at 0x407C595: sss_idmap_free_domain.part.1 (sss_idmap.c:220)
==17919== by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919== by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919== by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919== by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x80492D1: main (sss_idmap-tests.c:747)
==17919== Address 0x4437d68 is 64 bytes inside a block of size 68 free'd
==17919== at 0x402C26D: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==17919== by 0x4068C9E: _talloc_free_internal (talloc.c:1057)
==17919== by 0x4068C9E: _talloc_free (talloc.c:1581)
==17919== by 0x407C594: sss_idmap_free_domain.part.1 (sss_idmap.c:222)
==17919== by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919== by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919== by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919== by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919== by 0x80492D1: main (sss_idmap-tests.c:747)
==17919==
100%: Checks: 25, Failures: 0, Errors: 0
==17919==
==17919== HEAP SUMMARY:
==17919== in use at exit: 0 bytes in 0 blocks
==17919== total heap usage: 1,918 allocs, 1,918 frees, 749,697 bytes allocated
==17919==
==17919== All heap blocks were freed -- no leaks are possible
==17919==
==17919== For counts of detected and suppressed errors, rerun with: -v
==17919== ERROR SUMMARY: 32 errors from 1 contexts (suppressed: 0 from 0)
>
>>
>> diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
>> index c4ae811..7ab5c66 100644
>> --- a/src/lib/idmap/sss_idmap.c
>> +++ b/src/lib/idmap/sss_idmap.c
>> @@ -210,6 +210,7 @@ static void sss_idmap_free_domain(struct
>> sss_idmap_ctx *ctx,
>> struct idmap_domain_info *dom)
>> {
>> struct idmap_range_params *it;
>> + struct idmap_range_params *next;
>>
>> if (ctx == NULL || dom == NULL) {
>> return;
>> @@ -217,7 +218,8 @@ static void sss_idmap_free_domain(struct
>> sss_idmap_ctx *ctx,
>>
>> ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
>> /* Free list of secondary ranges */
>> - for (it = dom->sec_ranges; it != NULL; it = it->next) {
>> + for (it = dom->sec_ranges; it != NULL; it = next) {
>> + next = it->next;
>> ctx->free_func(it->range_id, ctx->alloc_pvt);
>> ctx->free_func(it, ctx->alloc_pvt);
>> }
>>
>>
>> While checking and playing with the test I came across
>>
>> + /* Todo - Add tests */
>> + /* Add size of primary slice for first_rid of secondary slices. */
>> + rid += ctx->idmap_opts.rangesize;
>>
>> in sss_idmap_add_auto_domain_ex(). Does the 'Todo' only refer to
'Add
>> tests' or to 'Add size of primary slice for first_rid of secondary
>> slices.' as well? I'm asking because you really have to use the size of
>> the primary slice and not ctx->idmap_opts.rangesize here because the
>> primary range is given as a parameter to sss_idmap_add_auto_domain_ex()
>> and the size might be different then ctx->idmap_opts.rangesize. E.g. in
>> the unit-test the range is [1234,9876].
>
> I removed the misleading comment about adding tests. I have no intention of changing
how 'rid' is set currently.
>
>>
>> About sss_idmap_add_auto_domain_ex(), I think having auto-generated
>> secondary ranges with external_mapping==true make no sense. I would
>> suggest to just create the primary range in this case.
>
> OK, done.
>
>>
>> I'll continue reviewing and testing ...
>
> Thanks!
> From e0aa542e1f1329583eb89d166df87185cffd6362 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 26 Nov 2015 10:46:34 -0500
> Subject: [PATCH 1/2] IDMAP: New structure for domain range params
>
> Create new internal structure idmap_range_params by merging ID mapping
> range relevant fields from idmap_domain_info and remove corrsponding
> fields.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2188
> ---
> src/lib/idmap/sss_idmap.c | 110 +++++++++++++++++++++++-----------------------
> 1 file changed, 56 insertions(+), 54 deletions(-)
>
...
> @@ -447,8 +437,13 @@ enum idmap_error_code sss_idmap_check_collision(struct
sss_idmap_ctx *ctx,
> enum idmap_error_code err;
>
> for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
> - err = sss_idmap_check_collision_ex(dom->name, dom->sid,
dom->range,
> - dom->first_rid, dom->range_id,
> + struct sss_idmap_range range = { dom->range_params.min_id,
> + dom->range_params.max_id };
In general I agree with the refactoring in this patch, but I don't like
the declaration of variables in the code. Especially in the case here
and below where variables are declared inside a loop. Mainly because I
do not know if compilers are able handle this smart or if this might be
more expensive than declaring the variables outside of the loop.
As discussed in separate thread I moved all declaration to the beginning of the
functions.
> +
> + err = sss_idmap_check_collision_ex(dom->name, dom->sid,
> + &range,
> + dom->range_params.first_rid,
> + dom->range_params.range_id,
> dom->external_mapping,
> n_name, n_sid, n_range, n_first_rid,
> n_range_id, n_external_mapping);
> @@ -467,12 +462,22 @@ static enum idmap_error_code dom_check_collision(
> enum idmap_error_code err;
>
> for (dom = dom_list; dom != NULL; dom = dom->next) {
> - err = sss_idmap_check_collision_ex(dom->name, dom->sid,
dom->range,
> - dom->first_rid, dom->range_id,
> + struct sss_idmap_range range = { dom->range_params.min_id,
> + dom->range_params.max_id };
> + struct sss_idmap_range new_dom_range = {
> + new_dom->range_params.min_id,
> + new_dom->range_params.max_id };
> +
> +
> + err = sss_idmap_check_collision_ex(dom->name, dom->sid,
> + &range,
> + dom->range_params.first_rid,
> + dom->range_params.range_id,
> dom->external_mapping,
> new_dom->name, new_dom->sid,
> - new_dom->range,
new_dom->first_rid,
> - new_dom->range_id,
> + &new_dom_range,
> + new_dom->range_params.first_rid,
> + new_dom->range_params.range_id,
> new_dom->external_mapping);
> if (err != IDMAP_SUCCESS) {
> return err;
...
> From ea48f8b5c252755168e9ae83d4b7ebb97152ea3c Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Fri, 27 Nov 2015 04:15:00 -0500
> Subject: [PATCH 2/2] IDMAP: introduce secondary slices
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2188
> ---
> src/config/SSSDConfig/__init__.py.in | 1 +
> src/config/etc/sssd.api.d/sssd-ad.conf | 1 +
> src/config/etc/sssd.api.d/sssd-ipa.conf | 1 +
> src/config/etc/sssd.api.d/sssd-ldap.conf | 1 +
> src/lib/idmap/sss_idmap.c | 430 +++++++++++++++++++++++++++----
> src/lib/idmap/sss_idmap.exports | 3 +
> src/lib/idmap/sss_idmap.h | 58 +++++
> src/lib/idmap/sss_idmap_private.h | 4 +
> src/man/include/ldap_id_mapping.xml | 11 +
> src/providers/ad/ad_opts.c | 1 +
> src/providers/ipa/ipa_opts.c | 1 +
> src/providers/ldap/ldap_opts.c | 1 +
> src/providers/ldap/sdap.h | 1 +
> src/providers/ldap/sdap_idmap.c | 15 +-
> src/tests/cmocka/test_sss_idmap.c | 86 ++++++-
> src/tests/sss_idmap-tests.c | 114 ++++++++
> 16 files changed, 667 insertions(+), 62 deletions(-)
>
> diff --git a/src/config/SSSDConfig/__init__.py.in
b/src/config/SSSDConfig/__init__.py.in
> index
fe2971d993ccf2975bbddbe6c74572ab9f5d4e51..7f9ced668b4b57c90ed4da4f49f56bf42b172304 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -354,6 +354,7 @@ option_strings = {
> 'ldap_idmap_autorid_compat' : _('Use autorid-compatible algorithm
for ID-mapping'),
> 'ldap_idmap_default_domain' : _('Name of the default domain for
ID-mapping'),
> 'ldap_idmap_default_domain_sid' : _('SID of the default domain for
ID-mapping'),
> + 'ldap_idmap_extra_slice_max' : _('Number of secondary slices'),
I think the _max might be a bit misleading here because it is possible
that more secondary slices can be used, this value just gives the number
of slices which are created at startup. So something like _initial,
_init or _start might fit better.
Btw, I'm not sure if it is useful, but currently there is no way to
switch of the usage of the secondary slices. Even setting
ldap_idmap_extra_slice_max to 0 will only skip the initial creation of
the slices but new slices will still be created when needed.
I changes the code so ldap_idmap_extra_slice_init == 0 should block adding new slices.
...
> enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
> const char *dom_sid,
> id_t *slice_num,
> @@ -327,12 +366,9 @@ enum idmap_error_code sss_idmap_calculate_range(struct
sss_idmap_ctx *ctx,
> /* Verify that this slice is not already in use */
> do {
> for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next)
{
> - uint32_t dmin = dom->range_params.min_id;
> - uint32_t dmax = dom->range_params.max_id;
>
> - if ((dmin <= min && dmax >= max) ||
> - (dmin >= min && dmin <= max) ||
> - (dmax >= min && dmax <= max)) {
> + if (check_overlap(&dom->range_params, min, max) ||
> + check_collision_sec_slices(dom->sec_ranges, min, max)) {
I think it would be better to have a single call which checks for
overlaps/collisions.
OK done.
> /* This range overlaps one already
registered
> * We'll try the next available slot
> */
...
> +static enum idmap_error_code
> +get_sec_slices(struct sss_idmap_ctx *ctx,
> + const char *domain_name,
> + uint32_t first_rid,
> + struct idmap_range_params **_sec_slices)
> +{
> + struct idmap_range_params *prev = NULL;
> + struct idmap_range_params *sec_slices = NULL;
> +
> + for (int i = 0; i < ctx->idmap_opts.extra_slice_max; i++) {
> + static enum idmap_error_code err;
> + struct idmap_range_params *slice;
> + char *secondary_name;
> +
> + secondary_name = generate_sec_slice_name(ctx, domain_name, i);
The number of the first secondary slice should be 1 otherwise there
would be a mismatch with get_new_sec_slide(), so please replace 'i' with
'i + 1'. Unfortunately you have to change some of the id values in the
tests because of this.
OK, done.
> + if (secondary_name == NULL) {
> + return IDMAP_OUT_OF_MEMORY;
> + }
> +
> + err = generate_slice(ctx, secondary_name, first_rid, &slice);
> + if (err != IDMAP_SUCCESS) {
> + ctx->free_func(secondary_name, ctx->alloc_pvt);
> + return err;
> + }
> +
> + first_rid += ctx->idmap_opts.rangesize;
> +
> + if (prev != NULL) {
> + prev->next = slice;
> + }
> +
> + if (sec_slices == NULL) {
> + sec_slices = slice;
> + }
> +
> + prev = slice;
> + }
> +
> + *_sec_slices = sec_slices;
> + return IDMAP_SUCCESS;
> +}
> +
> enum idmap_error_code sss_idmap_add_domain_ex(struct sss_idmap_ctx *ctx,
> const char *domain_name,
> const char *domain_sid,
> @@ -566,6 +702,48 @@ fail:
> return err;
> }
>
> +enum idmap_error_code
> +sss_idmap_add_auto_domain_ex(struct sss_idmap_ctx *ctx,
> + const char *domain_name,
> + const char *domain_sid,
> + struct sss_idmap_range *range,
> + const char *range_id,
> + uint32_t rid,
> + bool external_mapping)
> +{
> + enum idmap_error_code err;
> +
I think a check should be added if the size of the range given in the
sss_idmap_range struct is really idmap_opts.rangesize, otherwise we
would run into trouble.
OK done.
> + err = sss_idmap_add_domain_ex(ctx, domain_name, domain_sid,
range,
> + range_id, rid, external_mapping);
> + if (err != IDMAP_SUCCESS) {
> + return err;
> + }
> +
> + if (external_mapping) {
> + /* There's no point in generating secondary ranges if external_mapping
> + is enabled. */
> + ctx->idmap_domain_info->use_sec_ranges = false;
> + return IDMAP_SUCCESS;
> + }
> +
> + /* Add size of primary slice for first_rid of secondary slices. */
> + rid += ctx->idmap_opts.rangesize;
> + err = get_sec_slices(ctx, domain_name, rid,
> + &ctx->idmap_domain_info->sec_ranges);
> + if (err == IDMAP_SUCCESS) {
> + ctx->idmap_domain_info->use_sec_ranges = true;
> + } else {
> + /* Running out of slices for secondary mapping is a non-fatal
> + * problem. */
> + if (err == IDMAP_OUT_OF_SLICES) {
> + err = IDMAP_SUCCESS;
> + }
> + ctx->idmap_domain_info->use_sec_ranges = false;
> + }
> +
> + return err;
> +}
> +
> enum idmap_error_code sss_idmap_add_domain(struct sss_idmap_ctx *ctx,
> const char *domain_name,
> const char *domain_sid,
...
> +static bool comp_id(struct idmap_range_params *range_params, long long rid,
> + uint32_t *_id)
> +{
> + uint32_t id;
> +
> + if (rid >= range_params->first_rid) {
> + id = range_params->min_id + (rid - range_params->first_rid);
> + if (id <= range_params->max_id) {
By default we restrict the POSIX IDs to 2^31 and since the maximum for
uint32_t is 2^32 there should be no overrun. But we allow to configure
ldap_idmap_range_max and hence there might be the chance for overruns.
There are nice built-ins
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html which
are supported by clang
http://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-bui...
as well. But I'm not sure if we should start using them?
I would like to use them in future.
So I would suggest something like
if (rid >= range_params->first_rid &&
(UINT32_MAX - range_params->min_id) > (rid -
range_params->first_rid)) {
id = range_params->min_id + (rid - range_params->first_rid);
if (id <= range_params->max_id) {
to return false on overflows (Please check the math, I carefully looked
at it for some time and think it is ok, but an addition pair of eyes and
the corresponding brain are very welcome :-)
I added this statement to the code. LGTM.
> + *_id = id;
> + return true;
> + }
> + }
> + return false;
> +}
> +
...
> enum idmap_error_code sss_idmap_sid_to_unix(struct sss_idmap_ctx *ctx,
> const char *sid,
> uint32_t *_id)
> {
> struct idmap_domain_info *idmap_domain_info;
> + struct idmap_domain_info *matched_dom = NULL;
> size_t dom_len;
> long long rid;
> - char *endptr;
> - uint32_t id;
> - bool no_range = false;
>
> if (sid == NULL || _id == NULL) {
> return IDMAP_ERROR;
> @@ -607,39 +862,68 @@ enum idmap_error_code sss_idmap_sid_to_unix(struct
sss_idmap_ctx *ctx,
> return IDMAP_BUILTIN_SID;
> }
>
> + /* Try primary slices */
> while (idmap_domain_info != NULL) {
> - if (idmap_domain_info->sid != NULL) {
> - dom_len = strlen(idmap_domain_info->sid);
> - if (strlen(sid) > dom_len && sid[dom_len] == '-'
> - && strncmp(sid, idmap_domain_info->sid, dom_len) ==
0) {
>
> - if (idmap_domain_info->external_mapping == true) {
> - return IDMAP_EXTERNAL;
> - }
> + if (is_sid_from_dom(idmap_domain_info->sid, sid, &dom_len)) {
>
> - errno = 0;
> - rid = strtoull(sid + dom_len + 1, &endptr, 10);
> - if (errno != 0 || rid > UINT32_MAX || *endptr != '\0') {
> - return IDMAP_SID_INVALID;
> - }
> + if (idmap_domain_info->external_mapping == true) {
> + return IDMAP_EXTERNAL;
> + }
>
> - if (rid >= idmap_domain_info->range_params.first_rid) {
> - id = idmap_domain_info->range_params.min_id
> - + (rid - idmap_domain_info->range_params.first_rid);
> - if (id <= idmap_domain_info->range_params.max_id) {
> - *_id = id;
> - return IDMAP_SUCCESS;
> - }
> - }
> + if (parse_rid(sid, dom_len, &rid) == false) {
> + return IDMAP_SID_INVALID;
> + }
>
> - no_range = true;
> + if (comp_id(&idmap_domain_info->range_params, rid, _id)) {
> + return IDMAP_SUCCESS;
> }
> +
> + matched_dom = idmap_domain_info;
> }
>
> idmap_domain_info = idmap_domain_info->next;
> }
>
> - return no_range ? IDMAP_NO_RANGE : IDMAP_NO_DOMAIN;
> + /* Try secondary slices */
> + if (matched_dom != NULL && matched_dom->use_sec_ranges) {
maybe this part can be put into a separate function as well ?
OK done.
> + struct idmap_range_params *last_slide = NULL;
> + struct idmap_range_params *new_slice;
> + enum idmap_error_code err;
> +
> + dom_len = strlen(matched_dom->sid);
> +
> + if (parse_rid(sid, dom_len, &rid) == false) {
> + return IDMAP_SID_INVALID;
> + }
> +
> + for (struct idmap_range_params *it = matched_dom->sec_ranges;
> + it != NULL; it = it->next) {
> + if (comp_id(it, rid, _id)) {
> + return IDMAP_SUCCESS;
> + }
> + last_slide = it;
> + }
> +
> + /* If no secondary slide matched then new slide is added. */
> + err = get_new_sec_slide(ctx, matched_dom->name, rid, &new_slice);
> + if (err != IDMAP_SUCCESS) {
> + return err;
> + }
> +
> + if (last_slide == NULL) {
> + matched_dom->sec_ranges = new_slice;
> + } else {
> + last_slide->next = new_slice;
> + }
> +
> + if (comp_id(new_slice, rid, _id)) {
> + return IDMAP_SUCCESS;
> + }
> +
> + }
> +
> + return matched_dom ? IDMAP_NO_RANGE : IDMAP_NO_DOMAIN;
> }
>
> enum idmap_error_code sss_idmap_check_sid_unix(struct sss_idmap_ctx *ctx,
...
> diff --git a/src/lib/idmap/sss_idmap.exports
> b/src/lib/idmap/sss_idmap.exports
> index
> 52115636d5a6b936f18b4392e9d12adc26c85f53..d4d3d0cb50146ba62b1bbd068420be2e6ea83976
> 100644
> --- a/src/lib/idmap/sss_idmap.exports
> +++ b/src/lib/idmap/sss_idmap.exports
> @@ -8,13 +8,16 @@ SSS_IDMAP_0.4 {
I think you have to increase the version number.
> sss_idmap_ctx_set_lower;
> sss_idmap_ctx_set_upper;
> sss_idmap_ctx_set_rangesize;
> + sss_idmap_ctx_set_extra_slice_max;
> sss_idmap_ctx_get_autorid;
> sss_idmap_ctx_get_lower;
> sss_idmap_ctx_get_upper;
> sss_idmap_ctx_get_rangesize;
> + sss_idmap_ctx_get_extra_slice_max;
> sss_idmap_calculate_range;
> sss_idmap_add_domain;
> sss_idmap_add_domain_ex;
> + sss_idmap_add_auto_domain_ex;
> sss_idmap_check_collision;
> sss_idmap_check_collision_ex;
> sss_idmap_sid_to_unix;
...
I would add a test where you set the idmap parameters to some smaller
values than the default, e.g. range size to 10 and the number of initial
slices to 5. Then initialize the id-mapping for some domain and run a
loop with rid values from 0 to 80, call sss_idmap_sid_to_unix() and save
the POSIX IDs in an array. Then run sss_idmap_unix_to_sid() with the
values from the array and check if the expected SID is returned. This
will exercise adding more slices than initialized at startup and make
sure that there are no gaps between the slices or off-by-one errors at
the boundary of the slices.
OK, I will work on this now.
Additionally I think it would be more reliable if the secondary slices
can be saved to the cache and can be read at startup as well. I'm
thinking of the case when there is a collision in the secondary slices
of different domains. Currently we try to find a unused slice if a
collision is detected. But if after a restart the slices are initialized
in a different order the colliding slices might get swapped. Maybe
callbacks can be added to sss_idmap_add_auto_domain_ex() which can be
set to a function which can reads and writes the data from the cache?
OK, I will work on this after the test is done.
Thanks.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org