[SSSD] [PATCH] Fix saving empty LDAP groups

Jakub Hrozek jhrozek at redhat.com
Thu Nov 15 11:55:37 UTC 2012


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.
-------------- next part --------------
>From 86ccbcf7a1ab35b4521ccfe6688f0585108a6ad7 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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 f0185e41c730b0f30acfdb7c22b7bc3a60df6cf5..b461973bc2f73ac43d70e0e2693f122bfdc9f00f 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

-------------- next part --------------
>From 8316eeec49069e07de0a0adb18b06e3bce5505af Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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 b461973bc2f73ac43d70e0e2693f122bfdc9f00f..45832774a3f85ebea1464ba6a86f4d5b39f1e31d 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 (cnt > 0) {
         el->values = talloc_realloc(attrs, el->values, struct ldb_val,
                                     cnt);
         if (el->values == NULL) {
-- 
1.8.0

-------------- next part --------------
>From b42f5e0608d419b4f2223ba05c26f2b452f946d0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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 45832774a3f85ebea1464ba6a86f4d5b39f1e31d..62ea6161f533606e34d4a3c259ba900fe0a13606 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 (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

-------------- next part --------------
>From 55458a9c2c14d797545833a34e72d7250becfd36 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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 62ea6161f533606e34d4a3c259ba900fe0a13606..aea22abe43bdaaa47754d38395de1dfec58eded2 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 (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



More information about the sssd-devel mailing list