From ae3da497ac0e753201366ac8644ae5cbdddf2432 Mon Sep 17 00:00:00 2001 From: Thierry Bordaz Date: Wed, 24 Aug 2016 14:22:23 +0200 Subject: [PATCH] Ticket 48861 Memberof plugins can update several times the same entry to set the same values Bug Description: An update on a group triggers memberof computation of all the members of that group. With nested groups, a same entry can appear several times as member of a same group and trigger several memberof computation for that same entry. Fix Description: The post callback function will store in a hash table the DN's of each entry already fixe (regarding membership attributes). If the target entry of the fixup routine (memberof_fix_memberof_callback) is already present in the hash table, that means the entry is already up to date and fixup can be skipped https://fedorahosted.org/389/ticket/48861 Reviewed by: ? Platforms tested: F23 Flag Day: no Doc impact: no --- ldap/servers/plugins/memberof/memberof.c | 118 ++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index aad300f..8aa4132 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/ldap/servers/plugins/memberof/memberof.c @@ -34,9 +34,11 @@ # include #endif +#include #include "slapi-plugin.h" #include "string.h" #include "nspr.h" +#include "plhash.h" #include "memberof.h" static Slapi_PluginDesc pdesc = { "memberof", VENDOR, @@ -47,6 +49,8 @@ static Slapi_DN* _ConfigAreaDN = NULL; static Slapi_RWLock *config_rwlock = NULL; static Slapi_DN* _pluginDN = NULL; static PRMonitor *memberof_operation_lock = 0; +#define MEMBEROF_HASHTABLE_SIZE 10 +static PLHashTable *entry_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */ MemberOfConfig *qsortConfig = 0; static int usetxn = 0; static int premodfn = 0; @@ -147,6 +151,8 @@ 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 hashtable_empty(char *msg); /*** implementation ***/ @@ -337,6 +343,7 @@ int memberof_postop_start(Slapi_PBlock *pb) goto bail; } } + entry_hashtable = hashtable_new(); /* Set the alternate config area if one is defined. */ slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_AREA, &config_area); @@ -431,6 +438,10 @@ int memberof_postop_close(Slapi_PBlock *pb) config_rwlock = NULL; PR_DestroyMonitor(memberof_operation_lock); memberof_operation_lock = NULL; + if (entry_hashtable) { + hashtable_empty("memberof_postop_close"); + PL_HashTableDestroy(entry_hashtable); + } slapi_log_error( SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "<-- memberof_postop_close\n" ); @@ -2610,16 +2621,23 @@ int memberof_qsort_compare(const void *a, const void *b) } /* betxn: This locking mechanism is necessary to guarantee the memberof - * consistency */ + * consistency + * when acquiring/releasing the per operation lock, make sure to empty the hashtable*/ void memberof_lock() { if (usetxn) { PR_EnterMonitor(memberof_operation_lock); } + if (entry_hashtable) { + hashtable_empty("memberof_lock"); + } } void memberof_unlock() { + if (entry_hashtable) { + hashtable_empty("memberof_unlock"); + } if (usetxn) { PR_ExitMonitor(memberof_operation_lock); } @@ -2860,6 +2878,8 @@ int memberof_fix_memberof(MemberOfConfig *config, char *dn, char *filter_str) int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) { int rc = 0; + const char *dn; + char *dn_copy; Slapi_DN *sdn = slapi_entry_get_sdn(e); MemberOfConfig *config = (MemberOfConfig *)callback_data; memberof_del_dn_data del_data = {0, config->memberof_attr}; @@ -2872,6 +2892,14 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) rc = -1; goto bail; } + + /* Check if the entry has not already been fixed */ + dn = slapi_sdn_get_dn(sdn); + if (dn && entry_hashtable && PL_HashTableLookupConst(entry_hashtable, (void*) dn)) { + slapi_log_error(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "Entry %s already fixed up\n", dn); + goto bail; + } + /* get a list of all of the groups this user belongs to */ groups = memberof_get_groups(config, sdn); @@ -2912,6 +2940,21 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) } slapi_valueset_free(groups); + + /* records that this entry has been fixed up */ + if (entry_hashtable) { + dn_copy = slapi_ch_strdup(dn); + if (PL_HashTableAdd(entry_hashtable, dn_copy, dn_copy) == NULL) { + slapi_log_error(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: " + "failed to add dn (%s) in the hashtable; NSPR error - %d\n", + dn_copy, PR_GetError()); + slapi_ch_free((void **) &dn_copy); + rc = -1; + goto bail; + } + } + slapi_log_error(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: " + "add dn (%s) in the hashtable\n", dn_copy); bail: return rc; } @@ -3000,3 +3043,76 @@ 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; + rc = (0 == strcasecmp(v1, v2)) ? 1 : 0; + return rc; +} + +static PRIntn memberof_hash_compare_values(const void *v1, const void *v2) +{ + PRIntn rc; + rc = ( ((char *) v1 == (char *) v2) ? 1 : 0); + return rc; +} + +static PLHashNumber memberof_hash_fn(const void *entrydn) +{ + /* need a hash function here */ + /* Sum the bytes for now */ + PLHashNumber result = 0; + char * current_position = NULL; + char current_char = 0; + for (current_position = (char*) entrydn; *current_position; current_position++) { + current_char = (char) tolower(*current_position); + result += current_char; + } + return result; +} + +/* 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 hashtable_remove(PLHashEntry *he, PRIntn index, void *arg) +{ + char *dn_copy; + char *msg; /* used for debug purpose */ + + if (he == NULL) + return HT_ENUMERATE_NEXT; + + + dn_copy = (char*) he->value; + msg = (char *)arg; + + slapi_ch_free((void **) &dn_copy); + + return HT_ENUMERATE_REMOVE; +} + +static void hashtable_empty(char *msg) +{ + if (entry_hashtable) + PL_HashTableEnumerateEntries(entry_hashtable, hashtable_remove, msg); +} -- 2.5.0