8a4a2a
From d1236b0d3917b024a252b4d6acf823a975182d3b Mon Sep 17 00:00:00 2001
8a4a2a
From: William Brown <firstyear@redhat.com>
8a4a2a
Date: Mon, 11 Dec 2017 15:48:24 +0100
8a4a2a
Subject: [PATCH] Ticket 49495 - Fix memory management is vattr.
8a4a2a
8a4a2a
Bug Description:  During the fix for
8a4a2a
https://pagure.io/389-ds-base/issue/49436 a issue was exposed
8a4a2a
in how registration of attributes to cos work. With the change
8a4a2a
to handle -> attr link, this exposed that cos treats each attribute
8a4a2a
and template pair as a new type for the handle. As  aresult, this
8a4a2a
caused the sp_list to create a long linked list of M*N entries
8a4a2a
for each attr - template value. Obviously, this is extremely
8a4a2a
slow to traverse during a search!
8a4a2a
8a4a2a
Fix Description:  Undo part of the SLL next change and convert
8a4a2a
to reference counting. The issue remains that there is a defect
8a4a2a
in how cos handles attribute registration, but this can not be
8a4a2a
resolved without a significant rearchitecture of the code
8a4a2a
related to virtual attributes.
8a4a2a
8a4a2a
https://pagure.io/389-ds-base/issue/49495
8a4a2a
8a4a2a
Author: wibrown
8a4a2a
8a4a2a
Review by: tbordaz, lkrispen (Thanks!)
8a4a2a
8a4a2a
Note: includes backport of incr and decr for rc
8a4a2a
---
8a4a2a
 ldap/servers/plugins/cos/cos_cache.c |  28 ++++------
8a4a2a
 ldap/servers/slapd/slapi-plugin.h    |  21 ++++++++
8a4a2a
 ldap/servers/slapd/slapi_counter.c   |  30 +++++++++++
8a4a2a
 ldap/servers/slapd/vattr.c           | 100 +++++++++++++++++++++--------------
8a4a2a
 4 files changed, 122 insertions(+), 57 deletions(-)
8a4a2a
8a4a2a
diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c
8a4a2a
index 0e93183d2..74261af87 100644
8a4a2a
--- a/ldap/servers/plugins/cos/cos_cache.c
8a4a2a
+++ b/ldap/servers/plugins/cos/cos_cache.c
8a4a2a
@@ -275,7 +275,7 @@ static Slapi_Mutex *start_lock;
8a4a2a
 static Slapi_Mutex *stop_lock;
8a4a2a
 static Slapi_CondVar *something_changed = NULL;
8a4a2a
 static Slapi_CondVar *start_cond = NULL;
8a4a2a
-
8a4a2a
+static vattr_sp_handle *vattr_handle = NULL;
8a4a2a
 
8a4a2a
 /*
8a4a2a
 	cos_cache_init
8a4a2a
@@ -313,6 +313,15 @@ int cos_cache_init(void)
8a4a2a
         goto out;
8a4a2a
     }
8a4a2a
 
8a4a2a
+    if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
8a4a2a
+                                cos_cache_vattr_get,
8a4a2a
+                                cos_cache_vattr_compare,
8a4a2a
+                                cos_cache_vattr_types) != 0) {
8a4a2a
+        slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider\n");
8a4a2a
+        ret = -1;
8a4a2a
+        goto out;
8a4a2a
+    }
8a4a2a
+
8a4a2a
     /* grab the views interface */
8a4a2a
     if (slapi_apib_get_interface(Views_v1_0_GUID, &views_api)) {
8a4a2a
         /* lets be tolerant if views is disabled */
8a4a2a
@@ -872,22 +881,7 @@ cos_dn_defs_cb (Slapi_Entry* e, void *callback_data)
8a4a2a
                                           dnVals[valIndex]->bv_val);
8a4a2a
                 }
8a4a2a
 
