On Wed, Oct 09, 2013 at 04:08:16PM +0200, Sumit Bose wrote:
> Hi,
>
> this patch set aims to fix
https://fedorahosted.org/sssd/ticket/2105 .
> But since I had to touch the libsss_idmap code for this I found an issue
> here as well, which is fixed by the first two patches.
>
> bye,
> Sumit
> From 5e9daa3af6b1d5ef178ad8737e98908543aa66eb Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 9 Oct 2013 15:20:38 +0200
> Subject: [PATCH 1/5] idmap: add internal function to free a domain struct
ACK
>
> ---
> src/lib/idmap/sss_idmap.c | 19 +++++++++++++++----
> 1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
> index c7ac0c7..45797f2 100644
> --- a/src/lib/idmap/sss_idmap.c
> +++ b/src/lib/idmap/sss_idmap.c
> @@ -213,6 +213,20 @@ enum idmap_error_code sss_idmap_init(idmap_alloc_func
*alloc_func,
> return IDMAP_SUCCESS;
> }
>
> +static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
> + struct idmap_domain_info *dom)
> +{
> + if (ctx == NULL || dom == NULL) {
> + return;
> + }
> +
> + ctx->free_func(dom->range_id, ctx->alloc_pvt);
This is a nice catch and could be a leak with other allocator than
talloc.
> + ctx->free_func(dom->range, ctx->alloc_pvt);
> + ctx->free_func(dom->name, ctx->alloc_pvt);
> + ctx->free_func(dom->sid, ctx->alloc_pvt);
> + ctx->free_func(dom, ctx->alloc_pvt);
> +}
> +
> From e7263a273d6f0aa1d4ec83a9f9bcbd32d65e23c8 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 9 Oct 2013 15:21:48 +0200
> Subject: [PATCH 2/5] idmap: fix a memory leak if a collision is detected
I haven't tested the collision itself, but all paths that use the fail
label have the "err" variable set and cleanup function is called on
failure.
ACK.
> From c2af969630611dc1fc91d86e1d6141746c2cc82b Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 9 Oct 2013 15:22:53 +0200
> Subject: [PATCH 3/5] idmap: allow ranges with external mapping to overlap
>
> If POSIX IDs are managed externally e.g. by AD it might be possible that
> the IDs are centrally manages for the whole forest. Hence there might
> not be a single ID range for each member domain in the forest but only a
> single ID range for the whole forest. This means that we have to allow
> collisions if ID ranges in this case.
>
> Unit tests are added to make sure that the collisions are only allowed
> for external mappings.
ACK, thank you for the unit test.
> From af4ca9eec65e4bae6ed399bf2136c7a8542f2e59 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 9 Oct 2013 15:30:29 +0200
> Subject: [PATCH 4/5] sdap_idmap: add
> sdap_idmap_get_configured_external_range()
ACK, looks good and was tested as part of the next patch.
> From 96387421488982ef67dd90e6eb7653c2c9cff04d Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 9 Oct 2013 15:31:33 +0200
> Subject: [PATCH 5/5] sdap_idmap: properly handle ranges for external mappings
>
> Currently we relied on the fact that external ID mapping is used as
> default fallback in case of an error and did not properly add subdomains
> with external ID mapping to the idmap library. If debugging is enabled
> this leads to irritating debug messages for every user or group lookup.
>
> With this patch this subdomains are added to the idmap library.
>
> Fixes
https://fedorahosted.org/sssd/ticket/2105
> ---
The warning is gone when using POSIX attributes. I tested both with and
without ID mapping and both cases still work fine.
There is just a typo (this patch this) that I will fix before pushing.
ACK.