Blame SOURCES/0043-Ticket-49495-Fix-memory-management-is-vattr.patch

96373c
From 2c56e7dc08a41fc1dfa6a79213e93686f553847c Mon Sep 17 00:00:00 2001
96373c
From: William Brown <firstyear@redhat.com>
96373c
Date: Mon, 11 Dec 2017 15:48:24 +0100
96373c
Subject: [PATCH] Ticket 49495 - Fix memory management is vattr.
96373c
96373c
Bug Description:  During the fix for
96373c
https://pagure.io/389-ds-base/issue/49436 a issue was exposed
96373c
in how registration of attributes to cos work. With the change
96373c
to handle -> attr link, this exposed that cos treats each attribute
96373c
and template pair as a new type for the handle. As  aresult, this
96373c
caused the sp_list to create a long linked list of M*N entries
96373c
for each attr - template value. Obviously, this is extremely
96373c
slow to traverse during a search!
96373c
96373c
Fix Description:  Undo part of the SLL next change and convert
96373c
to reference counting. The issue remains that there is a defect
96373c
in how cos handles attribute registration, but this can not be
96373c
resolved without a significant rearchitecture of the code
96373c
related to virtual attributes.
96373c
96373c
https://pagure.io/389-ds-base/issue/49495
96373c
96373c
Author: wibrown
96373c
96373c
Review by: tbordaz, lkrispen (Thanks!)
96373c
---
96373c
 ldap/servers/plugins/cos/cos_cache.c | 28 +++++++++++-----------------
96373c
 ldap/servers/slapd/vattr.c           | 23 +++++++++++++++++++++--
96373c
 2 files changed, 32 insertions(+), 19 deletions(-)
96373c
96373c
diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c
96373c
index 662dace35..3b3c05783 100644
96373c
--- a/ldap/servers/plugins/cos/cos_cache.c
96373c
+++ b/ldap/servers/plugins/cos/cos_cache.c
96373c
@@ -275,7 +275,7 @@ static Slapi_Mutex *start_lock;
96373c
 static Slapi_Mutex *stop_lock;
96373c
 static Slapi_CondVar *something_changed = NULL;
96373c
 static Slapi_CondVar *start_cond = NULL;
96373c
-
96373c
+static vattr_sp_handle *vattr_handle = NULL;
96373c
 
96373c
 /*
96373c
     cos_cache_init
96373c
@@ -314,6 +314,15 @@ cos_cache_init(void)
96373c
         goto out;
96373c
     }
96373c
 
96373c
+    if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
96373c
+                                cos_cache_vattr_get,
96373c
+                                cos_cache_vattr_compare,
96373c
+                                cos_cache_vattr_types) != 0) {
96373c
+        slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider\n");
96373c
+        ret = -1;
96373c
+        goto out;
96373c
+    }
96373c
+
96373c
     /* grab the views interface */
96373c
     if (slapi_apib_get_interface(Views_v1_0_GUID, &views_api)) {
96373c
         /* lets be tolerant if views is disabled */
96373c
@@ -847,22 +856,7 @@ cos_dn_defs_cb(Slapi_Entry *e, void *callback_data)
96373c
                                           dnVals[valIndex]->bv_val);
96373c
                 }
96373c
 
96373c
-                /*
96373c
-                 * Each SP_handle is associated to one and only one vattr.
96373c
-                 * We could consider making this a single function rather
96373c
-                 * than the double-call.
96373c
-                 */
96373c
-
96373c
-                vattr_sp_handle *vattr_handle = NULL;
96373c
-
96373c
-                if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
96373c
-                                            cos_cache_vattr_get,
96373c
-                                            cos_cache_vattr_compare,
96373c
-                                            cos_cache_vattr_types) != 0) {
96373c
-                    slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val);
96373c
-                } else {
96373c
-                    slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);
96373c
-                }
96373c
+                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);
96373c
 
96373c
             } /* if(attrType is cosAttribute) */
96373c
 
96373c
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
96373c
index 432946c79..13e527188 100644
96373c
--- a/ldap/servers/slapd/vattr.c
96373c
+++ b/ldap/servers/slapd/vattr.c
96373c
@@ -1544,6 +1544,7 @@ struct _vattr_sp_handle
96373c
     vattr_sp *sp;
96373c
     struct _vattr_sp_handle *next; /* So we can link them together in the map */
96373c
     void *hint;                    /* Hint to the SP */
96373c
+    uint64_t rc;
96373c
 };
96373c
 
96373c
 /* Calls made by Service Providers */
96373c
@@ -1770,7 +1771,7 @@ is a separate thing in the insterests of stability.
96373c
 
96373c
  */
96373c
 
96373c
-#define VARRT_MAP_HASHTABLE_SIZE 10
96373c
+#define VARRT_MAP_HASHTABLE_SIZE 32
96373c
 
96373c
 /* Attribute map oject */
96373c
 /* Needs to contain: a linked list of pointers to provider handles handles,
96373c
@@ -1867,7 +1868,10 @@ vattr_map_entry_free(vattr_map_entry *vae)
96373c
     vattr_sp_handle *list_entry = vae->sp_list;
96373c
     while (list_entry != NULL) {
96373c
         vattr_sp_handle *next_entry = list_entry->next;
96373c
-        slapi_ch_free((void **)&list_entry);
96373c
+        if (slapi_atomic_decr_64(&(list_entry->rc), __ATOMIC_RELAXED) == 0) {
96373c
+            /* Only free on RC 0 */
96373c
+            slapi_ch_free((void **)&list_entry);
96373c
+        }
96373c
         list_entry = next_entry;
96373c
     }
96373c
     slapi_ch_free_string(&(vae->type_name));
96373c
@@ -2280,6 +2284,17 @@ to handle the calls on it, but return nothing */
96373c
  *
96373c
  * Better idea, is that regattr should just take the fn pointers
96373c
  * and callers never *see* the sp_handle structure at all.
96373c
+ *
96373c
+ * This leaves us with some quirks today. First: if you have plugin A
96373c
+ * and B, A registers attr 1 and B 1 and 2, it's possible that if you
96373c
+ * register A1 first, then B1, you have B->A in next. Then when you
96373c
+ * register B2, because we take 0==result from map_lookup, we add sp
96373c
+ * "as is" to the map. This means that B2 now has the same next to A1
96373c
+ * handle. This won't add a bug, because A1 won't be able to service the
96373c
+ * attr, but it could cause some head scratching ...
96373c
+ *
96373c
+ * Again, to fix this, the whole vattr external interface needs a
96373c
+ * redesign ... :(
96373c
  */
96373c
 
96373c
 int
96373c
@@ -2304,11 +2319,15 @@ vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)
96373c
         if (found) {
96373c
             return 0;
96373c
         }
96373c
+        /* Increase the ref count of the sphandle */
96373c
+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);
96373c
         /* We insert the SP handle into the linked list at the head */
96373c
         sp->next = map_entry->sp_list;
96373c
         map_entry->sp_list = sp;
96373c
     } else {
96373c
         /* If not, add it */
96373c
+        /* Claim a reference on the sp ... */
96373c
+        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);
96373c
         map_entry = vattr_map_entry_new(type_to_add, sp, hint);
96373c
         if (NULL == map_entry) {
96373c
             return ENOMEM;
96373c
-- 
96373c
2.13.6
96373c