[SSSD] [PATCH] sdap_idmap: properly handle ranges for external mappings

Jakub Hrozek jhrozek at redhat.com
Thu Oct 17 11:54:02 UTC 2013


On Thu, Oct 17, 2013 at 12:47:25PM +0200, Jakub Hrozek wrote:
> 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 at 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 at 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 at 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 at 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 at 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.

Pushed all to master and sssd-1-11



More information about the sssd-devel mailing list