Skip to content

Commit

Permalink
MDEV-31636 Memory leak in Sys_var_gtid_binlog_state::do_check()
Browse files Browse the repository at this point in the history
If multiple variables are processed in the same query and one variable
allocates heap memory while a later variable fails to be set, then
we leaked the allcated heap memory.

Mark variables such that we can free any associated memory during a
failure in SQL parsing.
  • Loading branch information
DaveGosselin-MariaDB committed Sep 18, 2024
1 parent 450040e commit 98ebf41
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 1 deletion.
8 changes: 8 additions & 0 deletions mysql-test/main/mdev-31636.result
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
RESET MASTER;
SET
@@global.gtid_binlog_state='1-1-101,2-1-2002',
@@global.slave_parallel_mode=x;
ERROR 42000: Variable 'slave_parallel_mode' can't be set to the value of 'x'
SELECT @@global.gtid_binlog_state;
@@global.gtid_binlog_state

7 changes: 7 additions & 0 deletions mysql-test/main/mdev-31636.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
--source include/have_log_bin.inc
RESET MASTER;
--error ER_WRONG_VALUE_FOR_VAR
SET
@@global.gtid_binlog_state='1-1-101,2-1-2002',
@@global.slave_parallel_mode=x;
SELECT @@global.gtid_binlog_state;
21 changes: 20 additions & 1 deletion sql/set_var.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ bool sys_var::check(THD *thd, set_var *var)
}
return true;
}
var->save_result_exists= true;
return false;
}

Expand Down Expand Up @@ -732,12 +733,15 @@ int sql_set_variables(THD *thd, List<set_var_base> *var_list, bool free)
bool was_error= thd->is_error();
List_iterator_fast<set_var_base> it(*var_list);
DBUG_ENTER("sql_set_variables");
bool failed= false; // true when var::check() returns an error

set_var_base *var;
while ((var=it++))
{
if (unlikely((error= var->check(thd))))
if (unlikely((error= var->check(thd)))) {
failed= true;
goto err;
}
}
if (unlikely(was_error) || likely(!(error= MY_TEST(thd->is_error()))))
{
Expand All @@ -747,6 +751,11 @@ int sql_set_variables(THD *thd, List<set_var_base> *var_list, bool free)
}

err:
if (failed) {
it.rewind();
while ((var=it++))
var->on_error();
}
if (free)
free_underlaid_joins(thd, thd->lex->first_select_lex());
DBUG_RETURN(error);
Expand All @@ -761,6 +770,16 @@ bool sys_var::on_check_access_global(THD *thd) const
return check_global_access(thd, PRIV_SET_GLOBAL_SYSTEM_VARIABLE);
}

void set_var::on_error()
{
/*
Forward the on_error event to the var because it
enapsulates its own error event handling.
*/
if (var)
var->on_error(this);
}

/**
Verify that the supplied value is correct.
Expand Down
6 changes: 6 additions & 0 deletions sql/set_var.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class sys_var: protected Value_source // for double_from_string_with_check
enum where value_origin;
const char *origin_filename;

virtual void on_error(set_var *var) {}

protected:
typedef bool (*on_check_function)(sys_var *self, THD *thd, set_var *var);
typedef bool (*on_update_function)(sys_var *self, THD *thd, enum_var_type type);
Expand Down Expand Up @@ -288,6 +290,7 @@ class set_var_base :public Sql_alloc
@returns whether this variable is @@@@optimizer_trace.
*/
virtual bool is_var_optimizer_trace() const { return false; }
virtual void on_error() {}
};


Expand All @@ -306,7 +309,10 @@ typedef struct my_time_t_hires
*/
class set_var :public set_var_base
{
void on_error() override;

public:
bool save_result_exists{false}; ///< true when sys_var::check succeeds
sys_var *var; ///< system variable to be updated
Item *value; ///< the expression that provides the new value of the variable
enum_var_type type;
Expand Down
14 changes: 14 additions & 0 deletions sql/sys_vars.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2013,6 +2013,20 @@ Sys_gtid_strict_mode(

struct gtid_binlog_state_data { rpl_gtid *list; uint32 list_len; };

void
Sys_var_gtid_binlog_state::on_error(set_var *var)
{
if (!var->save_result_exists)
return;
auto data= (gtid_binlog_state_data *)var->save_result.ptr;
if (data) {
if (data->list)
my_free(data->list);
my_free(data);
}
}


bool
Sys_var_gtid_binlog_state::do_check(THD *thd, set_var *var)
{
Expand Down
2 changes: 2 additions & 0 deletions sql/sys_vars.inl
Original file line number Diff line number Diff line change
Expand Up @@ -2575,6 +2575,8 @@ public:
*/
class Sys_var_gtid_binlog_state: public sys_var
{
void on_error(set_var *var) override;

public:
Sys_var_gtid_binlog_state(const char *name_arg,
const char *comment, int flag_args, ptrdiff_t off, size_t size,
Expand Down

0 comments on commit 98ebf41

Please sign in to comment.