8a4a2a
-                /*
8a4a2a
-                 * Each SP_handle is associated to one and only one vattr.
8a4a2a
-                 * We could consider making this a single function rather
8a4a2a
-                 * than the double-call.
8a4a2a
-                 */
8a4a2a
-
8a4a2a
-                vattr_sp_handle *vattr_handle = NULL;
8a4a2a
-
8a4a2a
-                if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
8a4a2a
-                                            cos_cache_vattr_get,
8a4a2a
-                                            cos_cache_vattr_compare,
8a4a2a
-                                            cos_cache_vattr_types) != 0) {
8a4a2a
-                    slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val);
8a4a2a
-                } else {
8a4a2a
-                    slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);
8a4a2a
-                }
8a4a2a
+                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);
8a4a2a
 
8a4a2a
             } /* if(attrType is cosAttribute) */
8a4a2a
 
8a4a2a
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
8a4a2a
index 4084945f4..16aa1b711 100644
8a4a2a
--- a/ldap/servers/slapd/slapi-plugin.h
8a4a2a
+++ b/ldap/servers/slapd/slapi-plugin.h
8a4a2a
@@ -8063,6 +8063,27 @@ int slapi_is_special_rdn(const char *rdn, int flag);
8a4a2a
  */
8a4a2a
 void    DS_Sleep(PRIntervalTime ticks);
8a4a2a
 
8a4a2a
+/**
8a4a2a
+ * Increment a 64bitintegral atomicly
8a4a2a
+ *
8a4a2a
+ * \param ptr - pointer to integral to increment
8a4a2a
+ * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE,
8a4a2a
+ * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST
8a4a2a
+ * \return - new value of ptr
8a4a2a
+ */
8a4a2a
+uint64_t slapi_atomic_incr_64(uint64_t *ptr, int memorder);
8a4a2a
+
8a4a2a
+/**
8a4a2a
+ * Decrement a 64bitintegral atomicly
8a4a2a
+ *
8a4a2a
+ * \param ptr - pointer to integral to decrement
8a4a2a
+ * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE,
8a4a2a
+ * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST
8a4a2a
+ * \return - new value of ptr
8a4a2a
+ */
8a4a2a
+uint64_t slapi_atomic_decr_64(uint64_t *ptr, int memorder);
8a4a2a
+
8a4a2a
+
8a4a2a
 #ifdef __cplusplus
8a4a2a
 }
8a4a2a
 #endif
8a4a2a
diff --git a/ldap/servers/slapd/slapi_counter.c b/ldap/servers/slapd/slapi_counter.c
8a4a2a
index 9904fe964..59e5223ad 100644
8a4a2a
--- a/ldap/servers/slapd/slapi_counter.c
8a4a2a
+++ b/ldap/servers/slapd/slapi_counter.c
8a4a2a
@@ -269,3 +269,33 @@ uint64_t slapi_counter_get_value(Slapi_Counter *counter)
8a4a2a
     return value;
8a4a2a
 }
8a4a2a
 
