Blob Blame Raw
From dcf75750dff23e848cde2ae63a0778b123de6dd7 Mon Sep 17 00:00:00 2001
From: William Brown <firstyear@redhat.com>
Date: Thu, 2 Nov 2017 13:32:41 +1000
Subject: [PATCH] Ticket 49436 - double free in COS in some conditions

Bug Description:  virtualattrs and COS have some serious memory
ownership issues. What was happening is that COS with multiple
attributes using the same sp_handle would cause a structure
to be registered twice. During shutdown we would then trigger
a double free in the process.

Fix Description:  Change the behaviour of sp_handles to use a
handle *per* attribute we register to guarantee the assocation
between them.

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

Author: wibrown

Review by: mreynolds, vashirov (Thanks!)

(cherry pick from commit ee4428a3f5d2d8e37a7107c7dce9d622fc17d41c)
---
 dirsrvtests/tests/suites/cos/indirect_cos_test.py | 43 +++++++----------------
 ldap/servers/plugins/cos/cos_cache.c              | 32 +++++++++--------
 ldap/servers/plugins/roles/roles_cache.c          |  8 ++---
 ldap/servers/slapd/vattr.c                        | 28 +++++++++------
 4 files changed, 51 insertions(+), 60 deletions(-)

diff --git a/dirsrvtests/tests/suites/cos/indirect_cos_test.py b/dirsrvtests/tests/suites/cos/indirect_cos_test.py
index 1aac6b8ed..452edcdf8 100644
--- a/dirsrvtests/tests/suites/cos/indirect_cos_test.py
+++ b/dirsrvtests/tests/suites/cos/indirect_cos_test.py
@@ -7,6 +7,7 @@ import subprocess
 
 from lib389 import Entry
 from lib389.idm.user import UserAccounts
+from lib389.idm.domain import Domain
 from lib389.topologies import topology_st as topo
 from lib389._constants import (DEFAULT_SUFFIX, DN_DM, PASSWORD, HOST_STANDALONE,
                                SERVERID_STANDALONE, PORT_STANDALONE)
@@ -48,14 +49,8 @@ def check_user(inst):
 def setup_subtree_policy(topo):
     """Set up subtree password policy
     """
-    try:
-        topo.standalone.modify_s("cn=config", [(ldap.MOD_REPLACE,
-                                                'nsslapd-pwpolicy-local',
-                                                'on')])
-    except ldap.LDAPError as e:
-        log.error('Failed to set fine-grained policy: error {}'.format(
-            e.message['desc']))
-        raise e
+
+    topo.standalone.config.set('nsslapd-pwpolicy-local', 'on')
 
     log.info('Create password policy for subtree {}'.format(OU_PEOPLE))
     try:
@@ -68,15 +63,9 @@ def setup_subtree_policy(topo):
             OU_PEOPLE, e.message['desc']))
         raise e
 
-    log.info('Add pwdpolicysubentry attribute to {}'.format(OU_PEOPLE))
-    try:
-        topo.standalone.modify_s(DEFAULT_SUFFIX, [(ldap.MOD_REPLACE,
-                                                   'pwdpolicysubentry',
-                                                   PW_POLICY_CONT_PEOPLE2)])
-    except ldap.LDAPError as e:
-        log.error('Failed to pwdpolicysubentry pw policy '
-                  'policy for {}: error {}'.format(OU_PEOPLE, e.message['desc']))
-        raise e
+    domain = Domain(topo.standalone, DEFAULT_SUFFIX)
+    domain.replace('pwdpolicysubentry', PW_POLICY_CONT_PEOPLE2)
+
     time.sleep(1)
 
 
