[SSSD] [PATCH] Fix nested groups processing

Jakub Hrozek jhrozek at redhat.com
Thu Mar 8 18:17:37 UTC 2012


The attached patch fixes the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=743133#c28

The cause is actually embarassingly simple, the bug just did occur only
with specific configurations our tests didn't exercise and moreover the
default debugging didn't include enough information to triage the issue
just from the debug logs.

The root cause was that we would process all nested groups in a nesting
level in a single tevent request and we kept the number of parent groups
stored in the "state" structure. However, when moving to next group in
the nesting leve, we didn't reset the number of parents..

Instead of keeping the number of parent groups in "state" and having to
reset the count when moving to another group on the same level, the patch
keeps track of the all groups on a particular level along with their
parents and parent count in a more structured manner. This makes the
code less fragile.

The patch also adds more debugging.
-------------- next part --------------
From 5ffa5dd755e43d07f90f2a8205818aee6d74a449 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 8 Mar 2012 18:24:29 +0100
Subject: [PATCH] Fix nested groups processing

Instead of keeping the number of parent groups in "state" and having to
reset the count when moving to another group on the same level, keep
track of the all groups on a particular level along with their parents
and parent count.
---
 src/providers/ldap/sdap_async_initgroups.c |   86 +++++++++++++++++++--------
 1 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index a4cac9532aba25ba9c415e886ac617e86a1fd38e..3e5064e4f6279d93fa59ce24ff4ca7a597f1ab0e 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -1544,6 +1544,8 @@ static void sdap_initgr_rfc2307bis_process(struct tevent_req *subreq)
         tevent_req_error(req, ret);
         return;
     }