8a4a2a
+/*
8a4a2a
+ * atomic increment functions (64bit)
8a4a2a
+ */
8a4a2a
+uint64_t
8a4a2a
+slapi_atomic_incr_64(uint64_t *ptr, int memorder)
8a4a2a
+{
8a4a2a
+#ifdef ATOMIC_64BIT_OPERATIONS
8a4a2a
+    return __atomic_add_fetch_8(ptr, 1, memorder);
8a4a2a
+#else
8a4a2a
+    PRInt32 *pr_ptr = (PRInt32 *)ptr;
8a4a2a
+    return PR_AtomicIncrement(pr_ptr);
8a4a2a
+#endif
8a4a2a
+}
8a4a2a
+
8a4a2a
+/*
8a4a2a
+ * atomic decrement functions (64bit)
8a4a2a
+ */
8a4a2a
+
8a4a2a
+uint64_t
8a4a2a
+slapi_atomic_decr_64(uint64_t *ptr, int memorder)
8a4a2a
+{
8a4a2a
+#ifdef ATOMIC_64BIT_OPERATIONS
8a4a2a
+    return __atomic_sub_fetch_8(ptr, 1, memorder);
8a4a2a
+#else
8a4a2a
+    PRInt32 *pr_ptr = (PRInt32 *)ptr;
8a4a2a
+    return PR_AtomicDecrement(pr_ptr);
8a4a2a
+#endif
8a4a2a
+}
8a4a2a
+
8a4a2a
+
8a4a2a
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
8a4a2a
index 84e01cd62..adf44b0b6 100644
8a4a2a
--- a/ldap/servers/slapd/vattr.c
8a4a2a
+++ b/ldap/servers/slapd/vattr.c
8a4a2a
@@ -1529,10 +1529,12 @@ struct _vattr_sp {
8a4a2a
 typedef struct _vattr_sp vattr_sp;
8a4a2a
 
8a4a2a
 /* Service provider handle */
8a4a2a
-struct _vattr_sp_handle {
8a4a2a
-	vattr_sp *sp;
8a4a2a
-	struct _vattr_sp_handle *next; /* So we can link them together in the map */
8a4a2a
-	void *hint; /* Hint to the SP */
8a4a2a
+struct _vattr_sp_handle
8a4a2a
+{
8a4a2a
+    vattr_sp *sp;
8a4a2a
+    struct _vattr_sp_handle *next; /* So we can link them together in the map */
8a4a2a
+    void *hint;                    /* Hint to the SP */
8a4a2a
+    uint64_t rc;
8a4a2a
 };
8a4a2a
 
8a4a2a
 /* Calls made by Service Providers */
8a4a2a
@@ -1758,7 +1760,7 @@ is a separate thing in the insterests of stability.
8a4a2a
 
8a4a2a
  */
8a4a2a
 
8a4a2a
-#define VARRT_MAP_HASHTABLE_SIZE 10
8a4a2a
+#define VARRT_MAP_HASHTABLE_SIZE 32
8a4a2a
 
8a4a2a
 /* Attribute map oject */
8a4a2a
 /* Needs to contain: a linked list of pointers to provider handles handles,
8a4a2a
@@ -1849,7 +1851,10 @@ vattr_map_entry_free(vattr_map_entry *vae)
8a4a2a
     vattr_sp_handle *list_entry = vae->sp_list;
8a4a2a
     while (list_entry != NULL) {
8a4a2a
         vattr_sp_handle *next_entry = list_entry->next;
8a4a2a
-        slapi_ch_free((void **)&list_entry);
8a4a2a
+        if (slapi_atomic_decr_64(&(list_entry->rc), __ATOMIC_RELAXED) == 0) {
8a4a2a
+            /* Only free on RC 0 */
8a4a2a
+            slapi_ch_free((void **)&list_entry);
8a4a2a
+        }
8a4a2a
         list_entry = next_entry;
8a4a2a
     }
8a4a2a
     slapi_ch_free_string(&(vae->type_name));
8a4a2a
@@ -2268,41 +2273,56 @@ to handle the calls on it, but return nothing */
8a4a2a
  *
8a4a2a
  * Better idea, is that regattr should just take the fn pointers
8a4a2a
  * and callers never *see* the sp_handle structure at all.
