On Thu, Nov 15, 2012 at 12:55:37PM +0100, Jakub Hrozek wrote:
> On Thu, Nov 15, 2012 at 10:57:37AM +0100, Jakub Hrozek wrote:
> > We broke saving nested LDAP groups with no members in 1.9 during the
> > conversion to ghost users. The attached patches fix that.
> >
> > The first three patches would be nice to get into 1.9, the last patch is
> > OK in master only. I just found the code hard to read sometimes so I
> > split it into a separate functions and used more descriptive variable
> > names. I also think that the branch that the branch that re-read
> > SDAP_AT_GROUP_MEMBER after reading SYSDB_GHOST is not needed.
>
> Sorry, self-nack. Changing the conditin was not quite correct in patch
> #2. New set is attached.
I changed the condition but managed to send patches with the wrong
one..correct set is attached.
>From 4306d117a74d627308de04d8401f9c4ce395f942 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 15 Nov 2012 07:11:33 +0100
Subject: [PATCH 1/4] LDAP: Allocate the temporary context on NULL, not memctx
Allocating temporary context on NULL helps vind memory leaks with
valgrind and avoid growing memory over time by allocating on a
long-lived context.
---
src/providers/ldap/sdap_async_groups.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
index
67dddae70030c8de8326918b6b1e17349af7e33a..a4891e33d9f1fd03b2be21e587111b3a54a4449f
100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -226,7 +226,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
hash_key_t key;
hash_value_t value;
- tmpctx = talloc_new(memctx);
+ tmpctx = talloc_new(NULL);
if (!tmpctx) {
ret = ENOMEM;
goto fail;
--
1.8.0
>From 60ea6ff99ef7fd47bae609fcd653a9c3b4f6e56b Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 15 Nov 2012 07:33:30 +0100
Subject: [PATCH 2/4] LDAP: Fix saving empty groups
https://fedorahosted.org/sssd/ticket/1647
A logic bug in the LDAP provider causes an attempt to allocate a zero-length
array for group members while processing an empty group. The allocation
would return NULL and saving the empty group would fail.
---
src/providers/ldap/sdap_async_groups.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
index
a4891e33d9f1fd03b2be21e587111b3a54a4449f..02c07afcca69d79c089555689a7ba7b42eede235
100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -433,9 +433,11 @@ static int sdap_save_group(TALLOC_CTX *memctx,
el->values = gh->values;
el->num_values = gh->num_values;
+ cnt = el->num_values + el1->num_values;
+ DEBUG(SSSDBG_TRACE_FUNC, ("Group %s has %d members\n", name, cnt));
+
/* Now process RFC2307bis ghost hash table */
- if (ghosts != NULL) {
- cnt = el->num_values + el1->num_values;
+ if (ghosts && cnt > 0) {
el->values = talloc_realloc(attrs, el->values, struct ldb_val,
cnt);
if (el->values == NULL) {
--
1.8.0
>From 02d4f8f665c3f0207f1c2bfb8368925ede3d0b07 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 15 Nov 2012 10:02:54 +0100
Subject: [PATCH 3/4] LDAP: use the correct memory context
The element being reallocated is part of the "group_attrs" array, not
attrs.
---
src/providers/ldap/sdap_async_groups.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
index
02c07afcca69d79c089555689a7ba7b42eede235..30069f23660989a4e2b9e5658e7b4724ca141373
100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -438,7 +438,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
/* Now process RFC2307bis ghost hash table */
if (ghosts && cnt > 0) {
- el->values = talloc_realloc(attrs, el->values, struct ldb_val,
+ el->values = talloc_realloc(group_attrs, el->values, struct ldb_val,
cnt);
if (el->values == NULL) {
ret = ENOMEM;
--
1.8.0
>From b1c67a255b4cb7e5efa497850cf522929770c5b4 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 15 Nov 2012 09:59:48 +0100
Subject: [PATCH 4/4] LDAP: Refactor saving ghost users
---
src/providers/ldap/sdap_async_groups.c | 187 +++++++++++++++++----------------
1 file changed, 99 insertions(+), 88 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
index
30069f23660989a4e2b9e5658e7b4724ca141373..74aee4444ca9671bb4177a7e961381fc396f0068
100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -200,6 +200,101 @@ sdap_store_group_with_gid(struct sysdb_ctx *ctx,
return ret;
}
+static errno_t
+sdap_process_ghost_members(struct sysdb_attrs *attrs,
+ struct sdap_options *opts,
+ hash_table_t *ghosts,
+ bool populate_members,
+ struct sysdb_attrs *sysdb_attrs)
+{
+ errno_t ret;
+ struct ldb_message_element *gh;
+ struct ldb_message_element *memberel;
+ struct ldb_message_element *sysdb_memberel;
+ struct ldb_message_element *ghostel;
+ size_t cnt;
+ int i;
+ int hret;
+ hash_key_t key;
+ hash_value_t value;
+
+ ret = sysdb_attrs_get_el(attrs, SYSDB_GHOST, &gh);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error reading ghost attributes: [%s]\n",
+ strerror(ret)));
+ return ret;
+ }
+
+ ret = sysdb_attrs_get_el(attrs,
+ opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name,
+ &memberel);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error reading members: [%s]\n", strerror(ret)));
+ return ret;
+ }
+
+ if (populate_members) {
+ ret = sysdb_attrs_get_el(sysdb_attrs, SYSDB_MEMBER, &sysdb_memberel);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error reading group members from group_attrs: [%s]\n",
+ strerror(ret)));
+ return ret;
+ }
+ sysdb_memberel->values = memberel->values;
+ sysdb_memberel->num_values = memberel->num_values;
+ }
+
+ ret = sysdb_attrs_get_el(sysdb_attrs, SYSDB_GHOST, &ghostel);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error getting ghost element: [%s]\n", strerror(ret)));
+ return ret;
+ }
+ ghostel->values = gh->values;
+ ghostel->num_values = gh->num_values;
+
+ cnt = ghostel->num_values + memberel->num_values;
+ DEBUG(SSSDBG_TRACE_FUNC, ("Group has %d members\n", cnt));
+
+ /* Now process RFC2307bis ghost hash table */
+ if (ghosts && cnt > 0) {
+ ghostel->values = talloc_realloc(sysdb_attrs, ghostel->values,
+ struct ldb_val, cnt);
+ if (ghostel->values == NULL) {
+ return ENOMEM;
+ }
+
+ for (i = 0; i < memberel->num_values; i++) {
+ key.type = HASH_KEY_STRING;
+ key.str = (char *) memberel->values[i].data;
+ hret = hash_lookup(ghosts, &key, &value);
+ if (hret == HASH_ERROR_KEY_NOT_FOUND) {
+ continue;
+ } else if (hret != HASH_SUCCESS) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error checking hash table: [%s]\n",
+ hash_error_string(hret)));
+ return EFAULT;
+ }
+
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("Adding ghost member for group [%s]\n", (char *)
value.ptr));
+ ghostel->values[ghostel->num_values].data = \
+ (uint8_t *) talloc_strdup(ghostel->values, value.ptr);
+ if (ghostel->values[ghostel->num_values].data == NULL) {
+ return ENOMEM;
+ }
+ ghostel->values[ghostel->num_values].length = strlen(value.ptr);
+ ghostel->num_values++;
+ }
+ }
+
+ return EOK;
+}
+
static int sdap_save_group(TALLOC_CTX *memctx,
struct sysdb_ctx *ctx,
struct sdap_options *opts,
@@ -211,20 +306,15 @@ static int sdap_save_group(TALLOC_CTX *memctx,
time_t now)
{
struct ldb_message_element *el;
- struct ldb_message_element *el1;
- struct ldb_message_element *gh;
struct sysdb_attrs *group_attrs;
const char *name = NULL;
gid_t gid;
errno_t ret;
- int hret, cnt, i;
char *usn_value = NULL;
TALLOC_CTX *tmpctx = NULL;
bool posix_group;
bool use_id_mapping = dp_opt_get_bool(opts->basic, SDAP_ID_MAPPING);
char *sid_str;
- hash_key_t key;
- hash_value_t value;
tmpctx = talloc_new(NULL);
if (!tmpctx) {
@@ -245,6 +335,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
DEBUG(1, ("Failed to save the group - entry has no name attribute\n"));
goto fail;
}
+ DEBUG(SSSDBG_TRACE_FUNC, ("Processing group %s\n", name));
if (use_id_mapping) {
posix_group = true;
@@ -383,93 +474,13 @@ static int sdap_save_group(TALLOC_CTX *memctx,
}
}
- ret = sysdb_attrs_get_el(attrs,
opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name,
- &el1);
+ ret = sdap_process_ghost_members(attrs, opts, ghosts,
+ populate_members, group_attrs);
if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error reading group members from attrs: [%s]\n",
- strerror(ret)));
+ DEBUG(SSSDBG_OP_FAILURE, ("Failed to save ghost members\n"));
goto fail;
}
- if (populate_members) {
- ret = sysdb_attrs_get_el(group_attrs, SYSDB_MEMBER, &el);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error reading group members from group_attrs: [%s]\n",
- strerror(ret)));
- goto fail;
- }
- el->values = el1->values;
- el->num_values = el1->num_values;
- }
-
- ret = sysdb_attrs_get_el(attrs, SYSDB_GHOST, &gh);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error reading ghost attributes: [%s]\n",
- strerror(ret)));
- goto fail;
- }
- if (gh->num_values == 0) {
- ret = sysdb_attrs_get_el(attrs,
-
opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name,
- &el1);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error reading members: [%s]\n",
- strerror(ret)));
- goto fail;
- }
- }
-
- ret = sysdb_attrs_get_el(group_attrs, SYSDB_GHOST, &el);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error getting ghost element: [%s]\n",
- strerror(ret)));
- goto fail;
- }
- el->values = gh->values;
- el->num_values = gh->num_values;
-
- cnt = el->num_values + el1->num_values;
- DEBUG(SSSDBG_TRACE_FUNC, ("Group %s has %d members\n", name, cnt));
-
- /* Now process RFC2307bis ghost hash table */
- if (ghosts && cnt > 0) {
- el->values = talloc_realloc(group_attrs, el->values, struct ldb_val,
- cnt);
- if (el->values == NULL) {
- ret = ENOMEM;
- goto fail;
- }
- for (i = 0; i < el1->num_values; i++) {
- key.type = HASH_KEY_STRING;
- key.str = (char *)el1->values[i].data;
- hret = hash_lookup(ghosts, &key, &value);
- if (hret == HASH_ERROR_KEY_NOT_FOUND) {
- continue;
- } else if (hret != HASH_SUCCESS) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Error checking hash table: [%s]\n",
- hash_error_string(hret)));
- ret = EFAULT;
- goto fail;
- }
-
- DEBUG(SSSDBG_TRACE_FUNC, ("Adding ghost member [%s] for group
[%s]\n",
- (char *)value.ptr, name));
- el->values[el->num_values].data = (uint8_t
*)talloc_strdup(el->values, value.ptr);
- if (el->values[el->num_values].data == NULL) {
- ret = ENOMEM;
- goto fail;
- }
- el->values[el->num_values].length = strlen(value.ptr);
- el->num_values++;
- }
- }
-
ret = sdap_save_all_names(name, attrs, !dom->case_sensitive, group_attrs);
if (ret != EOK) {
DEBUG(1, ("Failed to save group names\n"));
--
1.8.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
