Blob Blame Raw
From cbe71d7e4901232eaa423b9dc55dba9401c05bec Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Fri, 13 Oct 2017 07:09:08 -0400
Subject: [PATCH] Ticket 48235 - Remove memberOf global lock

Bug Description:  The memberOf global lock no longer servers a purpose since
                  the plugin is BETXN.  This was causing potential deadlocks
                  when multiple backends are used.

Fix Description:  Remove the lock, and rework the fixup/ancestors caches/hashtables.
                  Instead of reusing a single cache, we create a fresh cache
                  when we copy the plugin config (which only happens at the start
                  of an operation).  Then we destroy the caches when we free
                  the config.

https://pagure.io/389-ds-base/issue/48235

Reviewed by: firstyear & tbordaz(Thanks!!)

(cherry picked from commit 184b8a164f4ed456c72d58038aa9a0d512be61fa)
---
 ldap/servers/plugins/memberof/memberof.c        | 326 +++---------------------
 ldap/servers/plugins/memberof/memberof.h        |  17 ++
 ldap/servers/plugins/memberof/memberof_config.c | 166 +++++++++++-
 3 files changed, 210 insertions(+), 299 deletions(-)

diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
index a0f997ddf..a23c52abe 100644
--- a/ldap/servers/plugins/memberof/memberof.c
+++ b/ldap/servers/plugins/memberof/memberof.c
@@ -48,14 +48,11 @@ static Slapi_PluginDesc pdesc = {"memberof", VENDOR,
 static void *_PluginID = NULL;
 static Slapi_DN *_ConfigAreaDN = NULL;
 static Slapi_RWLock *config_rwlock = NULL;
-static Slapi_DN *_pluginDN = NULL;
-static PRMonitor *memberof_operation_lock = 0;
+static Slapi_DN* _pluginDN = NULL;
 MemberOfConfig *qsortConfig = 0;
 static int usetxn = 0;
 static int premodfn = 0;
-#define MEMBEROF_HASHTABLE_SIZE 1000
-static PLHashTable *fixup_entry_hashtable = NULL;     /* global hash table protected by memberof_lock (memberof_operation_lock) */
-static PLHashTable *group_ancestors_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */
+
 
 typedef struct _memberofstringll
 {
@@ -73,18 +70,6 @@ typedef struct _memberof_get_groups_data
     PRBool use_cache;
 } memberof_get_groups_data;
 
-/* The key to access the hash table is the normalized DN
- * The normalized DN is stored in the value because:
- *  - It is used in slapi_valueset_find
- *  - It is used to fill the memberof_get_groups_data.group_norm_vals
- */
-typedef struct _memberof_cached_value
-{
-    char *key;
-    char *group_dn_val;
-    char *group_ndn_val;
-    int valid;
-} memberof_cached_value;
 struct cache_stat
 {
     int total_lookup;
@@ -164,14 +149,9 @@ static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data);
 static int memberof_entry_in_scope(MemberOfConfig *config, Slapi_DN *sdn);
 static int memberof_add_objectclass(char *auto_add_oc, const char *dn);
 static int memberof_add_memberof_attr(LDAPMod **mods, const char *dn, char *add_oc);
-static PLHashTable *hashtable_new();
-static void fixup_hashtable_empty(char *msg);
-static PLHashTable *hashtable_new();
-static void ancestor_hashtable_empty(char *msg);
-static void ancestor_hashtable_entry_free(memberof_cached_value *entry);
-static memberof_cached_value *ancestors_cache_lookup(const char *ndn);
-static PRBool ancestors_cache_remove(const char *ndn);
-static PLHashEntry *ancestors_cache_add(const void *key, void *value);
+static memberof_cached_value *ancestors_cache_lookup(MemberOfConfig *config, const char *ndn);
+static PRBool ancestors_cache_remove(MemberOfConfig *config, const char *ndn);
+static PLHashEntry *ancestors_cache_add(MemberOfConfig *config, const void *key, void *value);
 
 /*** implementation ***/
 
@@ -344,11 +324,6 @@ memberof_postop_start(Slapi_PBlock *pb)
     slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
                   "--> memberof_postop_start\n");
 
