[SSSD] [PATCH] Fix saving empty LDAP groups

Jakub Hrozek jhrozek at redhat.com
Thu Nov 15 09:57:37 UTC 2012


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.
-------------- 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 db6b675ca358c1f324b81cdcf3e61a169351dc7f 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..d7e041873c005bb50dd1b6519c9e9c960f76dfbd 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 (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