From 80ac6e66c12e727c74b02a108b8e5f8052fbfca1 Mon Sep 17 00:00:00 2001 From: Ludwig Krispenz Date: Jul 04 2019 10:01:29 +0000 Subject: Ticket 50445 - ldbm_config_*_set functions need to validate valuse in apply=0 mode Bug: Some config set functions do not perform the checks for passed values if called in apply=0 mode. There is a memory leak if setting the config values in ldbm_config_modify_entry_callback() fails and reapply_mods was aready set. Fix: to prevent the memory leak only modify the list of mods if all parameter settings will succeed. this also requires that checks are correctly execute in apply mode 0. For all confi set functions which do checks do it also for apply_mode 0. Reviewd by: ? --- diff --git a/ldap/servers/slapd/back-ldbm/ldbm_config.c b/ldap/servers/slapd/back-ldbm/ldbm_config.c index 67a6475..59f176f 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_config.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_config.c @@ -434,45 +434,48 @@ ldbm_config_dbcachesize_set(void *arg, void *value, char *errorbuf, int phase, i * sure that it is sane. */ - if (apply) { /* Stop the user configuring a stupidly small cache */ /* min: 8KB (page size) * def thrd cnts (threadnumber==20). */ #define DBDEFMINSIZ 500000 - /* We allow a value of 0, because the autotuning in start.c will - * register that, and trigger the recalculation of the dbcachesize as - * needed on the next start up. - */ - if (val < DBDEFMINSIZ && val > 0) { - slapi_log_err(SLAPI_LOG_NOTICE, "ldbm_config_dbcachesize_set", "cache too small, increasing to %dK bytes\n", - DBDEFMINSIZ / 1000); - val = DBDEFMINSIZ; - } else if (val > li->li_dbcachesize) { - delta = val - li->li_dbcachesize; - - util_cachesize_result sane; - slapi_pal_meminfo *mi = spal_meminfo_get(); - sane = util_is_cachesize_sane(mi, &delta); - spal_meminfo_destroy(mi); - - if (sane != UTIL_CACHESIZE_VALID) { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: nsslapd-dbcachesize value is too large."); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_dbcachesize_set", - "nsslapd-dbcachesize value is too large.\n"); - return LDAP_UNWILLING_TO_PERFORM; - } + /* We allow a value of 0, because the autotuning in start.c will + * register that, and trigger the recalculation of the dbcachesize as + * needed on the next start up. + */ + if (val < DBDEFMINSIZ && val > 0) { + slapi_log_err(SLAPI_LOG_NOTICE, "ldbm_config_dbcachesize_set", "cache too small, increasing to %dK bytes\n", + DBDEFMINSIZ / 1000); + val = DBDEFMINSIZ; + } else if (val > li->li_dbcachesize) { + delta = val - li->li_dbcachesize; + + util_cachesize_result sane; + slapi_pal_meminfo *mi = spal_meminfo_get(); + sane = util_is_cachesize_sane(mi, &delta); + spal_meminfo_destroy(mi); + + if (sane != UTIL_CACHESIZE_VALID) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: nsslapd-dbcachesize value is too large."); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_dbcachesize_set", + "nsslapd-dbcachesize value is too large.\n"); + return LDAP_UNWILLING_TO_PERFORM; + } + } + + if (CONFIG_PHASE_RUNNING == phase) { + if (val > 0 && li->li_cache_autosize) { + /* We are auto-tuning the cache, so this change would be overwritten - return an error */ + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Error: \"nsslapd-dbcachesize\" can not be updated while \"nsslapd-cache-autosize\" is set " + "in \"cn=config,cn=ldbm database,cn=plugins,cn=config\"."); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_dbcachesize_set", + "\"nsslapd-dbcachesize\" can not be set while \"nsslapd-cache-autosize\" is set " + "in \"cn=config,cn=ldbm database,cn=plugins,cn=config\".\n"); + return LDAP_UNWILLING_TO_PERFORM; } + } + if(apply) { if (CONFIG_PHASE_RUNNING == phase) { - if (val > 0 && li->li_cache_autosize) { - /* We are auto-tuning the cache, so this change would be overwritten - return an error */ - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, - "Error: \"nsslapd-dbcachesize\" can not be updated while \"nsslapd-cache-autosize\" is set " - "in \"cn=config,cn=ldbm database,cn=plugins,cn=config\"."); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_dbcachesize_set", - "\"nsslapd-dbcachesize\" can not be set while \"nsslapd-cache-autosize\" is set " - "in \"cn=config,cn=ldbm database,cn=plugins,cn=config\".\n"); - return LDAP_UNWILLING_TO_PERFORM; - } li->li_new_dbcachesize = val; if (val == 0) { slapi_log_err(SLAPI_LOG_NOTICE, "ldbm_config_dbcachesize_set", "cache size reset to 0, will be autosized on next startup.\n"); @@ -503,13 +506,13 @@ ldbm_config_maxpassbeforemerge_set(void *arg, void *value, char *errorbuf __attr int retval = LDAP_SUCCESS; int val = (int)((uintptr_t)value); - if (apply) { - if (val < 0) { - slapi_log_err(SLAPI_LOG_NOTICE, "ldbm_config_maxpassbeforemerge_set", - "maxpassbeforemerge will not take negative value - setting to 100\n"); - val = 100; - } + if (val < 0) { + slapi_log_err(SLAPI_LOG_NOTICE, "ldbm_config_maxpassbeforemerge_set", + "maxpassbeforemerge will not take negative value - setting to 100\n"); + val = 100; + } + if (apply) { li->li_maxpassbeforemerge = val; } @@ -894,15 +897,15 @@ ldbm_config_db_old_idl_maxids_set(void *arg, int retval = LDAP_SUCCESS; int val = (int)((uintptr_t)value); + if (val < 0) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Error: Invalid value for %s (%d). Value must be equal or greater than zero.", + CONFIG_DB_OLD_IDL_MAXIDS, val); + return LDAP_UNWILLING_TO_PERFORM; + } + if (apply) { - if (val >= 0) { - li->li_old_idl_maxids = val; - } else { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, - "Error: Invalid value for %s (%d). Value must be equal or greater than zero.", - CONFIG_DB_OLD_IDL_MAXIDS, val); - return LDAP_UNWILLING_TO_PERFORM; - } + li->li_old_idl_maxids = val; } return retval; @@ -1270,21 +1273,21 @@ ldbm_config_db_cache_set(void *arg, * sure that it is sane. */ - if (apply) { - if (val > li->li_dblayer_private->dblayer_cache_config) { - delta = val - li->li_dblayer_private->dblayer_cache_config; - util_cachesize_result sane; - - slapi_pal_meminfo *mi = spal_meminfo_get(); - sane = util_is_cachesize_sane(mi, &delta); - spal_meminfo_destroy(mi); - - if (sane != UTIL_CACHESIZE_VALID) { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: db cachesize value is too large"); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_db_cache_set", "db cachesize value is too large.\n"); - return LDAP_UNWILLING_TO_PERFORM; - } + if (val > li->li_dblayer_private->dblayer_cache_config) { + delta = val - li->li_dblayer_private->dblayer_cache_config; + util_cachesize_result sane; + + slapi_pal_meminfo *mi = spal_meminfo_get(); + sane = util_is_cachesize_sane(mi, &delta); + spal_meminfo_destroy(mi); + + if (sane != UTIL_CACHESIZE_VALID) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: db cachesize value is too large"); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_db_cache_set", "db cachesize value is too large.\n"); + return LDAP_UNWILLING_TO_PERFORM; } + } + if (apply) { li->li_dblayer_private->dblayer_cache_config = val; } @@ -1385,17 +1388,17 @@ ldbm_config_cache_autosize_set(void *arg, { struct ldbminfo *li = (struct ldbminfo *)arg; + int val = (int)((uintptr_t)value); + if (val < 0 || val > 100) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Error: Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", + CONFIG_CACHE_AUTOSIZE, val); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_cache_autosize_set", + "Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", + CONFIG_CACHE_AUTOSIZE, val); + return LDAP_UNWILLING_TO_PERFORM; + } if (apply) { - int val = (int)((uintptr_t)value); - if (val < 0 || val > 100) { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, - "Error: Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", - CONFIG_CACHE_AUTOSIZE, val); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_cache_autosize_set", - "Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", - CONFIG_CACHE_AUTOSIZE, val); - return LDAP_UNWILLING_TO_PERFORM; - } li->li_cache_autosize = val; } return LDAP_SUCCESS; @@ -1418,17 +1421,17 @@ ldbm_config_cache_autosize_split_set(void *arg, { struct ldbminfo *li = (struct ldbminfo *)arg; + int val = (int)((uintptr_t)value); + if (val < 0 || val > 100) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Error: Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", + CONFIG_CACHE_AUTOSIZE_SPLIT, val); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_cache_autosize_split_set", + "Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", + CONFIG_CACHE_AUTOSIZE_SPLIT, val); + return LDAP_UNWILLING_TO_PERFORM; + } if (apply) { - int val = (int)((uintptr_t)value); - if (val < 0 || val > 100) { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, - "Error: Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", - CONFIG_CACHE_AUTOSIZE_SPLIT, val); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_cache_autosize_split_set", - "Invalid value for %s (%d). The value must be between \"0\" and \"100\"\n", - CONFIG_CACHE_AUTOSIZE_SPLIT, val); - return LDAP_UNWILLING_TO_PERFORM; - } li->li_cache_autosize_split = val; } return LDAP_SUCCESS; @@ -1461,22 +1464,22 @@ ldbm_config_import_cachesize_set(void *arg, * If we are setting a LARGER value, we check the delta of the two, and make * sure that it is sane. */ - if (apply) { - if (val > li->li_import_cachesize) { - delta = val - li->li_import_cachesize; - - util_cachesize_result sane; - slapi_pal_meminfo *mi = spal_meminfo_get(); - sane = util_is_cachesize_sane(mi, &delta); - spal_meminfo_destroy(mi); - - if (sane != UTIL_CACHESIZE_VALID) { - slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: import cachesize value is too large."); - slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_import_cachesize_set", - "Import cachesize value is too large.\n"); - return LDAP_UNWILLING_TO_PERFORM; - } + if (val > li->li_import_cachesize) { + delta = val - li->li_import_cachesize; + + util_cachesize_result sane; + slapi_pal_meminfo *mi = spal_meminfo_get(); + sane = util_is_cachesize_sane(mi, &delta); + spal_meminfo_destroy(mi); + + if (sane != UTIL_CACHESIZE_VALID) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "Error: import cachesize value is too large."); + slapi_log_err(SLAPI_LOG_ERR, "ldbm_config_import_cachesize_set", + "Import cachesize value is too large.\n"); + return LDAP_UNWILLING_TO_PERFORM; } + } + if (apply) { li->li_import_cachesize = val; } return LDAP_SUCCESS; @@ -2500,6 +2503,23 @@ ldbm_config_modify_entry_callback(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Sl returntext[0] = '\0'; + /* This function does: + * - check if the provided config modifications are valid. + * since there is no transaction for changes to config entries + * we do it in two passes. The first pass (apply_mod=0) just checks + * that changes are valid, if an error occurs procesing is aborted + * and function returns. In the second pass (apply_mod=1) changes + * are efectivly applied. + * - remove ignored attributes. Some attributes which were added to the + * list of mods (eg modifytimestamp) are irrelevant for the config + * changes and can be ignored, to ignore them also in the following + * plugin calls they are removed from the list of mods and from the + * postEntry, the setting of reapply mods ensures that the caller will + * reset the original mods. (Note by LK: I think it is not clear why + * this is done here, the callback should have no knowledge of what will + * follow and why the mods are removed, for the ldbm part it would be + * sufficient to just ignore them) + */ /* * First pass: set apply mods to 0 so only input validation will be done; * 2nd pass: set apply mods to 1 to apply changes to internal storage @@ -2525,11 +2545,11 @@ ldbm_config_modify_entry_callback(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Sl slapi_valueset_free(origvalues); } } + reapply_mods = 1; /* there is at least one mod we removed */ } continue; } - reapply_mods = 1; /* there is at least one mod we removed */ /* when deleting a value, and this is the last or only value, set the config param to its default value when adding a value, if the value is set to its default value, replace