-    memberof_operation_lock = PR_NewMonitor();
-    if (0 == memberof_operation_lock) {
-        rc = -1;
-        goto bail;
-    }
     if (config_rwlock == NULL) {
         if ((config_rwlock = slapi_new_rwlock()) == NULL) {
             rc = -1;
@@ -356,9 +331,6 @@ memberof_postop_start(Slapi_PBlock *pb)
         }
     }
 
-    fixup_entry_hashtable = hashtable_new();
-    group_ancestors_hashtable = hashtable_new();
-
     /* Set the alternate config area if one is defined. */
     slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_AREA, &config_area);
     if (config_area) {
@@ -413,13 +385,13 @@ memberof_postop_start(Slapi_PBlock *pb)
         goto bail;
     }
 
-/*
+    /*
      * TODO: start up operation actor thread
      * need to get to a point where server failure
-         * or shutdown doesn't hose our operations
-         * so we should create a task entry that contains
+     * or shutdown doesn't hose our operations
+     * so we should create a task entry that contains
      * all required information to complete the operation
-         * then the tasks can be restarted safely if
+     * then the tasks can be restarted safely if
      * interrupted
      */
 
@@ -451,18 +423,7 @@ memberof_postop_close(Slapi_PBlock *pb __attribute__((unused)))
     slapi_sdn_free(&_pluginDN);
     slapi_destroy_rwlock(config_rwlock);
     config_rwlock = NULL;
-    PR_DestroyMonitor(memberof_operation_lock);
-    memberof_operation_lock = NULL;
-
-    if (fixup_entry_hashtable) {
-        fixup_hashtable_empty("memberof_postop_close empty fixup_entry_hastable");
-        PL_HashTableDestroy(fixup_entry_hashtable);
-    }
 
-    if (group_ancestors_hashtable) {
-        ancestor_hashtable_empty("memberof_postop_close empty group_ancestors_hashtable");
-        PL_HashTableDestroy(group_ancestors_hashtable);
-    }
     slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
                   "<-- memberof_postop_close\n");
     return 0;
@@ -524,7 +485,7 @@ memberof_postop_del(Slapi_PBlock *pb)
 {
     int ret = SLAPI_PLUGIN_SUCCESS;
     MemberOfConfig *mainConfig = NULL;
-    MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+    MemberOfConfig configCopy = {0};
     Slapi_DN *sdn;
     void *caller_id = NULL;
 
@@ -553,9 +514,6 @@ memberof_postop_del(Slapi_PBlock *pb)
         memberof_copy_config(&configCopy, memberof_get_config());
         memberof_unlock_config();
 
-        /* get the memberOf operation lock */
-        memberof_lock();
-
         /* remove this DN from the
          * membership lists of groups
          */
@@ -563,7 +521,6 @@ memberof_postop_del(Slapi_PBlock *pb)
             slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
                           "memberof_postop_del - Error deleting dn (%s) from group. Error (%d)\n",
                           slapi_sdn_get_dn(sdn), ret);
-            memberof_unlock();
             goto bail;
         }
 
@@ -583,7 +540,6 @@ memberof_postop_del(Slapi_PBlock *pb)
                 }
             }
         }
-        memberof_unlock();
     bail:
         memberof_free_config(&configCopy);
     }
@@ -776,7 +732,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
         memberof_cached_value *ht_grp = NULL;
         const char *ndn = slapi_sdn_get_ndn(sdn);
 
-        ht_grp = ancestors_cache_lookup((const void *)ndn);
+        ht_grp = ancestors_cache_lookup(config, (const void *)ndn);
         if (ht_grp) {
 #if MEMBEROF_CACHE_DEBUG
             slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);
@@ -918,7 +874,7 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
 
     if (memberof_oktodo(pb)) {
         MemberOfConfig *mainConfig = 0;
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+        MemberOfConfig configCopy = {0};
         struct slapi_entry *pre_e = NULL;
         struct slapi_entry *post_e = NULL;
         Slapi_DN *pre_sdn = 0;
@@ -944,8 +900,6 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
             goto bail;
         }
 
