From 65c99207e0337854e8202442bead79ef0e3480ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 11 Sep 2023 10:27:21 +0300 Subject: [PATCH] MDEV-23841: Memory leak in innodb_monitor_validate() innodb_monitor_validate(): Let item_val_str() allocate the memory in THD, so that it will be available to innodb_monitor_update(). In this way, there is no need to allocate another buffer, and no problem if the call to innodb_monitor_update() is skipped due to an invalid value that is passed to another configuration parameter. There are some other callers to st_mysql_sys_var::val_str() that validate configuration parameters that are related to FULLTEXT INDEX, but they will allocate memory by invoking thd_strmake(). --- mysql-test/suite/innodb/r/monitor.result | 2 + mysql-test/suite/innodb/t/monitor.test | 2 + storage/innobase/handler/ha_innodb.cc | 78 ++++++------------------ 3 files changed, 22 insertions(+), 60 deletions(-) diff --git a/mysql-test/suite/innodb/r/monitor.result b/mysql-test/suite/innodb/r/monitor.result index b0ed4322fa0..5b7e36ca16c 100644 --- a/mysql-test/suite/innodb/r/monitor.result +++ b/mysql-test/suite/innodb/r/monitor.result @@ -637,6 +637,8 @@ LIKE 'buffer_page_written_index_leaf'; NAME COUNT > 0 buffer_page_written_index_leaf 0 SET GLOBAL innodb_monitor_enable='%'; +SET GLOBAL innodb_monitor_reset_all= '%', innodb_compression_algorithm= foo; +ERROR 42000: Variable 'innodb_compression_algorithm' can't be set to the value of 'foo' INSERT INTO t1 VALUES (5), (6), (7), (8); FLUSH TABLES t1 FOR EXPORT; UNLOCK TABLES; diff --git a/mysql-test/suite/innodb/t/monitor.test b/mysql-test/suite/innodb/t/monitor.test index 8ea56fd53bc..79fb6e80875 100644 --- a/mysql-test/suite/innodb/t/monitor.test +++ b/mysql-test/suite/innodb/t/monitor.test @@ -408,6 +408,8 @@ SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME LIKE 'buffer_page_written_index_leaf'; SET GLOBAL innodb_monitor_enable='%'; +--error ER_WRONG_VALUE_FOR_VAR +SET GLOBAL innodb_monitor_reset_all= '%', innodb_compression_algorithm= foo; INSERT INTO t1 VALUES (5), (6), (7), (8); FLUSH TABLES t1 FOR EXPORT; UNLOCK TABLES; SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 2676fa3fc25..8116b3f499a 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -17808,13 +17808,7 @@ innodb_monitor_validate_wildcard_name( Validate the passed in monitor name, find and save the corresponding monitor name in the function parameter "save". @return 0 if monitor name is valid */ -static -int -innodb_monitor_valid_byname( -/*========================*/ - void* save, /*!< out: immediate result - for update function */ - const char* name) /*!< in: incoming monitor name */ +static int innodb_monitor_valid_byname(const char *name) { ulint use; monitor_info_t* monitor_info; @@ -17862,9 +17856,6 @@ innodb_monitor_valid_byname( } } - /* Save the configure name for innodb_monitor_update() */ - *static_cast(save) = name; - return(0); } /*************************************************************//** @@ -17880,41 +17871,18 @@ innodb_monitor_validate( for update function */ struct st_mysql_value* value) /*!< in: incoming string */ { - const char* name; - char* monitor_name; - char buff[STRING_BUFFER_USUAL_SIZE]; - int len = sizeof(buff); - int ret; + int ret= 0; - ut_a(save != NULL); - ut_a(value != NULL); + if (const char *name= value->val_str(value, nullptr, &ret)) + { + ret= innodb_monitor_valid_byname(name); + if (!ret) + *static_cast(save)= name; + } + else + ret= 1; - name = value->val_str(value, buff, &len); - - /* monitor_name could point to memory from MySQL - or buff[]. Always dup the name to memory allocated - by InnoDB, so we can access it in another callback - function innodb_monitor_update() and free it appropriately */ - if (name) { - monitor_name = my_strdup(//PSI_INSTRUMENT_ME, - name, MYF(0)); - } else { - return(1); - } - - ret = innodb_monitor_valid_byname(save, monitor_name); - - if (ret) { - /* Validation failed */ - my_free(monitor_name); - } else { - /* monitor_name will be freed in separate callback function - innodb_monitor_update(). Assert "save" point to - the "monitor_name" variable */ - ut_ad(*static_cast(save) == monitor_name); - } - - return(ret); + return ret; } /****************************************************************//** @@ -17930,11 +17898,9 @@ innodb_monitor_update( formal string goes */ const void* save, /*!< in: immediate result from check function */ - mon_option_t set_option, /*!< in: the set option, + mon_option_t set_option) /*!< in: the set option, whether to turn on/off or reset the counter */ - ibool free_mem) /*!< in: whether we will - need to free the memory */ { monitor_info_t* monitor_info; ulint monitor_id; @@ -18018,12 +17984,6 @@ exit: sql_print_warning("InnoDB: Monitor %s is already enabled.", srv_mon_get_name((monitor_id_t) err_monitor)); } - - if (free_mem && name) { - my_free((void*) name); - } - - return; } #ifdef UNIV_DEBUG @@ -18114,7 +18074,7 @@ innodb_enable_monitor_update( const void* save) /*!< in: immediate result from check function */ { - innodb_monitor_update(thd, var_ptr, save, MONITOR_TURN_ON, TRUE); + innodb_monitor_update(thd, var_ptr, save, MONITOR_TURN_ON); } /****************************************************************//** @@ -18131,7 +18091,7 @@ innodb_disable_monitor_update( const void* save) /*!< in: immediate result from check function */ { - innodb_monitor_update(thd, var_ptr, save, MONITOR_TURN_OFF, TRUE); + innodb_monitor_update(thd, var_ptr, save, MONITOR_TURN_OFF); } /****************************************************************//** @@ -18149,7 +18109,7 @@ innodb_reset_monitor_update( const void* save) /*!< in: immediate result from check function */ { - innodb_monitor_update(thd, var_ptr, save, MONITOR_RESET_VALUE, TRUE); + innodb_monitor_update(thd, var_ptr, save, MONITOR_RESET_VALUE); } /****************************************************************//** @@ -18167,8 +18127,7 @@ innodb_reset_all_monitor_update( const void* save) /*!< in: immediate result from check function */ { - innodb_monitor_update(thd, var_ptr, save, MONITOR_RESET_ALL_VALUE, - TRUE); + innodb_monitor_update(thd, var_ptr, save, MONITOR_RESET_ALL_VALUE); } static @@ -18213,10 +18172,9 @@ innodb_enable_monitor_at_startup( for (char* option = my_strtok_r(str, sep, &last); option; option = my_strtok_r(NULL, sep, &last)) { - char* option_name; - if (!innodb_monitor_valid_byname(&option_name, option)) { + if (!innodb_monitor_valid_byname(option)) { innodb_monitor_update(NULL, NULL, &option, - MONITOR_TURN_ON, FALSE); + MONITOR_TURN_ON); } else { sql_print_warning("Invalid monitor counter" " name: '%s'", option);