From d1236b0d3917b024a252b4d6acf823a975182d3b Mon Sep 17 00:00:00 2001

From: William Brown <firstyear@redhat.com>

Date: Mon, 11 Dec 2017 15:48:24 +0100

Subject: [PATCH] Ticket 49495 - Fix memory management is vattr.

Bug Description:  During the fix for

https://pagure.io/389-ds-base/issue/49436 a issue was exposed

in how registration of attributes to cos work. With the change

to handle -> attr link, this exposed that cos treats each attribute

and template pair as a new type for the handle. As  aresult, this

caused the sp_list to create a long linked list of M*N entries

for each attr - template value. Obviously, this is extremely

slow to traverse during a search!

Fix Description:  Undo part of the SLL next change and convert

to reference counting. The issue remains that there is a defect

in how cos handles attribute registration, but this can not be

resolved without a significant rearchitecture of the code

related to virtual attributes.

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

Author: wibrown

Review by: tbordaz, lkrispen (Thanks!)

Note: includes backport of incr and decr for rc

---

 ldap/servers/plugins/cos/cos_cache.c |  28 ++++------

 ldap/servers/slapd/slapi-plugin.h    |  21 ++++++++

 ldap/servers/slapd/slapi_counter.c   |  30 +++++++++++

 ldap/servers/slapd/vattr.c           | 100 +++++++++++++++++++++--------------

 4 files changed, 122 insertions(+), 57 deletions(-)

diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c

index 0e93183d2..74261af87 100644

--- a/ldap/servers/plugins/cos/cos_cache.c

+++ b/ldap/servers/plugins/cos/cos_cache.c

@@ -275,7 +275,7 @@ static Slapi_Mutex *start_lock;

 static Slapi_Mutex *stop_lock;

 static Slapi_CondVar *something_changed = NULL;

 static Slapi_CondVar *start_cond = NULL;

-

+static vattr_sp_handle *vattr_handle = NULL;

 /*

 	cos_cache_init

@@ -313,6 +313,15 @@ int cos_cache_init(void)

         goto out;

     }

+    if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,

+                                cos_cache_vattr_get,

+                                cos_cache_vattr_compare,

+                                cos_cache_vattr_types) != 0) {

+        slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider\n");

+        ret = -1;

+        goto out;

+    }

+

     /* grab the views interface */

     if (slapi_apib_get_interface(Views_v1_0_GUID, &views_api)) {

         /* lets be tolerant if views is disabled */

@@ -872,22 +881,7 @@ cos_dn_defs_cb (Slapi_Entry* e, void *callback_data)

                                           dnVals[valIndex]->bv_val);

                 }

-                /*

-                 * Each SP_handle is associated to one and only one vattr.

-                 * We could consider making this a single function rather

-                 * than the double-call.

-                 */

-

-                vattr_sp_handle *vattr_handle = NULL;

-

-                if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,

-                                            cos_cache_vattr_get,

-                                            cos_cache_vattr_compare,

-                                            cos_cache_vattr_types) != 0) {

-                    slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val);

-                } else {

-                    slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);

-                }

+                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);

             } /* if(attrType is cosAttribute) */

diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h

index 4084945f4..16aa1b711 100644

--- a/ldap/servers/slapd/slapi-plugin.h

+++ b/ldap/servers/slapd/slapi-plugin.h

@@ -8063,6 +8063,27 @@ int slapi_is_special_rdn(const char *rdn, int flag);

  */

 void    DS_Sleep(PRIntervalTime ticks);

+/**

+ * Increment a 64bitintegral atomicly

+ *

+ * \param ptr - pointer to integral to increment

+ * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE,

+ * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST

+ * \return - new value of ptr

+ */

+uint64_t slapi_atomic_incr_64(uint64_t *ptr, int memorder);

+

+/**

+ * Decrement a 64bitintegral atomicly

+ *

+ * \param ptr - pointer to integral to decrement

+ * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE,

+ * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST

+ * \return - new value of ptr

+ */

+uint64_t slapi_atomic_decr_64(uint64_t *ptr, int memorder);

+

+

 #ifdef __cplusplus

 }

 #endif

diff --git a/ldap/servers/slapd/slapi_counter.c b/ldap/servers/slapd/slapi_counter.c

index 9904fe964..59e5223ad 100644

--- a/ldap/servers/slapd/slapi_counter.c

+++ b/ldap/servers/slapd/slapi_counter.c

@@ -269,3 +269,33 @@ uint64_t slapi_counter_get_value(Slapi_Counter *counter)

     return value;

 }

+/*

+ * atomic increment functions (64bit)

+ */

+uint64_t

+slapi_atomic_incr_64(uint64_t *ptr, int memorder)

+{

+#ifdef ATOMIC_64BIT_OPERATIONS

+    return __atomic_add_fetch_8(ptr, 1, memorder);

+#else

+    PRInt32 *pr_ptr = (PRInt32 *)ptr;

+    return PR_AtomicIncrement(pr_ptr);

+#endif

+}

+

+/*

+ * atomic decrement functions (64bit)

+ */

+

+uint64_t

+slapi_atomic_decr_64(uint64_t *ptr, int memorder)