@@ -116,12 +105,9 @@ def setup(topo, request):
     """
     log.info('Add custom schema...')
     try:
-        ATTR_1 = ("( 1.3.6.1.4.1.409.389.2.189 NAME 'x-department' " +
-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
-        ATTR_2 = ("( 1.3.6.1.4.1.409.389.2.187 NAME 'x-en-ou' " +
-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
-        OC = ("( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY " +
-              "( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")
+        ATTR_1 = (b"( 1.3.6.1.4.1.409.389.2.189 NAME 'x-department' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
+        ATTR_2 = (b"( 1.3.6.1.4.1.409.389.2.187 NAME 'x-en-ou' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
+        OC = (b"( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY ( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")
         topo.standalone.modify_s("cn=schema", [(ldap.MOD_ADD, 'attributeTypes', ATTR_1),
                                                (ldap.MOD_ADD, 'attributeTypes', ATTR_2),
                                                (ldap.MOD_ADD, 'objectClasses', OC)])
@@ -142,14 +128,9 @@ def setup(topo, request):
         'homeDirectory': '/home/test_user',
         'seeAlso': 'cn=cosTemplate,dc=example,dc=com'
     }
-    users.create(properties=user_properties)
-    try:
-        topo.standalone.modify_s(TEST_USER_DN, [(ldap.MOD_ADD,
-                                                 'objectclass',
-                                                 'xPerson')])
-    except ldap.LDAPError as e:
-        log.fatal('Failed to add objectclass to user')
-        raise e
+    user = users.create(properties=user_properties)
+
+    user.add('objectClass', 'xPerson')
 
     # Setup COS
     log.info("Setup indirect COS...")
diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c
index 9ae15db15..662dace35 100644
--- a/ldap/servers/plugins/cos/cos_cache.c
+++ b/ldap/servers/plugins/cos/cos_cache.c
@@ -109,9 +109,6 @@ void *cos_get_plugin_identity(void);
 #define COSTYPE_INDIRECT 3
 #define COS_DEF_ERROR_NO_TEMPLATES -2
 
-/* the global plugin handle */
-static volatile vattr_sp_handle *vattr_handle = NULL;
-
 /* both variables are protected by change_lock */
 static int cos_cache_notify_flag = 0;
 static PRBool cos_cache_at_work = PR_FALSE;
@@ -323,16 +320,6 @@ cos_cache_init(void)
         views_api = 0;
     }
 
-    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;
-    }
-
     if (PR_CreateThread(PR_USER_THREAD,
                         cos_cache_wait_on_change,
                         NULL,
@@ -860,8 +847,23 @@ cos_dn_defs_cb(Slapi_Entry *e, void *callback_data)
                                           dnVals[valIndex]->bv_val);
                 }
 
-                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle,
-                                       dnVals[valIndex]->bv_val, NULL, NULL);
+                /*
+                 * 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);
+                }
+
             } /* if(attrType is cosAttribute) */
 
             /*
diff --git a/ldap/servers/plugins/roles/roles_cache.c b/ldap/servers/plugins/roles/roles_cache.c
index 59f5a6081..1e5865af8 100644
--- a/ldap/servers/plugins/roles/roles_cache.c
+++ b/ldap/servers/plugins/roles/roles_cache.c
@@ -47,9 +47,6 @@ static char *allUserAttributes[] = {
 /* views scoping */
 static void **views_api;
 
-/* Service provider handler */
-static vattr_sp_handle *vattr_handle = NULL;
-
 /* List of nested roles */
 typedef struct _role_object_nested
 {
@@ -224,6 +221,10 @@ roles_cache_init()
       so that we update the corresponding cache */
     slapi_register_backend_state_change(NULL, roles_cache_trigger_update_suffix);
 
+    /* Service provider handler - only used once! and freed by vattr! */
+    vattr_sp_handle *vattr_handle = NULL;
+
+
     if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
                                 roles_sp_get_value,
                                 roles_sp_compare_value,
@@ -622,7 +623,6 @@ roles_cache_stop()
         current_role = next_role;
     }
     slapi_rwlock_unlock(global_lock);
-    slapi_ch_free((void **)&vattr_handle);
     roles_list = NULL;
 
     slapi_log_err(SLAPI_LOG_PLUGIN, ROLES_PLUGIN_SUBSYSTEM, "<-- roles_cache_stop\n");
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
index 82deb41fe..432946c79 100644
--- a/ldap/servers/slapd/vattr.c
+++ b/ldap/servers/slapd/vattr.c
@@ -1864,7 +1864,12 @@ vattr_map_create(void)
 void
 vattr_map_entry_free(vattr_map_entry *vae)
 {
-    slapi_ch_free((void **)&(vae->sp_list));
+    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);
+        list_entry = next_entry;
+    }
     slapi_ch_free_string(&(vae->type_name));
     slapi_ch_free((void **)&vae);
 }
@@ -2143,16 +2148,9 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)
 vattr_map_entry *
 vattr_map_entry_new(char *type_name, vattr_sp_handle *sph, void *hint)
 {
-    vattr_map_entry *result = NULL;
-    vattr_sp_handle *sp_copy = NULL;
-
-    sp_copy = (vattr_sp_handle *)slapi_ch_calloc(1, sizeof(vattr_sp_handle));
-    sp_copy->sp = sph->sp;
-    sp_copy->hint = hint;
-
-    result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));
+    vattr_map_entry *result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));
     result->type_name = slapi_ch_strdup(type_name);
-    result->sp_list = sp_copy;
+    result->sp_list = sph;
 
     /* go get schema */
     result->objectclasses = vattr_map_entry_build_schema(type_name);
@@ -2273,6 +2271,16 @@ we'd need to hold a lock on the read path, which we don't want to do.
 So any SP which relinquishes its need to handle a type needs to continue
 to handle the calls on it, but return nothing */
 /* DBDB need to sort out memory ownership here, it's not quite right */
+/*
+ * This function was inconsistent. We would allocated and "kind of",
+ * copy the sp_handle here for the vattr_map_entry_new path. But we
+ * would "take ownership" for the existing entry and the list addition
+ * path. Instead now, EVERY sp_handle we take, we take ownership of
+ * and the CALLER must allocate a new one each time.
+ *
+ * Better idea, is that regattr should just take the fn pointers
+ * and callers never *see* the sp_handle structure at all.
+ */
 
 int
 vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)
-- 
2.13.6