Blame SOURCES/0024-Ticket-49436-double-free-in-COS-in-some-conditions.patch

b045b9
From dcf75750dff23e848cde2ae63a0778b123de6dd7 Mon Sep 17 00:00:00 2001
b045b9
From: William Brown <firstyear@redhat.com>
b045b9
Date: Thu, 2 Nov 2017 13:32:41 +1000
b045b9
Subject: [PATCH] Ticket 49436 - double free in COS in some conditions
b045b9
b045b9
Bug Description:  virtualattrs and COS have some serious memory
b045b9
ownership issues. What was happening is that COS with multiple
b045b9
attributes using the same sp_handle would cause a structure
b045b9
to be registered twice. During shutdown we would then trigger
b045b9
a double free in the process.
b045b9
b045b9
Fix Description:  Change the behaviour of sp_handles to use a
b045b9
handle *per* attribute we register to guarantee the assocation
b045b9
between them.
b045b9
b045b9
https://pagure.io/389-ds-base/issue/49436
b045b9
b045b9
Author: wibrown
b045b9
b045b9
Review by: mreynolds, vashirov (Thanks!)
b045b9
b045b9
(cherry pick from commit ee4428a3f5d2d8e37a7107c7dce9d622fc17d41c)
b045b9
---
b045b9
 dirsrvtests/tests/suites/cos/indirect_cos_test.py | 43 +++++++----------------
b045b9
 ldap/servers/plugins/cos/cos_cache.c              | 32 +++++++++--------
b045b9
 ldap/servers/plugins/roles/roles_cache.c          |  8 ++---
b045b9
 ldap/servers/slapd/vattr.c                        | 28 +++++++++------
b045b9
 4 files changed, 51 insertions(+), 60 deletions(-)
b045b9
b045b9
diff --git a/dirsrvtests/tests/suites/cos/indirect_cos_test.py b/dirsrvtests/tests/suites/cos/indirect_cos_test.py
b045b9
index 1aac6b8ed..452edcdf8 100644
b045b9
--- a/dirsrvtests/tests/suites/cos/indirect_cos_test.py
b045b9
+++ b/dirsrvtests/tests/suites/cos/indirect_cos_test.py
b045b9
@@ -7,6 +7,7 @@ import subprocess
b045b9
 
b045b9
 from lib389 import Entry
b045b9
 from lib389.idm.user import UserAccounts
b045b9
+from lib389.idm.domain import Domain
b045b9
 from lib389.topologies import topology_st as topo
b045b9
 from lib389._constants import (DEFAULT_SUFFIX, DN_DM, PASSWORD, HOST_STANDALONE,
b045b9
                                SERVERID_STANDALONE, PORT_STANDALONE)
b045b9
@@ -48,14 +49,8 @@ def check_user(inst):
b045b9
 def setup_subtree_policy(topo):
b045b9
     """Set up subtree password policy
b045b9
     """
b045b9
-    try:
b045b9
-        topo.standalone.modify_s("cn=config", [(ldap.MOD_REPLACE,
b045b9
-                                                'nsslapd-pwpolicy-local',
b045b9
-                                                'on')])
b045b9
-    except ldap.LDAPError as e:
b045b9
-        log.error('Failed to set fine-grained policy: error {}'.format(
b045b9
-            e.message['desc']))
b045b9
-        raise e
b045b9
+
b045b9
+    topo.standalone.config.set('nsslapd-pwpolicy-local', 'on')
b045b9
 
b045b9
     log.info('Create password policy for subtree {}'.format(OU_PEOPLE))
b045b9
     try:
b045b9
@@ -68,15 +63,9 @@ def setup_subtree_policy(topo):
b045b9
             OU_PEOPLE, e.message['desc']))
b045b9
         raise e
b045b9
 
b045b9
-    log.info('Add pwdpolicysubentry attribute to {}'.format(OU_PEOPLE))
b045b9
-    try:
b045b9
-        topo.standalone.modify_s(DEFAULT_SUFFIX, [(ldap.MOD_REPLACE,
b045b9
-                                                   'pwdpolicysubentry',
b045b9
-                                                   PW_POLICY_CONT_PEOPLE2)])
b045b9
-    except ldap.LDAPError as e:
b045b9
-        log.error('Failed to pwdpolicysubentry pw policy '
b045b9
-                  'policy for {}: error {}'.format(OU_PEOPLE, e.message['desc']))
b045b9
-        raise e
b045b9
+    domain = Domain(topo.standalone, DEFAULT_SUFFIX)
b045b9
+    domain.replace('pwdpolicysubentry', PW_POLICY_CONT_PEOPLE2)
b045b9
+
b045b9
     time.sleep(1)
b045b9
 
