From dcf75750dff23e848cde2ae63a0778b123de6dd7 Mon Sep 17 00:00:00 2001 From: William Brown 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