-        memberof_lock();
-
         /*  update any downstream members */
         if (pre_sdn && post_sdn && configCopy.group_filter &&
             0 == slapi_filter_test_simple(post_e, configCopy.group_filter)) {
@@ -1010,7 +964,6 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
                 }
             }
         }
-        memberof_unlock();
     bail:
         memberof_free_config(&configCopy);
     }
@@ -1166,7 +1119,7 @@ memberof_postop_modify(Slapi_PBlock *pb)
     if (memberof_oktodo(pb)) {
         int config_copied = 0;
         MemberOfConfig *mainConfig = 0;
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+        MemberOfConfig configCopy = {0};
 
         /* get the mod set */
         slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods);
@@ -1209,8 +1162,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
             if (interested) {
                 int op = slapi_mod_get_operation(smod);
 
-                memberof_lock();
-
                 /* the modify op decides the function */
                 switch (op & ~LDAP_MOD_BVALUES) {
                 case LDAP_MOD_ADD: {
@@ -1221,7 +1172,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
                                       "Error (%d)\n",
                                       slapi_sdn_get_dn(sdn), ret);
                         slapi_mod_done(next_mod);
-                        memberof_unlock();
                         goto bail;
                     }
                     break;
@@ -1239,7 +1189,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
                                           "Error (%d)\n",
                                           slapi_sdn_get_dn(sdn), ret);
                             slapi_mod_done(next_mod);
-                            memberof_unlock();
                             goto bail;
                         }
                     } else {
@@ -1250,7 +1199,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
                                           "Error (%d)\n",
                                           slapi_sdn_get_dn(sdn), ret);
                             slapi_mod_done(next_mod);
-                            memberof_unlock();
                             goto bail;
                         }
                     }
@@ -1265,7 +1213,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
                                       "Error (%d)\n",
                                       slapi_sdn_get_dn(sdn), ret);
                         slapi_mod_done(next_mod);
-                        memberof_unlock();
                         goto bail;
                     }
                     break;
@@ -1280,8 +1227,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
                     break;
                 }
                 }
-
-                memberof_unlock();
             }
 
             slapi_mod_done(next_mod);
@@ -1336,7 +1281,7 @@ memberof_postop_add(Slapi_PBlock *pb)
 
     if (memberof_oktodo(pb) && (sdn = memberof_getsdn(pb))) {
         struct slapi_entry *e = NULL;
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+        MemberOfConfig configCopy = {0};
         MemberOfConfig *mainConfig;
         slapi_pblock_get(pb, SLAPI_ENTRY_POST_OP, &e);
 
@@ -1361,8 +1306,6 @@ memberof_postop_add(Slapi_PBlock *pb)
             int i = 0;
             Slapi_Attr *attr = 0;
 
-            memberof_lock();
-
             for (i = 0; configCopy.groupattrs && configCopy.groupattrs[i]; i++) {
                 if (0 == slapi_entry_attr_find(e, configCopy.groupattrs[i], &attr)) {
                     if ((ret = memberof_add_attr_list(pb, &configCopy, sdn, attr))) {
@@ -1373,8 +1316,6 @@ memberof_postop_add(Slapi_PBlock *pb)
                     }
                 }
             }
-
-            memberof_unlock();
             memberof_free_config(&configCopy);
         }
     }
@@ -2094,7 +2035,7 @@ dump_cache_entry(memberof_cached_value *double_check, const char *msg)
  * the firsts elements of the array has 'valid=1' and the dn/ndn of group it belong to
  */
 static void
-cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
+cache_ancestors(MemberOfConfig *config, Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
 {
     Slapi_ValueSet *groupvals = *((memberof_get_groups_data *)groups)->groupvals;
     Slapi_Value *sval;
@@ -2191,14 +2132,14 @@ cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
 #if MEMBEROF_CACHE_DEBUG
     dump_cache_entry(cache_entry, key);
 #endif
-    if (ancestors_cache_add((const void *)key_copy, (void *)cache_entry) == NULL) {
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key);
+    if (ancestors_cache_add(config, (const void*) key_copy, (void *) cache_entry) == NULL) {
+        slapi_log_err( SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key);
         ancestor_hashtable_entry_free(cache_entry);
-        slapi_ch_free((void **)&cache_entry);
+        slapi_ch_free ((void**)&cache_entry);
         return;
     }
 #if MEMBEROF_CACHE_DEBUG
-    if (double_check = ancestors_cache_lookup((const void *)key)) {
+    if (double_check = ancestors_cache_lookup(config, (const void*) key)) {
         dump_cache_entry(double_check, "read back");
     }
 #endif
@@ -2283,8 +2224,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn, memberof_get
 
     merge_ancestors(&member_ndn_val, &member_data, data);
     if (!cached && member_data.use_cache)
-        cache_ancestors(&member_ndn_val, &member_data);
-
+        cache_ancestors(config, &member_ndn_val, &member_data);
 
     slapi_value_free(&member_ndn_val);
     slapi_valueset_free(groupvals);
@@ -2825,49 +2765,10 @@ memberof_qsort_compare(const void *a, const void *b)
                                     val1, val2);
 }
 
-/* betxn: This locking mechanism is necessary to guarantee the memberof
- * consistency */
-void
-memberof_lock()
-{
-    if (usetxn) {
-        PR_EnterMonitor(memberof_operation_lock);
-    }
-    if (fixup_entry_hashtable) {
-        fixup_hashtable_empty("memberof_lock");
-    }
-    if (group_ancestors_hashtable) {
-        ancestor_hashtable_empty("memberof_lock empty group_ancestors_hashtable");
-        memset(&cache_stat, 0, sizeof(cache_stat));
-    }
-}
-
-void
-memberof_unlock()
-{
-    if (group_ancestors_hashtable) {
-        ancestor_hashtable_empty("memberof_unlock empty group_ancestors_hashtable");
-#if MEMBEROF_CACHE_DEBUG
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics: total lookup %d (success %d), add %d, remove %d, enum %d\n",
-                      cache_stat.total_lookup, cache_stat.successfull_lookup,
-                      cache_stat.total_add, cache_stat.total_remove, cache_stat.total_enumerate);
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics duration: lookup %ld, add %ld, remove %ld, enum %ld\n",
-                      cache_stat.cumul_duration_lookup, cache_stat.cumul_duration_add,
-                      cache_stat.cumul_duration_remove, cache_stat.cumul_duration_enumerate);
-#endif
-    }
-    if (fixup_entry_hashtable) {
-        fixup_hashtable_empty("memberof_lock");
-    }
-    if (usetxn) {
-        PR_ExitMonitor(memberof_operation_lock);
-    }
-}
-
 void
 memberof_fixup_task_thread(void *arg)
 {
-    MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+    MemberOfConfig configCopy = {0};
     Slapi_Task *task = (Slapi_Task *)arg;
     task_data *td = NULL;
     int rc = 0;
@@ -2933,9 +2834,6 @@ memberof_fixup_task_thread(void *arg)
     /* do real work */
     rc = memberof_fix_memberof(&configCopy, task, td);
 
-    /* release the memberOf operation lock */
-    memberof_unlock();
-
 done:
     if (usetxn && fixup_pb) {
         if (rc) { /* failed */
@@ -3100,7 +2998,7 @@ memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td)
 }
 
 static memberof_cached_value *
-ancestors_cache_lookup(const char *ndn)
+ancestors_cache_lookup(MemberOfConfig *config, const char *ndn)
 {
     memberof_cached_value *e;
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
@@ -3118,7 +3016,7 @@ ancestors_cache_lookup(const char *ndn)
     }
 #endif
 
-    e = (memberof_cached_value *)PL_HashTableLookupConst(group_ancestors_hashtable, (const void *)ndn);
+    e = (memberof_cached_value *) PL_HashTableLookupConst(config->ancestors_cache, (const void *) ndn);
 
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
     if (start) {
@@ -3133,7 +3031,7 @@ ancestors_cache_lookup(const char *ndn)
     return e;
 }
 static PRBool
-ancestors_cache_remove(const char *ndn)
+ancestors_cache_remove(MemberOfConfig *config, const char *ndn)
 {
     PRBool rc;
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
@@ -3151,7 +3049,8 @@ ancestors_cache_remove(const char *ndn)
     }
 #endif
 
-    rc = PL_HashTableRemove(group_ancestors_hashtable, (const void *)ndn);
+
+    rc = PL_HashTableRemove(config->ancestors_cache, (const void *)ndn);
 
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
     if (start) {
@@ -3164,7 +3063,7 @@ ancestors_cache_remove(const char *ndn)
 }
 
 static PLHashEntry *
-ancestors_cache_add(const void *key, void *value)
+ancestors_cache_add(MemberOfConfig *config, const void *key, void *value)
 {
     PLHashEntry *e;
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
@@ -3181,7 +3080,7 @@ ancestors_cache_add(const void *key, void *value)
     }
 #endif
 
-    e = PL_HashTableAdd(group_ancestors_hashtable, key, value);
+    e = PL_HashTableAdd(config->ancestors_cache, key, value);
 
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
     if (start) {
@@ -3211,7 +3110,6 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
     const char *ndn;
     char *dn_copy;
 
-
     /*
      * If the server is ordered to shutdown, stop the fixup and return an error.
      */
@@ -3222,7 +3120,7 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
 
     /* Check if the entry has not already been fixed */
     ndn = slapi_sdn_get_ndn(sdn);
-    if (ndn && fixup_entry_hashtable && PL_HashTableLookupConst(fixup_entry_hashtable, (void *)ndn)) {
+    if (ndn && config->fixup_cache && PL_HashTableLookupConst(config->fixup_cache, (void *)ndn)) {
         slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn);
         goto bail;
     }
@@ -3240,12 +3138,13 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
              * so free this memory
              */
             ndn = slapi_sdn_get_ndn(sdn);
+
 #if MEMBEROF_CACHE_DEBUG
             slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: This is NOT a group %s\n", ndn);
 #endif
-            ht_grp = ancestors_cache_lookup((const void *)ndn);
+            ht_grp = ancestors_cache_lookup(config, (const void *)ndn);
             if (ht_grp) {
-                if (ancestors_cache_remove((const void *)ndn)) {
+                if (ancestors_cache_remove(config, (const void *)ndn)) {
                     slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: free cached values for %s\n", ndn);
                     ancestor_hashtable_entry_free(ht_grp);
                     slapi_ch_free((void **)&ht_grp);
@@ -3297,11 +3196,11 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
     slapi_valueset_free(groups);
 
     /* records that this entry has been fixed up */
-    if (fixup_entry_hashtable) {
+    if (config->fixup_cache) {
         dn_copy = slapi_ch_strdup(ndn);
-        if (PL_HashTableAdd(fixup_entry_hashtable, dn_copy, dn_copy) == NULL) {
+        if (PL_HashTableAdd(config->fixup_cache, dn_copy, dn_copy) == NULL) {
             slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: "
-                                                                      "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n",
+                          "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n",
                           dn_copy, PR_GetError());
             slapi_ch_free((void **)&dn_copy);
             /* let consider this as not a fatal error, it just skip an optimization */
@@ -3397,157 +3296,8 @@ memberof_add_objectclass(char *auto_add_oc, const char *dn)
     return rc;
 }
 
-static PRIntn
-memberof_hash_compare_keys(const void *v1, const void *v2)
-{
-    PRIntn rc;
-    if (0 == strcasecmp((const char *)v1, (const char *)v2)) {
-        rc = 1;
-    } else {
-        rc = 0;
-    }
-    return rc;
-}
-
-static PRIntn
-memberof_hash_compare_values(const void *v1, const void *v2)
-{
-    PRIntn rc;
-    if ((char *)v1 == (char *)v2) {
-        rc = 1;
-    } else {
-        rc = 0;
-    }
-    return rc;
-}
-
-/*
- *  Hashing function using Bernstein's method
- */
-static PLHashNumber
-memberof_hash_fn(const void *key)
-{
-    PLHashNumber hash = 5381;
-    unsigned char *x = (unsigned char *)key;
-    int c;
-
-    while ((c = *x++)) {
-        hash = ((hash << 5) + hash) ^ c;
-    }
-    return hash;
-}
-
-/* allocates the plugin hashtable
- * This hash table is used by operation and is protected from
- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
- * is not implemented and the hash table will be not used.
- *
- * The hash table contains all the DN of the entries for which the memberof
- * attribute has been computed/updated during the current operation
- *
- * hash table should be empty at the beginning and end of the plugin callback
- */
-static PLHashTable *
-hashtable_new()
-{
-    if (!usetxn) {
-        return NULL;
-    }
-
-    return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE,
-                           memberof_hash_fn,
-                           memberof_hash_compare_keys,
-                           memberof_hash_compare_values, NULL, NULL);
-}
-/* this function called for each hash node during hash destruction */
-static PRIntn
-fixup_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
-{
-    char *dn_copy;
-
-    if (he == NULL) {
-        return HT_ENUMERATE_NEXT;
-    }
-    dn_copy = (char *)he->value;
-    slapi_ch_free_string(&dn_copy);
-
-    return HT_ENUMERATE_REMOVE;
-}
-
-static void
-fixup_hashtable_empty(char *msg)
-{
-    if (fixup_entry_hashtable) {
-        PL_HashTableEnumerateEntries(fixup_entry_hashtable, fixup_hashtable_remove, msg);
-    }
-}
-
-
-/* allocates the plugin hashtable
- * This hash table is used by operation and is protected from
- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
- * is not implemented and the hash table will be not used.
- *
- * The hash table contains all the DN of the entries for which the memberof
- * attribute has been computed/updated during the current operation
- *
- * hash table should be empty at the beginning and end of the plugin callback
- */
-
-static void
-ancestor_hashtable_entry_free(memberof_cached_value *entry)
-{
-    int i;
-    for (i = 0; entry[i].valid; i++) {
-        slapi_ch_free((void **)&entry[i].group_dn_val);
-        slapi_ch_free((void **)&entry[i].group_ndn_val);
-    }
-    /* Here we are at the ending element containing the key */
-    slapi_ch_free((void **)&entry[i].key);
-}
-/* this function called for each hash node during hash destruction */
-static PRIntn
-ancestor_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
-{
-    memberof_cached_value *group_ancestor_array;
-
-    if (he == NULL) {
-        return HT_ENUMERATE_NEXT;
-    }
-
-
-    group_ancestor_array = (memberof_cached_value *)he->value;
-    ancestor_hashtable_entry_free(group_ancestor_array);
-    slapi_ch_free((void **)&group_ancestor_array);
-
-    return HT_ENUMERATE_REMOVE;
-}
-
-static void
-ancestor_hashtable_empty(char *msg)
+int
+memberof_use_txn()
 {
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
-    long int start;
-    struct timespec tsnow;
-#endif
-
-    if (group_ancestors_hashtable) {
-        cache_stat.total_enumerate++;
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
-        if (clock_gettime(CLOCK_REALTIME, &tsnow) != 0) {
-            start = 0;
-        } else {
-            start = tsnow.tv_nsec;
-        }
-#endif
-        PL_HashTableEnumerateEntries(group_ancestors_hashtable, ancestor_hashtable_remove, msg);
-
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
-        if (start) {
-            if (clock_gettime(CLOCK_REALTIME, &tsnow) == 0) {
-                cache_stat.cumul_duration_enumerate += (tsnow.tv_nsec - start);
-            }
-        }
-#endif
-    }
+    return usetxn;
 }
diff --git a/ldap/servers/plugins/memberof/memberof.h b/ldap/servers/plugins/memberof/memberof.h
index 4833ce221..ba64e9dfa 100644
--- a/ldap/servers/plugins/memberof/memberof.h
+++ b/ldap/servers/plugins/memberof/memberof.h
@@ -64,8 +64,22 @@ typedef struct memberofconfig
     int skip_nested;
     int fixup_task;
     char *auto_add_oc;
+    PLHashTable *ancestors_cache;
+    PLHashTable *fixup_cache;
 } MemberOfConfig;
 
+/* The key to access the hash table is the normalized DN
+ * The normalized DN is stored in the value because:
+ *  - It is used in slapi_valueset_find
+ *  - It is used to fill the memberof_get_groups_data.group_norm_vals
+ */
+typedef struct _memberof_cached_value
+{
+    char *key;
+    char *group_dn_val;
+    char *group_ndn_val;
+    int valid;
+} memberof_cached_value;
 
 /*
  * functions
@@ -89,5 +103,8 @@ int memberof_apply_config(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Slapi_Entr
 void *memberof_get_plugin_id(void);
 void memberof_release_config(void);
 PRUint64 get_plugin_started(void);
+void ancestor_hashtable_entry_free(memberof_cached_value *entry);
+PLHashTable *hashtable_new();
+int memberof_use_txn();
 
 #endif /* _MEMBEROF_H_ */
diff --git a/ldap/servers/plugins/memberof/memberof_config.c b/ldap/servers/plugins/memberof/memberof_config.c
index c5ca4b137..3f22d95d6 100644
--- a/ldap/servers/plugins/memberof/memberof_config.c
+++ b/ldap/servers/plugins/memberof/memberof_config.c
@@ -14,12 +14,12 @@
  * memberof_config.c - configuration-related code for memberOf plug-in
  *
  */
-
+#include "plhash.h"
 #include <plstr.h>
-
 #include "memberof.h"
 
 #define MEMBEROF_CONFIG_FILTER "(objectclass=*)"
+#define MEMBEROF_HASHTABLE_SIZE 1000
 
 /*
  * The configuration attributes are contained in the plugin entry e.g.
@@ -34,14 +34,16 @@
 /*
  * function prototypes
  */
-static int memberof_validate_config(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Slapi_Entry *e, int *returncode, char *returntext, void *arg);
-static int
-memberof_search(Slapi_PBlock *pb __attribute__((unused)),
-                Slapi_Entry *entryBefore __attribute__((unused)),
-                Slapi_Entry *e __attribute__((unused)),
-                int *returncode __attribute__((unused)),
-                char *returntext __attribute__((unused)),
-                void *arg __attribute__((unused)))
+static void fixup_hashtable_empty( MemberOfConfig *config, char *msg);
+static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg);
+static int memberof_validate_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, 
+										 int *returncode, char *returntext, void *arg);
+static int memberof_search (Slapi_PBlock *pb __attribute__((unused)),
+                            Slapi_Entry* entryBefore __attribute__((unused)),
+                            Slapi_Entry* e __attribute__((unused)),
+                            int *returncode __attribute__((unused)),
+                            char *returntext __attribute__((unused)),
+                            void *arg __attribute__((unused)))
 {
     return SLAPI_DSE_CALLBACK_OK;
 }
@@ -52,7 +54,7 @@ memberof_search(Slapi_PBlock *pb __attribute__((unused)),
 /* This is the main configuration which is updated from dse.ldif.  The
  * config will be copied when it is used by the plug-in to prevent it
  * being changed out from under a running memberOf operation. */
-static MemberOfConfig theConfig = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+static MemberOfConfig theConfig = {0};
 static Slapi_RWLock *memberof_config_lock = 0;
 static int inited = 0;
 
@@ -693,6 +695,13 @@ void
 memberof_copy_config(MemberOfConfig *dest, MemberOfConfig *src)
 {
     if (dest && src) {
+
+        /* Allocate our caches here since we only copy the config at the start of an op */
+        if (memberof_use_txn() == 1){
+            dest->ancestors_cache = hashtable_new();
+            dest->fixup_cache = hashtable_new();
+        }
+
         /* Check if the copy is already up to date */
         if (src->groupattrs) {
             int i = 0, j = 0;
@@ -787,6 +796,14 @@ memberof_free_config(MemberOfConfig *config)
         slapi_ch_free_string(&config->memberof_attr);
         memberof_free_scope(&(config->entryScopes), &config->entryScopeCount);
         memberof_free_scope(&(config->entryScopeExcludeSubtrees), &config->entryExcludeScopeCount);
+        if (config->fixup_cache) {
+            fixup_hashtable_empty(config, "memberof_free_config empty fixup_entry_hastable");
+            PL_HashTableDestroy(config->fixup_cache);
+        }
+        if (config->ancestors_cache) {
+            ancestor_hashtable_empty(config, "memberof_free_config empty group_ancestors_hashtable");
+            PL_HashTableDestroy(config->ancestors_cache);
+        }
     }
 }
 
@@ -982,3 +999,130 @@ bail:
 
     return ret;
 }
+
+
+static PRIntn memberof_hash_compare_keys(const void *v1, const void *v2)
+{
+    PRIntn rc;
+    if (0 == strcasecmp((const char *) v1, (const char *) v2)) {
+        rc = 1;
+    } else {
+        rc = 0;
+    }
+    return rc;
+}
+
+static PRIntn memberof_hash_compare_values(const void *v1, const void *v2)
+{
+    PRIntn rc;
+    if ((char *) v1 == (char *) v2) {
+        rc = 1;
+    } else {
+        rc = 0;
+    }
+    return rc;
+}
+
+/*
+ *  Hashing function using Bernstein's method
+ */
+static PLHashNumber memberof_hash_fn(const void *key)
+{
+    PLHashNumber hash = 5381;
+    unsigned char *x = (unsigned char *)key;
+    int c;
+
+    while ((c = *x++)){
+        hash = ((hash << 5) + hash) ^ c;
+    }
+    return hash;
+}
+
+/* allocates the plugin hashtable
+ * This hash table is used by operation and is protected from
+ * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
+ * is not implemented and the hash table will be not used.
+ *
+ * The hash table contains all the DN of the entries for which the memberof
+ * attribute has been computed/updated during the current operation
+ *
+ * hash table should be empty at the beginning and end of the plugin callback
+ */
+PLHashTable *hashtable_new(int usetxn)
+{
+    if (!usetxn) {
+        return NULL;
+    }
+
+    return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE,
+        memberof_hash_fn,
+        memberof_hash_compare_keys,
+        memberof_hash_compare_values, NULL, NULL);
+}
+
+/* this function called for each hash node during hash destruction */
+static PRIntn fixup_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
+{
+	char *dn_copy;
+
+	if (he == NULL) {
+		return HT_ENUMERATE_NEXT;
+	}
+	dn_copy = (char*) he->value;
+	slapi_ch_free_string(&dn_copy);
+
+	return HT_ENUMERATE_REMOVE;
+}
+
+static void fixup_hashtable_empty(MemberOfConfig *config, char *msg)
+{
+    if (config->fixup_cache) {
+        PL_HashTableEnumerateEntries(config->fixup_cache, fixup_hashtable_remove, msg);
+    }
+}
+
+
+/* allocates the plugin hashtable
+ * This hash table is used by operation and is protected from
+ * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
+ * is not implemented and the hash table will be not used.
+ *
+ * The hash table contains all the DN of the entries for which the memberof
+ * attribute has been computed/updated during the current operation
+ *
+ * hash table should be empty at the beginning and end of the plugin callback
+ */
+
+void ancestor_hashtable_entry_free(memberof_cached_value *entry)
+{
+    int i;
+
+    for (i = 0; entry[i].valid; i++) {
+        slapi_ch_free((void **) &entry[i].group_dn_val);
+        slapi_ch_free((void **) &entry[i].group_ndn_val);
+    }
+    /* Here we are at the ending element containing the key */
+    slapi_ch_free((void**) &entry[i].key);
+}
+
+/* this function called for each hash node during hash destruction */
+static PRIntn ancestor_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
+{
+    memberof_cached_value *group_ancestor_array;
+
+    if (he == NULL) {
+        return HT_ENUMERATE_NEXT;
+    }
+    group_ancestor_array = (memberof_cached_value *) he->value;
+    ancestor_hashtable_entry_free(group_ancestor_array);
+    slapi_ch_free((void **)&group_ancestor_array);
+
+    return HT_ENUMERATE_REMOVE;
+}
+
+static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg)
+{
+    if (config->ancestors_cache) {
+        PL_HashTableEnumerateEntries(config->ancestors_cache, ancestor_hashtable_remove, msg);
+    }
+}
-- 
2.13.6