b045b9
 
b045b9
@@ -116,12 +105,9 @@ def setup(topo, request):
b045b9
     """
b045b9
     log.info('Add custom schema...')
b045b9
     try:
b045b9
-        ATTR_1 = ("( 1.3.6.1.4.1.409.389.2.189 NAME 'x-department' " +
b045b9
-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
b045b9
-        ATTR_2 = ("( 1.3.6.1.4.1.409.389.2.187 NAME 'x-en-ou' " +
b045b9
-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")
b045b9
-        OC = ("( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY " +
b045b9
-              "( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")
b045b9
+        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' )")
b045b9
+        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' )")
b045b9
+        OC = (b"( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY ( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")
b045b9
         topo.standalone.modify_s("cn=schema", [(ldap.MOD_ADD, 'attributeTypes', ATTR_1),
b045b9
                                                (ldap.MOD_ADD, 'attributeTypes', ATTR_2),
b045b9
                                                (ldap.MOD_ADD, 'objectClasses', OC)])
b045b9
@@ -142,14 +128,9 @@ def setup(topo, request):
b045b9
         'homeDirectory': '/home/test_user',
b045b9
         'seeAlso': 'cn=cosTemplate,dc=example,dc=com'
b045b9
     }
b045b9
-    users.create(properties=user_properties)
b045b9
-    try:
b045b9
-        topo.standalone.modify_s(TEST_USER_DN, [(ldap.MOD_ADD,
b045b9
-                                                 'objectclass',
b045b9
-                                                 'xPerson')])
b045b9
-    except ldap.LDAPError as e:
b045b9
-        log.fatal('Failed to add objectclass to user')
b045b9
-        raise e
b045b9
+    user = users.create(properties=user_properties)
b045b9
+
b045b9
+    user.add('objectClass', 'xPerson')
b045b9
 
b045b9
     # Setup COS
b045b9
     log.info("Setup indirect COS...")
b045b9
diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c
b045b9
index 9ae15db15..662dace35 100644
b045b9
--- a/ldap/servers/plugins/cos/cos_cache.c
b045b9
+++ b/ldap/servers/plugins/cos/cos_cache.c
b045b9
@@ -109,9 +109,6 @@ void *cos_get_plugin_identity(void);
b045b9
 #define COSTYPE_INDIRECT 3
b045b9
 #define COS_DEF_ERROR_NO_TEMPLATES -2
b045b9
 
b045b9
-/* the global plugin handle */
b045b9
-static volatile vattr_sp_handle *vattr_handle = NULL;
b045b9
-
b045b9
 /* both variables are protected by change_lock */
b045b9
 static int cos_cache_notify_flag = 0;
b045b9
 static PRBool cos_cache_at_work = PR_FALSE;
b045b9
@@ -323,16 +320,6 @@ cos_cache_init(void)
b045b9
         views_api = 0;
b045b9
     }
b045b9
 
b045b9
-    if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
b045b9
-                                cos_cache_vattr_get,
b045b9
-                                cos_cache_vattr_compare,
b045b9
-                                cos_cache_vattr_types) != 0) {
b045b9
-        slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM,
b045b9
-                      "cos_cache_init - Cannot register as service provider\n");
b045b9
-        ret = -1;
b045b9
-        goto out;
b045b9
-    }
b045b9
-
b045b9
     if (PR_CreateThread(PR_USER_THREAD,
b045b9
                         cos_cache_wait_on_change,
b045b9
                         NULL,
b045b9
@@ -860,8 +847,23 @@ cos_dn_defs_cb(Slapi_Entry *e, void *callback_data)
b045b9
                                           dnVals[valIndex]->bv_val);
b045b9
                 }
b045b9
 
b045b9
-                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle,
b045b9
-                                       dnVals[valIndex]->bv_val, NULL, NULL);
b045b9
+                /*
b045b9
+                 * Each SP_handle is associated to one and only one vattr.
b045b9
+                 * We could consider making this a single function rather
b045b9
+                 * than the double-call.
b045b9
+                 */
b045b9
+
b045b9
+                vattr_sp_handle *vattr_handle = NULL;
b045b9
+
b045b9
+                if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
b045b9
+                                            cos_cache_vattr_get,
b045b9
+                                            cos_cache_vattr_compare,
b045b9
+                                            cos_cache_vattr_types) != 0) {
b045b9
+                    slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val);
b045b9
+                } else {
b045b9
+                    slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);
b045b9
+                }
b045b9
+
b045b9
             } /* if(attrType is cosAttribute) */
b045b9
 
b045b9
             /*
b045b9
diff --git a/ldap/servers/plugins/roles/roles_cache.c b/ldap/servers/plugins/roles/roles_cache.c
b045b9
index 59f5a6081..1e5865af8 100644
b045b9
--- a/ldap/servers/plugins/roles/roles_cache.c
b045b9
+++ b/ldap/servers/plugins/roles/roles_cache.c
b045b9
@@ -47,9 +47,6 @@ static char *allUserAttributes[] = {
b045b9
 /* views scoping */
b045b9
 static void **views_api;
b045b9
 
b045b9
-/* Service provider handler */
b045b9
-static vattr_sp_handle *vattr_handle = NULL;
b045b9
-
b045b9
 /* List of nested roles */
b045b9
 typedef struct _role_object_nested
b045b9
 {
b045b9
@@ -224,6 +221,10 @@ roles_cache_init()
b045b9
       so that we update the corresponding cache */
b045b9
     slapi_register_backend_state_change(NULL, roles_cache_trigger_update_suffix);
b045b9
 
b045b9
+    /* Service provider handler - only used once! and freed by vattr! */
b045b9
+    vattr_sp_handle *vattr_handle = NULL;
b045b9
+
b045b9
+
b045b9
     if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,
b045b9
                                 roles_sp_get_value,
b045b9
                                 roles_sp_compare_value,
b045b9
@@ -622,7 +623,6 @@ roles_cache_stop()
b045b9
         current_role = next_role;
b045b9
     }
b045b9
     slapi_rwlock_unlock(global_lock);
b045b9
-    slapi_ch_free((void **)&vattr_handle);
b045b9
     roles_list = NULL;
b045b9
 
b045b9
     slapi_log_err(SLAPI_LOG_PLUGIN, ROLES_PLUGIN_SUBSYSTEM, "<-- roles_cache_stop\n");
b045b9
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
b045b9
index 82deb41fe..432946c79 100644
b045b9
--- a/ldap/servers/slapd/vattr.c
b045b9
+++ b/ldap/servers/slapd/vattr.c
b045b9
@@ -1864,7 +1864,12 @@ vattr_map_create(void)
b045b9
 void
b045b9
 vattr_map_entry_free(vattr_map_entry *vae)
b045b9
 {
b045b9
-    slapi_ch_free((void **)&(vae->sp_list));
b045b9
+    vattr_sp_handle *list_entry = vae->sp_list;
b045b9
+    while (list_entry != NULL) {
b045b9
+        vattr_sp_handle *next_entry = list_entry->next;
b045b9
+        slapi_ch_free((void **)&list_entry);
b045b9
+        list_entry = next_entry;
b045b9
+    }
b045b9
     slapi_ch_free_string(&(vae->type_name));
b045b9
     slapi_ch_free((void **)&vae);
b045b9
 }
b045b9
@@ -2143,16 +2148,9 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)
b045b9
 vattr_map_entry *
b045b9
 vattr_map_entry_new(char *type_name, vattr_sp_handle *sph, void *hint)
b045b9
 {
b045b9
-    vattr_map_entry *result = NULL;
b045b9
-    vattr_sp_handle *sp_copy = NULL;
b045b9
-
b045b9
-    sp_copy = (vattr_sp_handle *)slapi_ch_calloc(1, sizeof(vattr_sp_handle));
b045b9
-    sp_copy->sp = sph->sp;
b045b9
-    sp_copy->hint = hint;
b045b9
-
b045b9
-    result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));
b045b9
+    vattr_map_entry *result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));
b045b9
     result->type_name = slapi_ch_strdup(type_name);
b045b9
-    result->sp_list = sp_copy;
b045b9
+    result->sp_list = sph;
b045b9
 
b045b9
     /* go get schema */
b045b9
     result->objectclasses = vattr_map_entry_build_schema(type_name);
b045b9
@@ -2273,6 +2271,16 @@ we'd need to hold a lock on the read path, which we don't want to do.
b045b9
 So any SP which relinquishes its need to handle a type needs to continue
b045b9
 to handle the calls on it, but return nothing */
b045b9
 /* DBDB need to sort out memory ownership here, it's not quite right */
b045b9
+/*
b045b9
+ * This function was inconsistent. We would allocated and "kind of",
b045b9
+ * copy the sp_handle here for the vattr_map_entry_new path. But we
b045b9
+ * would "take ownership" for the existing entry and the list addition
b045b9
+ * path. Instead now, EVERY sp_handle we take, we take ownership of
b045b9
+ * and the CALLER must allocate a new one each time.
b045b9
+ *
b045b9
+ * Better idea, is that regattr should just take the fn pointers
b045b9
+ * and callers never *see* the sp_handle structure at all.
b045b9
+ */
b045b9
 
b045b9
 int
b045b9
 vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)
b045b9
-- 
b045b9
2.13.6
b045b9