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

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

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

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

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<Item> &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();
 }
 
 

sql/sql_class.h

@@ -5585,7 +5585,8 @@ public:
   */
   virtual int send_data(List<Item> &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<Item> *column_types,
                                    bool is_distinct, ulonglong options,
                                    const LEX_CSTRING *alias,
@@ -6406,9 +6405,10 @@ class select_union_recursive :public select_unit
  */
   List<TABLE_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<Item> &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<Item> &items) override;
   bool cmp_real();
   bool cmp_int();
@@ -7049,7 +7049,7 @@ public:
   int send_data(List<Item> &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 */

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

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)

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

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;
     }

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_... */
 

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	'<select expression> INTO <destination>;' is deprecated and will be removed in a future release. Please use 'SELECT <select list> INTO <destination> 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

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