maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 12/27/2017 08:33 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Dec 26, Alexander Barkov wrote:
> > Hi Sergei,
> >
> > can you please review a patch for MDEV-14603?
>
> I agree with the fix.
>
> But I don't like that there are many things to backup/restore
> (Statement, arena, free_list, and now change_list), they're all
> saved/restored in different places - it's easy to miss something when
> making changes.
>
> Would it be possible to move all that saving/restoring into dedicated
> helpers and use them in all three places (prepare, execute, execute
> immediate)?

Thanks for a good idea.

Done. Please find attached.

By the way, it seems this approach with a helper class is fixing one 
more potential problem.

Details:

Before the patch mysql_sql_stmt_prepare()
did not backup/restore change_list, but also it did not
backup/restore free_list.

Only mysql_sql_stmt_execute() and mysql_sql_stmt_execute_immediate()
preserved/restored free_list.

I think this was wrong.

I could not invent a test case to get some
unexpected behavior because of free_list
not preserved/restored in mysql_sql_stmt_prepare().
But logically it looked not correct for me.

In a query like this:

PREPARE stmt FROM CONCAT('SELECT 1');

Item_func_concat and Item_string('SELECT 1')
were deleted with this call stack:

#0  Item::delete_self
#1  Query_arena::free_items
#2  THD::cleanup_after_query
#3  Prepared_statement::cleanup_stmt
#4  Prepared_statement::prepare ("SELECT 1")

So it was an impression that those items belonged to the "SELECT" query.
But in fact they belong to the "PREPARE" query.


After this patch Item_func_concat and Item_string are deleted
with this stack:

#0  Item::delete_self
#1  0x0000555555abf330 in Query_arena::free_items
#2  0x0000555555aba9ab in THD::cleanup_after_query
#3  0x0000555555b104fe in mysql_parse (
    rawbuf=0x7fff580131e8 "PREPARE stmt FROM CONCAT('SELECT 1')",...

Which seems to be more correct.

Thanks!

diff --git a/mysql-test/r/ps.result b/mysql-test/r/ps.result
index 1958caf..96d4b3d 100644
--- a/mysql-test/r/ps.result
+++ b/mysql-test/r/ps.result
@@ -5121,3 +5121,45 @@ END;
 $$
 ERROR 22007: Incorrect datetime value: '10' for column 'a' at row 1
 DROP PROCEDURE p1;
+#
+# MDEV-14603 signal 11 with short stacktrace
+#
+SET NAMES utf8;
+CREATE TABLE t1(i INT);
+CREATE PROCEDURE p1(tn VARCHAR(32))
+EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn);
+CALL p1('t1');
+Table	Op	Msg_type	Msg_text
+test.t1	analyze	status	Table is already up to date
+DROP PROCEDURE p1;
+DROP TABLE t1;
+SET NAMES utf8;
+CREATE PROCEDURE p1()
+EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+CALL p1();
+DROP PROCEDURE p1;
+SET NAMES utf8;
+CREATE PROCEDURE p1()
+BEGIN
+PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+EXECUTE stmt;
+DEALLOCATE PREPARE stmt;
+END;
+$$
+CALL p1();
+DROP PROCEDURE p1;
+SET NAMES utf8;
+CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1));
+CALL p1('x');
+DROP PROCEDURE p1;
+SET NAMES utf8;
+CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+BEGIN
+PREPARE stmt FROM 'SELECT ?';
+EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1));
+DEALLOCATE PREPARE stmt;
+END;
+$$
+CALL p1('x');
+DROP PROCEDURE p1;
diff --git a/mysql-test/t/ps.test b/mysql-test/t/ps.test
index 327a94c..565f831 100644
--- a/mysql-test/t/ps.test
+++ b/mysql-test/t/ps.test
@@ -4578,3 +4578,60 @@ END;
 $$
 DELIMITER ;$$
 DROP PROCEDURE p1;
+
+--echo #
+--echo # MDEV-14603 signal 11 with short stacktrace
+--echo #
+
+SET NAMES utf8;
+CREATE TABLE t1(i INT);
+CREATE PROCEDURE p1(tn VARCHAR(32))
+  EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn);
+CALL p1('t1');
+DROP PROCEDURE p1;
+DROP TABLE t1;
+
+SET NAMES utf8;
+CREATE PROCEDURE p1()
+  EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+--disable_result_log
+CALL p1();
+--enable_result_log
+DROP PROCEDURE p1;
+
+SET NAMES utf8;
+DELIMITER $$;
+CREATE PROCEDURE p1()
+BEGIN
+  PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+  EXECUTE stmt;
+  DEALLOCATE PREPARE stmt;
+END;
+$$
+DELIMITER ;$$
+--disable_result_log
+CALL p1();
+--enable_result_log
+DROP PROCEDURE p1;
+
+SET NAMES utf8;
+CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+  EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1));
+--disable_result_log
+CALL p1('x');
+--enable_result_log
+DROP PROCEDURE p1;
+
+SET NAMES utf8;
+DELIMITER $$;
+CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+BEGIN
+  PREPARE stmt FROM 'SELECT ?';
+  EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1));
+  DEALLOCATE PREPARE stmt;
+END;
+$$
+DELIMITER ;$$
+--disable_result_log
+CALL p1('x');
+DROP PROCEDURE p1;
diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
index 4e847fb..a850452 100644
--- a/sql/sql_prepare.cc
+++ b/sql/sql_prepare.cc
@@ -2760,6 +2760,32 @@ bool LEX::get_dynamic_sql_string(LEX_CSTRING *dst, String *buffer)
 }
 
 
