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