+{

+#ifdef ATOMIC_64BIT_OPERATIONS

+    return __atomic_sub_fetch_8(ptr, 1, memorder);

+#else

+    PRInt32 *pr_ptr = (PRInt32 *)ptr;

+    return PR_AtomicDecrement(pr_ptr);

+#endif

+}

+

+

diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c

index 84e01cd62..adf44b0b6 100644

--- a/ldap/servers/slapd/vattr.c

+++ b/ldap/servers/slapd/vattr.c

@@ -1529,10 +1529,12 @@ struct _vattr_sp {

 typedef struct _vattr_sp vattr_sp;

 /* Service provider handle */

-struct _vattr_sp_handle {

-	vattr_sp *sp;

-	struct _vattr_sp_handle *next; /* So we can link them together in the map */

-	void *hint; /* Hint to the SP */

+struct _vattr_sp_handle

+{

+    vattr_sp *sp;

+    struct _vattr_sp_handle *next; /* So we can link them together in the map */

+    void *hint;                    /* Hint to the SP */

+    uint64_t rc;

 };

 /* Calls made by Service Providers */

@@ -1758,7 +1760,7 @@ is a separate thing in the insterests of stability.

  */

-#define VARRT_MAP_HASHTABLE_SIZE 10

+#define VARRT_MAP_HASHTABLE_SIZE 32

 /* Attribute map oject */

 /* Needs to contain: a linked list of pointers to provider handles handles,

@@ -1849,7 +1851,10 @@ vattr_map_entry_free(vattr_map_entry *vae)

     vattr_sp_handle *list_entry = vae->sp_list;

     while (list_entry != NULL) {

         vattr_sp_handle *next_entry = list_entry->next;

-        slapi_ch_free((void **)&list_entry);

+        if (slapi_atomic_decr_64(&(list_entry->rc), __ATOMIC_RELAXED) == 0) {

+            /* Only free on RC 0 */

+            slapi_ch_free((void **)&list_entry);

+        }

         list_entry = next_entry;

     }

     slapi_ch_free_string(&(vae->type_name));

@@ -2268,41 +2273,56 @@ to handle the calls on it, but return nothing */

  *

  * Better idea, is that regattr should just take the fn pointers

  * and callers never *see* the sp_handle structure at all.

+ *

+ * This leaves us with some quirks today. First: if you have plugin A

+ * and B, A registers attr 1 and B 1 and 2, it's possible that if you

+ * register A1 first, then B1, you have B->A in next. Then when you

+ * register B2, because we take 0==result from map_lookup, we add sp

+ * "as is" to the map. This means that B2 now has the same next to A1

+ * handle. This won't add a bug, because A1 won't be able to service the

+ * attr, but it could cause some head scratching ...

+ *

+ * Again, to fix this, the whole vattr external interface needs a

+ * redesign ... :(

  */

-

-int vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)

-{

-	int result = 0;

-	vattr_map_entry *map_entry = NULL;

-	/* Is this type already there ? */

-	result = vattr_map_lookup(type_to_add,&map_entry);

-	/* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */

-	if (0 == result) {

-		int found = 0;

-		vattr_sp_handle *list_entry = NULL;

-		/* Walk the list checking that the daft SP isn't already here */

-		for (list_entry = map_entry->sp_list ; list_entry; list_entry = list_entry->next) {

-			if (list_entry == sp) {

-				found = 1;

-				break;

-			}

-		}

-		/* If it is, we do nothing */

-		if(found) {

-			return 0;

-		}

-		/* We insert the SP handle into the linked list at the head */

-		sp->next = map_entry->sp_list;

-		map_entry->sp_list = sp;

-	} else {

-	/* If not, add it */

-		map_entry = vattr_map_entry_new(type_to_add,sp,hint);

-		if (NULL == map_entry) {

-			return ENOMEM;

-		}

-		return vattr_map_insert(map_entry);

-	}

-	return 0;

+int

+vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)

+{

+    int result = 0;

+    vattr_map_entry *map_entry = NULL;

+    /* Is this type already there ? */

+    result = vattr_map_lookup(type_to_add, &map_entry);

+    /* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */

+    if (0 == result) {

+        int found = 0;

+        vattr_sp_handle *list_entry = NULL;

+        /* Walk the list checking that the daft SP isn't already here */

+        for (list_entry = map_entry->sp_list; list_entry; list_entry = list_entry->next) {

+            if (list_entry == sp) {

+                found = 1;

+                break;

+            }

+        }

+        /* If it is, we do nothing */

+        if (found) {

+            return 0;

+        }

+        /* Increase the ref count of the sphandle */

+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);

+        /* We insert the SP handle into the linked list at the head */

+        sp->next = map_entry->sp_list;

+        map_entry->sp_list = sp;

+    } else {

+        /* If not, add it */

+        /* Claim a reference on the sp ... */

+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);

+        map_entry = vattr_map_entry_new(type_to_add, sp, hint);

+        if (NULL == map_entry) {

+            return ENOMEM;

+        }

+        return vattr_map_insert(map_entry);

+    }

+    return 0;

 }

 /*

-- 

2.13.6