+class PS_state_savepoint: public Item_change_list
+{
+  Item *free_list;
+public:
+  PS_state_savepoint(THD *thd)
+   :free_list(thd->free_list)
+  {
+    thd->free_list= NULL;
+    thd->move_elements_to(this);
+  }
+  void rollback(THD *thd)
+  {
+    thd->rollback_item_tree_changes();
+    move_elements_to(thd);
+    thd->free_items();
+    thd->free_list= free_list;
+    free_list= NULL;
+  }
+  ~PS_state_savepoint()
+  {
+    DBUG_ASSERT(free_list == NULL);
+    DBUG_ASSERT(Item_change_list::is_empty());
+  }
+};
+
+
 /**
   SQLCOM_PREPARE implementation.
 
@@ -2823,6 +2849,24 @@ void mysql_sql_stmt_prepare(THD *thd)
     DBUG_VOID_RETURN;
   }
 
+  /*
+    Make sure we call Prepared_statement::prepare() with empty
+    THD::free_list and THD::change_list. They can be non-empty as the above
+    LEX::get_dynamic_sql_string() calls fix_fields() for the Item
+    containing the PS source, e.g. on character set conversion:
+
+    SET NAMES utf8;
+    DELIMITER $$
+    CREATE PROCEDURE p1()
+    BEGIN
+      PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+      EXECUTE stmt;
+    END;
+    $$
+    DELIMITER ;
+    CALL p1();
+  */
+  PS_state_savepoint ps_state_savepoint(thd);
   if (stmt->prepare(query.str, (uint) query.length))
   {
     /* Statement map deletes the statement on erase */
@@ -2833,6 +2877,7 @@ void mysql_sql_stmt_prepare(THD *thd)
     SESSION_TRACKER_CHANGED(thd, SESSION_STATE_CHANGE_TRACKER, NULL);
     my_ok(thd, 0L, 0L, "Statement prepared");
   }
+  ps_state_savepoint.rollback(thd);
 
   DBUG_VOID_RETURN;
 }
@@ -2861,12 +2906,31 @@ void mysql_sql_stmt_execute_immediate(THD *thd)
       !(stmt= new Prepared_statement(thd)))
     DBUG_VOID_RETURN;                           // out of memory
 
-  // See comments on thd->free_list in mysql_sql_stmt_execute()
-  Item *free_list_backup= thd->free_list;
-  thd->free_list= NULL;
+  /*
+    Make sure we call Prepared_statement::execute_immediate()
+    with empty THD::free_list and THD::change_list.
+    It can be non empty as the above LEX::prepared_stmt_params_fix_fields()
+    and LEX::get_dynamic_str_string() call fix_fields() for the PS source
+    and PS parameter Items and can do Item tree changes,
+    e.g. on character set conversion:
+
+    - Example #1: Item tree changes in get_dynamic_str_string()
+    SET NAMES utf8;
+    CREATE PROCEDURE p1()
+      EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1));
+    CALL p1();
+
+    - Example #2: Item tree changes in prepared_stmt_param_fix_fields():
+    SET NAMES utf8;
+    CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+      EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1));
+    CALL p1('x');
+
+    See also comments on thd->free_list in mysql_sql_stmt_execute()
+  */
+  PS_state_savepoint ps_state_savepoint(thd);
   (void) stmt->execute_immediate(query.str, (uint) query.length);
-  thd->free_items();
-  thd->free_list= free_list_backup;
+  ps_state_savepoint.rollback(thd);
 
   stmt->lex->restore_set_statement_var();
   delete stmt;
@@ -3259,16 +3323,32 @@ void mysql_sql_stmt_execute(THD *thd)
     "SET STATEMENT" or "USING" parts of the query,
     so they don't get freed in case of re-prepare.
     See MDEV-10702 Crash in SET STATEMENT FOR EXECUTE
+
+    Also we make sure we call Prepared_statement::execute_loop() with an empty
+    THD::change_list. It can be non-empty because the above
+    LEX::prepared_stmt_params_fix_fields() calls fix_fields() for
+    the PS parameter Items and can do some Item tree changes,
+    e.g. on character set conversion:
+
+    SET NAMES utf8;
+    DELIMITER $$
+    CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8)
+    BEGIN
+      PREPARE stmt FROM 'SELECT ?';
+      EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1));
+    END;
+    $$
+    DELIMITER ;
+    CALL p1('x');
   */
-  Item *free_list_backup= thd->free_list;
-  thd->free_list= NULL; // Hide the external (e.g. "SET STATEMENT") Items
+  PS_state_savepoint ps_state_savepoint(thd);
   (void) stmt->execute_loop(&expanded_query, FALSE, NULL, NULL);
-  thd->free_items();    // Free items created by execute_loop()
   /*
-    Now restore the "external" (e.g. "SET STATEMENT") Item list.
+    Free items created by execute_loop() and
+    restore the "external" (e.g. "SET STATEMENT") Item list.
     It will be freed normaly in THD::cleanup_after_query().
   */
-  thd->free_list= free_list_backup;
+  ps_state_savepoint.rollback(thd);
 
   stmt->lex->restore_set_statement_var();
   DBUG_VOID_RETURN;
@@ -3931,6 +4011,7 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len)
     external changes when cleaning up after validation.
   */
   DBUG_ASSERT(thd->Item_change_list::is_empty());
+  DBUG_ASSERT(thd->free_list == NULL);
 
   /*
     Marker used to release metadata locks acquired while the prepared