+    DEBUG(SSSDBG_TRACE_LIBS,
+          ("Found %d parent groups for user [%s]\n", count, state->name));
 
     /* Add this batch of groups to the list */
     if (count > 0) {
@@ -1975,8 +1977,7 @@ struct sdap_rfc2307bis_nested_ctx {
     size_t nesting_level;
 
     size_t group_iter;
-    struct sysdb_attrs **ldap_parents;
-    size_t parents_count;
+    struct sdap_nested_group **processed_groups;
 
     hash_table_t *group_hash;
     const char *primary_name;
@@ -1999,6 +2000,9 @@ struct tevent_req *rfc2307bis_nested_groups_send(
     struct tevent_req *req;
     struct sdap_rfc2307bis_nested_ctx *state;
 
+    DEBUG(SSSDBG_TRACE_INTERNAL,
+          ("About to process %d groups in nesting level %d\n", num_groups, nesting));
+
     req = tevent_req_create(mem_ctx, &state,
                             struct sdap_rfc2307bis_nested_ctx);
     if (!req) return NULL;
@@ -2034,6 +2038,14 @@ struct tevent_req *rfc2307bis_nested_groups_send(
         goto done;
     }
 
+    state->processed_groups = talloc_array(state,
+                                           struct sdap_nested_group *,
+                                           state->num_groups);
+    if (state->processed_groups == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+
     ret = rfc2307bis_nested_groups_step(req);
 
 done:
@@ -2058,6 +2070,7 @@ static errno_t rfc2307bis_nested_groups_step(struct tevent_req *req)
     TALLOC_CTX *tmp_ctx = NULL;
     char *clean_orig_dn;
     hash_key_t key;
+    hash_value_t value;
     struct sdap_rfc2307bis_nested_ctx *state =
             tevent_req_data(req, struct sdap_rfc2307bis_nested_ctx);
 
@@ -2083,14 +2096,33 @@ static errno_t rfc2307bis_nested_groups_step(struct tevent_req *req)
         goto done;
     }
 
-    DEBUG(6, ("Processing group [%s]\n", state->primary_name));
+    DEBUG(SSSDBG_TRACE_LIBS, ("Processing group [%s]\n", state->primary_name));
 
-    if (hash_has_key(state->group_hash, &key)) {
+    ret = hash_lookup(state->group_hash, &key, &value);
+    if (ret == HASH_SUCCESS) {
+        DEBUG(SSSDBG_TRACE_INTERNAL, ("Group [%s] was already processed, "
+              "taking a shortcut\n", state->primary_name));
+        state->processed_groups[state->group_iter] = \
+            talloc_get_type(value.ptr, struct sdap_nested_group);
         talloc_free(key.str);
         ret = EOK;
         goto done;
     }
 
+    /* Need to try to find parent groups for this group. */
+    state->processed_groups[state->group_iter] = talloc_zero(state->processed_groups,
+                                                             struct sdap_nested_group);
+    if (!state->processed_groups[state->group_iter]) {
+        ret = ENOMEM;
+        goto done;
+    }
+
+    /* this steal doesn't change much now, but will be helpful later on
+     * if we steal the whole processed_group on the hash table */
+    state->processed_groups[state->group_iter]->group =
+        talloc_steal(state->processed_groups[state->group_iter],
+                     state->groups[state->group_iter]);
+
     /* Get any parent groups for this group */
     ret = sysdb_attrs_get_string(state->groups[state->group_iter],
                                  SYSDB_ORIG_DN,
@@ -2196,14 +2228,18 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
         return;
     }
 
+    DEBUG(SSSDBG_TRACE_LIBS,
+          ("Found %d parent groups of [%s]\n", count, state->orig_dn));
+    ngr = state->processed_groups[state->group_iter];
+
     /* Add this batch of groups to the list */
     if (count > 0) {
-        state->ldap_parents =
-                talloc_realloc(state,
-                               state->ldap_parents,
+        ngr->ldap_parents =
+                talloc_realloc(ngr,
+                               ngr->ldap_parents,
                                struct sysdb_attrs *,
-                               state->parents_count + count + 1);
-        if (!state->ldap_parents) {
+                               ngr->parents_count + count + 1);
+        if (!ngr->ldap_parents) {
             tevent_req_error(req, ENOMEM);
             return;
         }
@@ -2214,13 +2250,16 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
          * we finish this nesting level.
          */
         for (i = 0; i < count; i++) {
-            state->ldap_parents[state->parents_count + i] =
-                talloc_steal(state->ldap_parents, ldap_groups[i]);
+            ngr->ldap_parents[ngr->parents_count + i] =
+                talloc_steal(ngr->ldap_parents, ldap_groups[i]);
         }
 
-        state->parents_count += count;
+        ngr->parents_count += count;
 
-        state->ldap_parents[state->parents_count] = NULL;
+        ngr->ldap_parents[ngr->parents_count] = NULL;
+        DEBUG(SSSDBG_TRACE_INTERNAL,
+              ("Total of %d direct parents after this iteration\n",
+               ngr->parents_count));
     }
 
     state->base_iter++;
@@ -2239,16 +2278,7 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
     /* Reset the base iterator for future lookups */
     state->base_iter = 0;
 
-    ngr = talloc_zero(state->group_hash, struct sdap_nested_group);
-    if (!ngr) {
-        tevent_req_error(req, ENOMEM);
-        return;
-    }
-
-    ngr->group = talloc_steal(ngr, state->groups[state->group_iter]);
-    ngr->ldap_parents = talloc_steal(ngr, state->ldap_parents);
-    ngr->parents_count = state->parents_count;
-
+    /* Save the group into the hash table */
     key.type = HASH_KEY_STRING;
     key.str = talloc_strdup(state, state->primary_name);
     if (!key.str) {
@@ -2256,6 +2286,10 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
         return;
     }
 
+    /* Steal the nested group entry on the group_hash context so it can
+     * outlive this request */
+    talloc_steal(state->group_hash, ngr);
+
     value.type = HASH_VALUE_PTR;
     value.ptr = ngr;
 
@@ -2267,7 +2301,7 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
     }
     talloc_free(key.str);
 
-    if (state->parents_count == 0) {
+    if (ngr->parents_count == 0) {
         /* No parent groups for this group in LDAP
          * Move on to the next group
          */
@@ -2298,8 +2332,8 @@ static void rfc2307bis_nested_groups_process(struct tevent_req *subreq)
     subreq = rfc2307bis_nested_groups_send(
             state, state->ev, state->opts, state->sysdb,
             state->dom, state->sh,
-            state->ldap_parents,
-            state->parents_count,
+            ngr->ldap_parents,
+            ngr->parents_count,
             state->group_hash,
             state->nesting_level+1);
     if (!subreq) {
-- 
1.7.7.6



More information about the sssd-devel mailing list