From 0d3720c12a6ff0a2bb104c852e6c204c887b3dd6 Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Mon, 15 May 2023 12:41:31 +0400 Subject: [PATCH] MDEV-30680 Warning: Memory not freed: 280 on mangled query, LeakSanitizer: detected memory leaks The parser works as follows: The rule expr_lex returns a pointer to a newly created sp_expr_lex instance which is not linked to any MariaDB structures yet - it is pointed only from a Bison stack variable. The sp_expr_lex instance gets linked to other structures (such as sp_instr_jump_if_not) later, after scanning some following grammar. Problem before the fix: If a parse error happened immediately after expr_lex (before it got linked), the created sp_expr_lex value got lost causing a memory leak. Fix: - Using Bison's "destructor" directive to free the results of expr_lex on parse/oom errors. - Moving the call for LEX::cleanup_lex_after_parse_error() from MYSQL_YYABORT and yyerror inside parse_sql(). This is needed because Bison calls destructors after yyerror(), while it's important to delete the sp_expr_lex instance before LEX::cleanup_lex_after_parse_error(). The latter frees the memory root containing the sp_expr_lex instance. After this change the code block are executed in the following order: - yyerror() -- now only raises the error to DA (no cleanup done any more) - %destructor { delete $$; } -- destructs the sp_expr_lex instance - LEX::cleanup_lex_after_parse_error() -- frees the memory root containing the sp_expr_lex instance - Removing the "delete sublex" related code from restore_lex(): - restore_lex() is called in most cases on success, when delete is not needed. - There is one place when restore_lex() is called on error: In sp_create_assignment_instr(). But in this case LEX::sp_lex_in_use is true anyway. The patch adds a new DBUG_ASSERT(lex->sp_lex_in_use) to guard this. --- mysql-test/main/sp-memory-leak.result | 21 +++++++++++++++++++ mysql-test/main/sp-memory-leak.test | 29 +++++++++++++++++++++++++++ sql/sp_head.h | 24 +++++++++++++--------- sql/sql_lex.cc | 11 ++++++---- sql/sql_parse.cc | 11 ++++++++++ sql/sql_yacc.yy | 24 ++++++++++++---------- 6 files changed, 95 insertions(+), 25 deletions(-) create mode 100644 mysql-test/main/sp-memory-leak.result create mode 100644 mysql-test/main/sp-memory-leak.test diff --git a/mysql-test/main/sp-memory-leak.result b/mysql-test/main/sp-memory-leak.result new file mode 100644 index 00000000000..aea278801d8 --- /dev/null +++ b/mysql-test/main/sp-memory-leak.result @@ -0,0 +1,21 @@ +# +# MDEV-30680 Warning: Memory not freed: 280 on mangled query, LeakSanitizer: detected memory leaks +# +BEGIN NOT ATOMIC +IF SCALAR() expected_THEN_here; +END +$$ +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'expected_THEN_here; +END' at line 2 +BEGIN NOT ATOMIC +WHILE SCALAR() expected_DO_here; +END +$$ +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'expected_DO_here; +END' at line 2 +BEGIN NOT ATOMIC +REPEAT SELECT 1; UNTIL SCALAR() expected_END_here; +END +$$ +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'expected_END_here; +END' at line 2 diff --git a/mysql-test/main/sp-memory-leak.test b/mysql-test/main/sp-memory-leak.test new file mode 100644 index 00000000000..5b346aa8b10 --- /dev/null +++ b/mysql-test/main/sp-memory-leak.test @@ -0,0 +1,29 @@ +--echo # +--echo # MDEV-30680 Warning: Memory not freed: 280 on mangled query, LeakSanitizer: detected memory leaks +--echo # + +DELIMITER $$; +--error ER_PARSE_ERROR +BEGIN NOT ATOMIC + IF SCALAR() expected_THEN_here; +END +$$ +DELIMITER ;$$ + + +DELIMITER $$; +--error ER_PARSE_ERROR +BEGIN NOT ATOMIC + WHILE SCALAR() expected_DO_here; +END +$$ +DELIMITER ;$$ + + +DELIMITER $$; +--error ER_PARSE_ERROR +BEGIN NOT ATOMIC + REPEAT SELECT 1; UNTIL SCALAR() expected_END_here; +END +$$ +DELIMITER ;$$ diff --git a/sql/sp_head.h b/sql/sp_head.h index 12db9c485cf..700485ffd21 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -608,20 +608,24 @@ public: restore_lex(THD *thd) { DBUG_ENTER("sp_head::restore_lex"); + /* + There is no a need to free the current thd->lex here. + - In the majority of the cases restore_lex() is called + on success and thd->lex does not need to be deleted. + - In cases when restore_lex() is called on error, + e.g. from sp_create_assignment_instr(), thd->lex is + already linked to some sp_instr_xxx (using sp_lex_keeper). + + Note, we don't get to here in case of a syntax error + when the current thd->lex is not yet completely + initialized and linked. It gets automatically deleted + by the Bison %destructor in sql_yacc.yy. + */ LEX *oldlex= (LEX *) m_lex.pop(); if (!oldlex) DBUG_RETURN(false); // Nothing to restore - LEX *sublex= thd->lex; // This restores thd->lex and thd->stmt_lex - if (thd->restore_from_local_lex_to_old_lex(oldlex)) - DBUG_RETURN(true); - if (!sublex->sp_lex_in_use) - { - sublex->sphead= NULL; - lex_end(sublex); - delete sublex; - } - DBUG_RETURN(false); + DBUG_RETURN(thd->restore_from_local_lex_to_old_lex(oldlex)); } /** diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 257acebe9e0..7e0ca6d868d 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -485,11 +485,15 @@ bool sp_create_assignment_instr(THD *thd, bool no_lookahead, be deleted by the destructor ~sp_instr_xxx(). So we should remove "lex" from the stack sp_head::m_lex, to avoid double free. - Note, in case "lex" is not owned by any sp_instr_xxx, - it's also safe to remove it from the stack right now. - So we can remove it unconditionally, without testing lex->sp_lex_in_use. */ lex->sphead->restore_lex(thd); + /* + No needs for "delete lex" here: "lex" is already linked + to the sp_instr_stmt (using sp_lex_keeper) instance created by + the call for new_sp_instr_stmt() above. It will be freed + by ~sp_head/~sp_instr/~sp_lex_keeper during THD::end_statement(). + */ + DBUG_ASSERT(lex->sp_lex_in_use); // used by sp_instr_stmt return true; } enum_var_type inner_option_type= lex->option_type; @@ -6773,7 +6777,6 @@ bool LEX::sp_for_loop_implicit_cursor_statement(THD *thd, if (unlikely(!(bounds->m_index= new (thd->mem_root) sp_assignment_lex(thd, this)))) return true; - bounds->m_index->sp_lex_in_use= true; sphead->reset_lex(thd, bounds->m_index); DBUG_ASSERT(thd->lex != this); /* diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 59f21247445..1e8ea5c5c80 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -10454,6 +10454,17 @@ bool parse_sql(THD *thd, Parser_state *parser_state, ((thd->variables.sql_mode & MODE_ORACLE) ? ORAparse(thd) : MYSQLparse(thd)) != 0; + + if (mysql_parse_status) + { + /* + Restore the original LEX if it was replaced when parsing + a stored procedure. We must ensure that a parsing error + does not leave any side effects in the THD. + */ + LEX::cleanup_lex_after_parse_error(thd); + } + DBUG_ASSERT(opt_bootstrap || mysql_parse_status || thd->lex->select_stack_top == 0); thd->lex->current_select= thd->lex->first_select_lex(); diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index a5a39228776..601ff119fc3 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -98,7 +98,6 @@ int yylex(void *yylval, void *yythd); #define MYSQL_YYABORT \ do \ { \ - LEX::cleanup_lex_after_parse_error(thd); \ YYABORT; \ } while (0) @@ -149,13 +148,6 @@ static Item* escape(THD *thd) static void yyerror(THD *thd, const char *s) { - /* - Restore the original LEX if it was replaced when parsing - a stored procedure. We must ensure that a parsing error - does not leave any side effects in the THD. - */ - LEX::cleanup_lex_after_parse_error(thd); - /* "parse error" changed into "syntax error" between bison 1.75 and 1.875 */ if (strcmp(s,"parse error") == 0 || strcmp(s,"syntax error") == 0) s= ER_THD(thd, ER_SYNTAX_ERROR); @@ -1521,6 +1513,19 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); %type expr_lex +%destructor +{ + /* + In case of a syntax/oom error let's free the sp_expr_lex + instance, but only if it has not been linked to any structures + such as sp_instr_jump_if_not::m_lex_keeper yet, e.g.: + IF f1() THEN1 + i.e. THEN1 came instead of the expected THEN causing a syntax error. + */ + if (!$$->sp_lex_in_use) + delete $$; +} + %type assignment_source_lex assignment_source_expr @@ -3806,7 +3811,6 @@ expr_lex: expr { $$= $1; - $$->sp_lex_in_use= true; $$->set_item($2); Lex->pop_select(); //min select if (Lex->check_cte_dependencies_and_resolve_references()) @@ -3838,7 +3842,6 @@ assignment_source_expr: { DBUG_ASSERT($1 == thd->lex); $$= $1; - $$->sp_lex_in_use= true; $$->set_item_and_free_list($3, thd->free_list); thd->free_list= NULL; Lex->pop_select(); //min select @@ -3859,7 +3862,6 @@ for_loop_bound_expr: { DBUG_ASSERT($1 == thd->lex); $$= $1; - $$->sp_lex_in_use= true; $$->set_item_and_free_list($3, NULL); Lex->pop_select(); //main select if (unlikely($$->sphead->restore_lex(thd)))