8a4a2a
+ *
8a4a2a
+ * This leaves us with some quirks today. First: if you have plugin A
8a4a2a
+ * and B, A registers attr 1 and B 1 and 2, it's possible that if you
8a4a2a
+ * register A1 first, then B1, you have B->A in next. Then when you
8a4a2a
+ * register B2, because we take 0==result from map_lookup, we add sp
8a4a2a
+ * "as is" to the map. This means that B2 now has the same next to A1
8a4a2a
+ * handle. This won't add a bug, because A1 won't be able to service the
8a4a2a
+ * attr, but it could cause some head scratching ...
8a4a2a
+ *
8a4a2a
+ * Again, to fix this, the whole vattr external interface needs a
8a4a2a
+ * redesign ... :(
8a4a2a
  */
8a4a2a
-
8a4a2a
-int vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)
8a4a2a
-{
8a4a2a
-	int result = 0;
8a4a2a
-	vattr_map_entry *map_entry = NULL;
8a4a2a
-	/* Is this type already there ? */
8a4a2a
-	result = vattr_map_lookup(type_to_add,&map_entry);
8a4a2a
-	/* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */
8a4a2a
-	if (0 == result) {
8a4a2a
-		int found = 0;
8a4a2a
-		vattr_sp_handle *list_entry = NULL;
8a4a2a
-		/* Walk the list checking that the daft SP isn't already here */
8a4a2a
-		for (list_entry = map_entry->sp_list ; list_entry; list_entry = list_entry->next) {
8a4a2a
-			if (list_entry == sp) {
8a4a2a
-				found = 1;
8a4a2a
-				break;
8a4a2a
-			}
8a4a2a
-		}
8a4a2a
-		/* If it is, we do nothing */
8a4a2a
-		if(found) {
8a4a2a
-			return 0;
8a4a2a
-		}
8a4a2a
-		/* We insert the SP handle into the linked list at the head */
8a4a2a
-		sp->next = map_entry->sp_list;
8a4a2a
-		map_entry->sp_list = sp;
8a4a2a
-	} else {
8a4a2a
-	/* If not, add it */
8a4a2a
-		map_entry = vattr_map_entry_new(type_to_add,sp,hint);
8a4a2a
-		if (NULL == map_entry) {
8a4a2a
-			return ENOMEM;
8a4a2a
-		}
8a4a2a
-		return vattr_map_insert(map_entry);
8a4a2a
-	}
8a4a2a
-	return 0;
8a4a2a
+int
8a4a2a
+vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)
8a4a2a
+{
8a4a2a
+    int result = 0;
8a4a2a
+    vattr_map_entry *map_entry = NULL;
8a4a2a
+    /* Is this type already there ? */
8a4a2a
+    result = vattr_map_lookup(type_to_add, &map_entry);
8a4a2a
+    /* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */
8a4a2a
+    if (0 == result) {
8a4a2a
+        int found = 0;
8a4a2a
+        vattr_sp_handle *list_entry = NULL;
8a4a2a
+        /* Walk the list checking that the daft SP isn't already here */
8a4a2a
+        for (list_entry = map_entry->sp_list; list_entry; list_entry = list_entry->next) {
8a4a2a
+            if (list_entry == sp) {
8a4a2a
+                found = 1;
8a4a2a
+                break;
8a4a2a
+            }
8a4a2a
+        }
8a4a2a
+        /* If it is, we do nothing */
8a4a2a
+        if (found) {
8a4a2a
+            return 0;
8a4a2a
+        }
8a4a2a
+        /* Increase the ref count of the sphandle */
8a4a2a
+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);
8a4a2a
+        /* We insert the SP handle into the linked list at the head */
8a4a2a
+        sp->next = map_entry->sp_list;
8a4a2a
+        map_entry->sp_list = sp;
8a4a2a
+    } else {
8a4a2a
+        /* If not, add it */
8a4a2a
+        /* Claim a reference on the sp ... */
8a4a2a
+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);
8a4a2a
+        map_entry = vattr_map_entry_new(type_to_add, sp, hint);
8a4a2a
+        if (NULL == map_entry) {
8a4a2a
+            return ENOMEM;
8a4a2a
+        }
8a4a2a
+        return vattr_map_insert(map_entry);
8a4a2a
+    }
8a4a2a
+    return 0;
8a4a2a
 }
8a4a2a
 
8a4a2a
 /*
8a4a2a
-- 
8a4a2a
2.13.6
8a4a2a