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.