From 0d35fe6e5761cbee731ffcac55f2bf8eaf2a6a8f Mon Sep 17 00:00:00 2001 From: Oleksandr Byelkin Date: Mon, 23 Dec 2024 22:36:01 +0100 Subject: [PATCH] MDEV-35326: Memory Leak in init_io_cache_ext upon SHUTDOWN The problems were that: 1) resources was freed "asimetric" normal execution in send_eof, in case of error in destructor. 2) destructor was not called in case of SP for result objects. (so if the last SP execution ended with error resorces was not freeded on reinit before execution (cleanup() called before next execution) and destructor also was not called due to lack of delete call for the object) Result cleanup() renamed to reset_for_next_ps_execution() to better reflect function(). All result method revised and freeing resources made "symetric". Destructor of result object called for SP. Added skipped invalidation in case of error in insert. Removed misleading naming of reset(thd) (could be mixed with with reset()). --- sql/item_subselect.cc | 6 +- sql/sp_head.cc | 11 ++++ sql/sp_head.h | 11 +--- sql/sql_class.cc | 56 +++++++++++-------- sql/sql_class.h | 48 ++++++++-------- sql/sql_delete.cc | 14 +++++ sql/sql_insert.cc | 24 +++++++- sql/sql_prepare.cc | 2 +- sql/sql_union.cc | 10 ++-- sql/sql_update.cc | 14 +++++ .../spider/bugfix/r/mdev_35326.result | 14 +++++ .../spider/bugfix/t/mdev_35326.test | 21 +++++++ 12 files changed, 162 insertions(+), 69 deletions(-) create mode 100644 storage/spider/mysql-test/spider/bugfix/r/mdev_35326.result create mode 100644 storage/spider/mysql-test/spider/bugfix/t/mdev_35326.test diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc index c0ac4022cd9..8da808ca3a0 100644 --- a/sql/item_subselect.cc +++ b/sql/item_subselect.cc @@ -3753,7 +3753,7 @@ void subselect_single_select_engine::cleanup() DBUG_ENTER("subselect_single_select_engine::cleanup"); prepared= executed= 0; join= 0; - result->cleanup(); + result->reset_for_next_ps_execution(); select_lex->uncacheable&= ~UNCACHEABLE_DEPENDENT_INJECTED; DBUG_VOID_RETURN; } @@ -3763,7 +3763,7 @@ void subselect_union_engine::cleanup() { DBUG_ENTER("subselect_union_engine::cleanup"); unit->reinit_exec_mechanism(); - result->cleanup(); + result->reset_for_next_ps_execution(); unit->uncacheable&= ~UNCACHEABLE_DEPENDENT_INJECTED; for (SELECT_LEX *sl= unit->first_select(); sl; sl= sl->next_select()) sl->uncacheable&= ~UNCACHEABLE_DEPENDENT_INJECTED; @@ -5436,7 +5436,7 @@ void subselect_hash_sj_engine::cleanup() } DBUG_ASSERT(lookup_engine->engine_type() == UNIQUESUBQUERY_ENGINE); lookup_engine->cleanup(); - result->cleanup(); /* Resets the temp table as well. */ + result->reset_for_next_ps_execution(); /* Resets the temp table as well. */ DBUG_ASSERT(tmp_table); free_tmp_table(thd, tmp_table); tmp_table= NULL; diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 506e755c2fd..c751636fbf7 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -3665,6 +3665,17 @@ int sp_lex_keeper::cursor_reset_lex_and_exec_core(THD *thd, uint *nextp, return res; } +sp_lex_keeper::~sp_lex_keeper() +{ + if (m_lex_resp) + { + /* Prevent endless recursion. */ + m_lex->sphead= NULL; + delete m_lex->result; + lex_end(m_lex); + delete m_lex; + } +} /* sp_instr class functions diff --git a/sql/sp_head.h b/sql/sp_head.h index 3de8e843753..ba677053e87 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -1273,16 +1273,7 @@ public: { lex->sp_lex_in_use= TRUE; } - virtual ~sp_lex_keeper() - { - if (m_lex_resp) - { - /* Prevent endless recursion. */ - m_lex->sphead= NULL; - lex_end(m_lex); - delete m_lex; - } - } + virtual ~sp_lex_keeper(); /** Prepare execution of instruction using LEX, if requested check whenever diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 73e182edd0f..8cb5b4954aa 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -3066,7 +3066,7 @@ void Item_change_list::rollback_item_tree_changes() ** Functions to provide a interface to select results *****************************************************************************/ -void select_result::cleanup() +void select_result::reset_for_next_ps_execution() { /* do nothing */ } @@ -3135,6 +3135,7 @@ void select_send::abort_result_set() */ thd->spcont->end_partial_result_set= TRUE; } + reset_for_next_ps_execution(); DBUG_VOID_RETURN; } @@ -3145,7 +3146,7 @@ void select_send::abort_result_set() stored procedure statement. */ -void select_send::cleanup() +void select_send::reset_for_next_ps_execution() { is_result_set_started= FALSE; } @@ -3183,7 +3184,7 @@ bool select_send::send_eof() if (unlikely(thd->is_error())) return TRUE; ::my_eof(thd); - is_result_set_started= 0; + reset_for_next_ps_execution(); return FALSE; } @@ -3192,10 +3193,22 @@ bool select_send::send_eof() Handling writing to file ************************************************************************/ +bool select_to_file::free_recources() +{ + if (file >= 0) + { + (void) end_io_cache(&cache); + bool error= mysql_file_close(file, MYF(MY_WME)); + file= -1; + return error; + } + return FALSE; +} + bool select_to_file::send_eof() { - int error= MY_TEST(end_io_cache(&cache)); - if (unlikely(mysql_file_close(file, MYF(MY_WME))) || + int error= false; + if (unlikely(free_recources()) || unlikely(thd->is_error())) error= true; @@ -3203,20 +3216,19 @@ bool select_to_file::send_eof() { ::my_ok(thd,row_count); } - file= -1; return error; } +void select_to_file::abort_result_set() +{ + select_result_interceptor::abort_result_set(); + free_recources(); +} -void select_to_file::cleanup() +void select_to_file::reset_for_next_ps_execution() { /* In case of error send_eof() may be not called: close the file here. */ - if (file >= 0) - { - (void) end_io_cache(&cache); - mysql_file_close(file, MYF(0)); - file= -1; - } + free_recources(); path[0]= '\0'; row_count= 0; } @@ -3224,12 +3236,8 @@ void select_to_file::cleanup() select_to_file::~select_to_file() { - if (file >= 0) - { // This only happens in case of error - (void) end_io_cache(&cache); - mysql_file_close(file, MYF(0)); - file= -1; - } + DBUG_ASSERT(file < 0); + free_recources(); // just in case } /*************************************************************************** @@ -3716,9 +3724,9 @@ int select_singlerow_subselect::send_data(List &items) } -void select_max_min_finder_subselect::cleanup() +void select_max_min_finder_subselect::reset_for_next_ps_execution() { - DBUG_ENTER("select_max_min_finder_subselect::cleanup"); + DBUG_ENTER("select_max_min_finder_subselect::reset_for_next_ps_execution"); cache= 0; DBUG_VOID_RETURN; } @@ -3943,7 +3951,7 @@ bool select_dumpvar::check_simple_select() const } -void select_dumpvar::cleanup() +void select_dumpvar::reset_for_next_ps_execution() { row_count= 0; } @@ -4372,10 +4380,10 @@ void select_materialize_with_stats::reset() } -void select_materialize_with_stats::cleanup() +void select_materialize_with_stats::reset_for_next_ps_execution() { reset(); - select_unit::cleanup(); + select_unit::reset_for_next_ps_execution(); } diff --git a/sql/sql_class.h b/sql/sql_class.h index 28aef92c31e..032d0bc2606 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -5585,7 +5585,8 @@ public: */ virtual int send_data(List &items)=0; virtual ~select_result_sink() = default; - void reset(THD *thd_arg) { thd= thd_arg; } + // Used in cursors to initialize and reset + void reinit(THD *thd_arg) { thd= thd_arg; } }; class select_result_interceptor; @@ -5659,15 +5660,11 @@ public: */ virtual bool check_simple_select() const; virtual void abort_result_set() {} - /* - Cleanup instance of this class for next execution of a prepared - statement/stored procedure. - */ - virtual void cleanup(); + virtual void reset_for_next_ps_execution(); void set_thd(THD *thd_arg) { thd= thd_arg; } - void reset(THD *thd_arg) + void reinit(THD *thd_arg) { - select_result_sink::reset(thd_arg); + select_result_sink::reinit(thd_arg); unit= NULL; } #ifdef EMBEDDED_LIBRARY @@ -5773,9 +5770,9 @@ public: elsewhere. (this is used by ANALYZE $stmt feature). */ void disable_my_ok_calls() { suppress_my_ok= true; } - void reset(THD *thd_arg) + void reinit(THD *thd_arg) { - select_result::reset(thd_arg); + select_result::reinit(thd_arg); suppress_my_ok= false; } protected: @@ -5827,7 +5824,7 @@ private: {} void reset(THD *thd_arg) { - select_result_interceptor::reset(thd_arg); + select_result_interceptor::reinit(thd_arg); spvar_list= NULL; field_count= 0; } @@ -5871,7 +5868,7 @@ public: void reset(THD *thd_arg, sp_lex_keeper *lex_keeper) { sp_cursor_statistics::reset(); - result.reset(thd_arg); + result.reinit(thd_arg); m_lex_keeper= lex_keeper; server_side_cursor= NULL; } @@ -5899,7 +5896,7 @@ public: bool send_eof() override; bool check_simple_select() const override { return FALSE; } void abort_result_set() override; - void cleanup() override; + void reset_for_next_ps_execution() override; select_result_interceptor *result_interceptor() override { return NULL; } }; @@ -5934,7 +5931,9 @@ public: { path[0]=0; } ~select_to_file(); bool send_eof() override; - void cleanup() override; + void abort_result_set() override; + void reset_for_next_ps_execution() override; + bool free_recources(); }; @@ -6011,7 +6010,7 @@ class select_insert :public select_result_interceptor { bool send_eof() override; void abort_result_set() override; /* not implemented: select_insert is never re-used in prepared statements */ - void cleanup() override; + void reset_for_next_ps_execution() override; }; @@ -6231,7 +6230,7 @@ public: int delete_record(); bool send_eof() override; virtual bool flush(); - void cleanup() override; + void reset_for_next_ps_execution() override; virtual bool create_result_table(THD *thd, List *column_types, bool is_distinct, ulonglong options, const LEX_CSTRING *alias, @@ -6406,9 +6405,10 @@ class select_union_recursive :public select_unit */ List rec_table_refs; /* - The count of how many times cleanup() was called with cleaned==false - for the unit specifying the recursive CTE for which this object was created - or for the unit specifying a CTE that mutually recursive with this CTE. + The count of how many times reset_for_next_ps_execution() was called with + cleaned==false for the unit specifying the recursive CTE for which this + object was created or for the unit specifying a CTE that mutually + recursive with this CTE. */ uint cleanup_count; long row_counter; @@ -6427,7 +6427,7 @@ class select_union_recursive :public select_unit bool create_table, bool keep_row_order, uint hidden) override; - void cleanup() override; + void reset_for_next_ps_execution() override; }; /** @@ -6497,7 +6497,7 @@ public: { result->abort_result_set(); /* purecov: inspected */ } - void cleanup() override + void reset_for_next_ps_execution() override { send_records= 0; } @@ -6600,7 +6600,7 @@ public: uint hidden) override; bool init_result_table(ulonglong select_options); int send_data(List &items) override; - void cleanup() override; + void reset_for_next_ps_execution() override; ha_rows get_null_count_of_col(uint idx) { DBUG_ASSERT(idx < table->s->fields); @@ -6633,7 +6633,7 @@ public: bool mx, bool all): select_subselect(thd_arg, item_arg), cache(0), fmax(mx), is_all(all) {} - void cleanup() override; + void reset_for_next_ps_execution() override; int send_data(List &items) override; bool cmp_real(); bool cmp_int(); @@ -7049,7 +7049,7 @@ public: int send_data(List &items) override; bool send_eof() override; bool check_simple_select() const override; - void cleanup() override; + void reset_for_next_ps_execution() override; }; /* Bits in sql_command_flags */ diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 25b3aef3ebe..fa677feb551 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -1422,6 +1422,13 @@ void multi_delete::abort_result_set() { DBUG_ENTER("multi_delete::abort_result_set"); + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + multi_delete::send_eof(). + + ***************************************************************************/ + /* the error was handled or nothing deleted and no side effects return */ if (error_handled || (!thd->transaction->stmt.modified_non_trans_table && !deleted)) @@ -1622,6 +1629,13 @@ bool multi_delete::send_eof() /* reset used flags */ THD_STAGE_INFO(thd, stage_end); + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + multi_delete::abort_result_set(). + + ***************************************************************************/ + if (thd->transaction->stmt.modified_non_trans_table) thd->transaction->all.modified_non_trans_table= TRUE; thd->transaction->all.m_unsafe_rollback_flags|= diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index e1d87e29869..000695c5399 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4131,7 +4131,7 @@ int select_insert::prepare2(JOIN *) } -void select_insert::cleanup() +void select_insert::reset_for_next_ps_execution() { /* select_insert/select_create are never re-used in prepared statement */ DBUG_ASSERT(0); @@ -4245,6 +4245,13 @@ bool select_insert::prepare_eof() DBUG_PRINT("enter", ("trans_table: %d, table_type: '%s'", trans_table, table->file->table_type())); + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + select_insert::abort_result_set(). + + ****************************************************************************/ + #ifdef WITH_WSREP error= (thd->wsrep_cs().current_error()) ? -1 : (thd->locked_tables_mode <= LTM_LOCK_TABLES) ? @@ -4377,6 +4384,12 @@ void select_insert::abort_result_set() */ if (table && table->file->is_open()) { + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + select_insert::prepare_eof(). + + ****************************************************************************/ bool changed, transactional_table; /* If we are not in prelocked mode, we end the bulk insert started @@ -4404,7 +4417,14 @@ void select_insert::abort_result_set() If table creation failed, the number of rows modified will also be zero, so no check for that is made. */ - changed= (info.copied || info.deleted || info.updated); + if ((changed= (info.copied || info.deleted || info.updated))) + { + /* + We must invalidate the table in the query cache before binlog writing + and ha_autocommit_or_rollback. + */ + query_cache_invalidate3(thd, table, 1); + } transactional_table= table->file->has_transactions_and_rollback(); if (thd->transaction->stmt.modified_non_trans_table || thd->log_current_statement) diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 93e81b2025d..9d53d7cf543 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -3194,7 +3194,7 @@ void reinit_stmt_before_use(THD *thd, LEX *lex) if (lex->result) { - lex->result->cleanup(); + lex->result->reset_for_next_ps_execution(); lex->result->set_thd(thd); } lex->allow_sum_func.clear_all(); diff --git a/sql/sql_union.cc b/sql/sql_union.cc index 53e604bfa92..09dc045b543 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -547,7 +547,7 @@ int select_unit::delete_record() tables of JOIN - exec_tmp_table_[1 | 2]. */ -void select_unit::cleanup() +void select_unit::reset_for_next_ps_execution() { table->file->extra(HA_EXTRA_RESET_STATE); table->file->ha_delete_all_rows(); @@ -902,11 +902,11 @@ bool select_unit_ext::send_eof() return (MY_TEST(error)); } -void select_union_recursive::cleanup() +void select_union_recursive::reset_for_next_ps_execution() { if (table) { - select_unit::cleanup(); + select_unit::reset_for_next_ps_execution(); free_tmp_table(thd, table); } @@ -2194,7 +2194,7 @@ bool st_select_lex_unit::exec() if (uncacheable || !item || !item->assigned() || describe) { if (!fake_select_lex && !(with_element && with_element->is_recursive)) - union_result->cleanup(); + union_result->reset_for_next_ps_execution(); for (SELECT_LEX *sl= select_cursor; sl; sl= sl->next_select()) { ha_rows records_at_start= 0; @@ -2636,7 +2636,7 @@ bool st_select_lex_unit::cleanup() { if (union_result) { - ((select_union_recursive *) union_result)->cleanup(); + ((select_union_recursive *) union_result)->reset_for_next_ps_execution(); delete union_result; union_result= 0; } diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 4a13d83d52f..9dfb681b119 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -2743,6 +2743,13 @@ void multi_update::abort_result_set() (!thd->transaction->stmt.modified_non_trans_table && !updated))) return; + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + multi_update::send_eof(). + + ***************************************************************************/ + /* Something already updated so we have to invalidate cache */ if (updated) query_cache_invalidate3(thd, update_tables, 1); @@ -3072,6 +3079,13 @@ bool multi_update::send_eof() killed_status= (local_error == 0) ? NOT_KILLED : thd->killed; THD_STAGE_INFO(thd, stage_end); + /**************************************************************************** + + NOTE: if you change here be aware that almost the same code is in + multi_update::abort_result_set(). + + ***************************************************************************/ + /* We must invalidate the query cache before binlog writing and ha_autocommit_... */ diff --git a/storage/spider/mysql-test/spider/bugfix/r/mdev_35326.result b/storage/spider/mysql-test/spider/bugfix/r/mdev_35326.result new file mode 100644 index 00000000000..41481447a6c --- /dev/null +++ b/storage/spider/mysql-test/spider/bugfix/r/mdev_35326.result @@ -0,0 +1,14 @@ +for master_1 +for child2 +for child3 +CREATE TABLE t (c INT) ENGINE=Spider; +CREATE PROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt'; +Warnings: +Warning 1287 ' INTO FROM...' instead +CALL p(); +ERROR HY000: Unable to connect to foreign data source: localhost +drop procedure p; +drop table t; +for master_1 +for child2 +for child3 diff --git a/storage/spider/mysql-test/spider/bugfix/t/mdev_35326.test b/storage/spider/mysql-test/spider/bugfix/t/mdev_35326.test new file mode 100644 index 00000000000..1ce8e1aa780 --- /dev/null +++ b/storage/spider/mysql-test/spider/bugfix/t/mdev_35326.test @@ -0,0 +1,21 @@ +--disable_query_log +--disable_result_log +--source ../../t/test_init.inc +--enable_result_log +--enable_query_log + +let $MYSQLD_DATADIR= `select @@datadir`; + +CREATE TABLE t (c INT) ENGINE=Spider; +CREATE PROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt'; +--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE +CALL p(); +remove_file $MYSQLD_DATADIR/test/foo.txt; +drop procedure p; +drop table t; + +--disable_query_log +--disable_result_log +--source ../../t/test_deinit.inc +--enable_result_log +--enable_query_log