database/mariadb
1
0
Fork 0
mirror of https://github.com/MariaDB/server.git synced 2025-08-01 03:47:19 +03:00
Code Activity

MDEV-30680 Warning: Memory not freed: 280 on mangled query, LeakSanitizer: detected memory leaks

Browse Source 
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 $$; } <expr_lex>  -- 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.
This commit is contained in:
Alexander Barkov
2023-05-15 12:41:31 +04:00
parent 33877cfeae
commit 0d3720c12a
6 changed files with 95 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
mysql-test/main/sp-memory-leak.result Normal file
View File

@ -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

29
mysql-test/main/sp-memory-leak.test Normal file
View File

@ -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 ;$$

24
sql/sp_head.h
View File

@ -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));
}
/**

11
sql/sql_lex.cc
View File

@ -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);
/*

11
sql/sql_parse.cc
View File

@ -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();

24
sql/sql_yacc.yy
View File

@ -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>
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 $$;
} <expr_lex>
%type <assignment_lex>
assignment_source_lex
assignment_source_expr
@ -3806,7 +3811,6 @@ expr_lex:
expr
{
$$= $<expr_lex>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)))