Hi, Oleksandr, On Jan 09, Oleksandr Byelkin wrote:

revision-id: d47b1ebc06d (mariadb-10.5.27-143-gd47b1ebc06d) parent(s): 3bbbeae7924 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2025-01-08 14:14:10 +0100 message:

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)

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

diff --git a/sql/sp_head.cc b/sql/sp_head.cc --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -3665,6 +3665,18 @@ 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; + if (m_lex->result)

you don't need this if(), the standard explicitly says that delete of a NULL pointer is valid.

+ delete m_lex->result; + lex_end(m_lex); + delete m_lex; + } +}

/* sp_instr class functions diff --git a/sql/sql_class.cc b/sql/sql_class.cc --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -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))) || + if (unlikely(free_recources()) ||

free_recources() incluses end_io_cache(), you don't need to call it separately above.

unlikely(thd->is_error())) error= true;

@@ -5934,7 +5935,15 @@ class select_to_file :public select_result_interceptor { { path[0]=0; } ~select_to_file(); bool send_eof() override; + void abort_result_set() override; void cleanup() override; + /* + It is separated from cleanup() becaouse:

1. typo 2. if you rename cleanup() to reset_for_next_ps_execution() then you won't need that comment, because, obviously, "freeing resources" is distinct from "resetting for next ps execution" but it's not obvious that "freeing resources" is distinct from "cleanup"

+ 1. cleanup can not be called (conventional execution) + 2. cleanup not only free recources but reset counters (some sends + counter updates in destructor) + */ + bool free_recources(); };

diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc --- 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(). + + ***************************************************************************/

Let's talk about it separately.

+ /* the error was handled or nothing deleted and no side effects return */ if (error_handled || (!thd->transaction->stmt.modified_non_trans_table && !deleted)) diff --git a/sql/sql_update.cc b/sql/sql_update.cc --- 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_delete::send_eof().

multi_update::send_eof()

+ + ***************************************************************************/ + /* Something already updated so we have to invalidate cache */ if (updated) query_cache_invalidate3(thd, update_tables, 1);

Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org