Bug#31691060: Assertion `!is_open()' in Materialized_cursor::~Materialized_cursor

Bug#31781145: Sig 11 in setup_tmp_table_handler at sql/sql_tmp_table.CC:2067

The problem in both these bug reports is bad handling of an error
during execution in cursor-based operations. This is manifested both
with cursors in stored procedures and with cursors associated with
prepared statements in the client API.
Furthermore, some opportunity for specific problems due to execution
error because of invalid metadata existed.

Changes in this patch:

sp_cursor::open():

- After mysql_open_cursor(), if execution failed, the cursor is
  closed (just in case, as it should already be closed), and the
  cursor pointer is unassigned.

mysql_open_cursor():

- The cursor is returned regardless of the execution completion:
  If execution is unsuccessful, a cursor may still have been created.

- When a cursor is created and execution is successful, the cursor
  is opened (as before).

- When a cursor is created and execution fails, the cursor is closed
  but it is not deleted.

Prepared_statement::close_cursor() and ~Prepared_statement():

- cursor->close() is always called since it is state-aware.
  Note also that the is_open() result is ambiguous: There are actually
  two potential "open" states for a cursor: 1) the cursor has been
  populated with a result, and 2) the cursor has been opened against
  that result and can be read from. In the latter case, the table
  access must be closed. In both cases, the temporary table holding
  the result must be deleted.

Prepared_statement::swap_prepared_statement():

- The members result and cursor are also swapped, since they are
  associated with the statement. Without this, repreparation may cause
  access to deleted memory.

Prepared_statement::execute():

- After an error from mysql_open_cursor(), the result object was
  destroyed unconditionally. Now it is destroyed only if it is not
  referenced by a cursor.

Several tests have been added:

- A test that creates a stored procedure with a cursor, for which the
  SQL statement is causing an execution error. Multiple calls are made
  to the procedure, to ensure that subsequent executions are not affected
  by prior errors. FLUSH TABLES is performed between executions to
  ensure recovery from error due to metadata  invalidation.

- A test that creates a stored procedure with a cursor that is invoked
  multiple times within the procedure, with FLUSH TABLES performed
  between executions.

- A client test that creates a cursor that executes a prepared
  statement with an associated cursor. It is verified that errors
  in statement execution is handled correctly.

- A client test that creates a cursor for a SHOW PRIVILEGES statement.
  When prepared, this statement does not create a cursor, so the client
  library simulates a cursor. It is also demonstrated that the statement
  can be executed repeatedly.

Reviewed-by: Knut Anders Hatlen <knut.hatlen@oracle.com>

Author: roylyseng

mysql-test/r/sp.result

@@ -8465,3 +8465,79 @@ IF (COUNT(*) > 0, 'affected', 'not affected')
 not affected
 DROP FUNCTION ReturnFalse;
 DROP TABLE t1;
+# Bug#31691060: Assertion `!is_open()' in Materialized_cursor::~Materialized_cursor
+CREATE TABLE t1(a INTEGER);
+CREATE TABLE t2(a INTEGER, b INTEGER);
+INSERT INTO t1 VALUES(0), (1), (2);
+INSERT INTO t2 VALUES(1, 10), (2, 20), (2, 21);
+CREATE PROCEDURE pc(val INTEGER)
+BEGIN
+DECLARE finished, col_a, col_b INTEGER DEFAULT 0;
+DECLARE c CURSOR FOR
+SELECT a, (SELECT b FROM t2 WHERE t1.a=t2.a) FROM t1 WHERE a = val;
+DECLARE CONTINUE HANDLER FOR NOT FOUND SET finished = 1;
+SET finished = 0;
+OPEN c;
+loop1: LOOP
+FETCH c INTO col_a, col_b;
+IF finished = 1 THEN
+LEAVE loop1;
+END IF;
+END LOOP loop1;
+CLOSE c;
+END //
+CREATE PROCEDURE pc_with_flush()
+BEGIN
+DECLARE finished, col_a, col_b INTEGER DEFAULT 0;
+DECLARE val INTEGER DEFAULT 0;
+DECLARE c CURSOR FOR
+SELECT a, (SELECT b FROM t2 WHERE t1.a=t2.a) FROM t1 WHERE a = val;
+DECLARE CONTINUE HANDLER FOR NOT FOUND SET finished = 1;
+SET finished = 0;
+OPEN c;
+loop1: LOOP
+FETCH c INTO col_a, col_b;
+IF finished = 1 THEN
+LEAVE loop1;
+END IF;
+END LOOP loop1;
+CLOSE c;
+FLUSH TABLES;
+SET finished = 0;
+SET val = 1;
+OPEN c;
+loop2: LOOP
+FETCH c INTO col_a, col_b;
+IF finished = 1 THEN
+LEAVE loop2;
+END IF;
+END LOOP loop2;
+CLOSE c;
+FLUSH TABLES;
+SET val = 2;
+SET finished = 0;
+OPEN c;
+loop3: LOOP
+FETCH c INTO col_a, col_b;
+IF finished = 1 THEN
+LEAVE loop3;
+END IF;
+END LOOP loop3;
+CLOSE c;
+END //
+CALL pc(1);
+CALL pc(2);
+ERROR 21000: Subquery returns more than 1 row
+FLUSH TABLES;
+CALL pc(1);
+CALL pc(2);
+ERROR 21000: Subquery returns more than 1 row
+FLUSH TABLES;
+CALL pc(2);
+ERROR 21000: Subquery returns more than 1 row
+CALL pc(1);
+CALL pc_with_flush();
+ERROR 21000: Subquery returns more than 1 row
+DROP PROCEDURE pc;
+DROP PROCEDURE pc_with_flush;
+DROP TABLE t1, t2;

mysql-test/t/sp.test

@@ -9869,3 +9869,83 @@ SELECT IF (COUNT(*) > 0, 'affected', 'not affected') FROM t1
 
 DROP FUNCTION ReturnFalse;
 DROP TABLE t1;
+
+--echo # Bug#31691060: Assertion `!is_open()' in Materialized_cursor::~Materialized_cursor
+
+CREATE TABLE t1(a INTEGER);
+CREATE TABLE t2(a INTEGER, b INTEGER);
+INSERT INTO t1 VALUES(0), (1), (2);
+INSERT INTO t2 VALUES(1, 10), (2, 20), (2, 21);
+DELIMITER //;
+CREATE PROCEDURE pc(val INTEGER)
+BEGIN
+  DECLARE finished, col_a, col_b INTEGER DEFAULT 0;
+  DECLARE c CURSOR FOR
+      SELECT a, (SELECT b FROM t2 WHERE t1.a=t2.a) FROM t1 WHERE a = val;
+  DECLARE CONTINUE HANDLER FOR NOT FOUND SET finished = 1;
+  SET finished = 0;
+  OPEN c;
+  loop1: LOOP
+    FETCH c INTO col_a, col_b;
+    IF finished = 1 THEN
+      LEAVE loop1;
+    END IF;
+  END LOOP loop1;
+  CLOSE c;
+END //
+CREATE PROCEDURE pc_with_flush()
+BEGIN
+  DECLARE finished, col_a, col_b INTEGER DEFAULT 0;
+  DECLARE val INTEGER DEFAULT 0;
+  DECLARE c CURSOR FOR
+      SELECT a, (SELECT b FROM t2 WHERE t1.a=t2.a) FROM t1 WHERE a = val;
+  DECLARE CONTINUE HANDLER FOR NOT FOUND SET finished = 1;
+  SET finished = 0;
+  OPEN c;
+  loop1: LOOP
+    FETCH c INTO col_a, col_b;
+    IF finished = 1 THEN
+      LEAVE loop1;
+    END IF;
+  END LOOP loop1;
+  CLOSE c;
+  FLUSH TABLES;
+  SET finished = 0;
+  SET val = 1;
+  OPEN c;
+  loop2: LOOP
+    FETCH c INTO col_a, col_b;
+    IF finished = 1 THEN
+      LEAVE loop2;
+    END IF;
+  END LOOP loop2;
+  CLOSE c;
+  FLUSH TABLES;
+  SET val = 2;
+  SET finished = 0;
+  OPEN c;
+  loop3: LOOP
+    FETCH c INTO col_a, col_b;
+    IF finished = 1 THEN
+      LEAVE loop3;
+    END IF;
+  END LOOP loop3;
+  CLOSE c;
+END //
+DELIMITER ;//
+CALL pc(1);
+--error ER_SUBQUERY_NO_1_ROW
+CALL pc(2);
+FLUSH TABLES;
+CALL pc(1);
+--error ER_SUBQUERY_NO_1_ROW
+CALL pc(2);
+FLUSH TABLES;
+--error ER_SUBQUERY_NO_1_ROW
+CALL pc(2);
+CALL pc(1);
+--error ER_SUBQUERY_NO_1_ROW
+CALL pc_with_flush();
+DROP PROCEDURE pc;
+DROP PROCEDURE pc_with_flush;
+DROP TABLE t1, t2;

sql/sp_rcontext.cc

@@ -458,7 +458,14 @@ bool sp_cursor::open(THD *thd) {
     return true;
   }
 
-  return mysql_open_cursor(thd, &m_result, &m_server_side_cursor);
+  bool rc = mysql_open_cursor(thd, &m_result, &m_server_side_cursor);
+
+  // If execution failed, ensure that the cursor is closed.
+  if (rc && m_server_side_cursor != nullptr) {
+    m_server_side_cursor->close();
+    m_server_side_cursor = nullptr;
+  }
+  return rc;
 }
 
 bool sp_cursor::close() {

sql/sql_cursor.cc

@@ -138,9 +138,11 @@ class Query_result_materialize final : public Query_result_union {
   @param      thd           thread handle
   @param[in]  result        result class of the caller used as a destination
                             for the rows fetched from the cursor
-  @param[in,out] pcursor    a pointer to store a pointer to cursor in
-                            If non-NULL on entry, use the supplied cursor.
-                            Must be NULL on first invocation.
+  @param[in,out] pcursor    a pointer to store a pointer to cursor in.
+                            The cursor is usually created on first call.
+                            Notice that a cursor may be returned even though
+                            execution causes an error. Cursor is open
+                            when execution is successful, closed otherwise.
 
   @return Error status
 
@@ -234,47 +236,41 @@ bool mysql_open_cursor(THD *thd, Query_result *result,
   DEBUG_SYNC(thd, "after_table_close");
   thd->m_statement_psi = parent_locker;
 
-  /*
-    Possible options here:
-    - a materialized cursor is open. In this case rc is 0 and
-      result_materialize->materialized is not NULL
-    - an error occurred during materialization.
-      result_materialize->materialized_cursor is not NULL, but rc != 0
-    - successful completion of mysql_execute_command without
-      a cursor: rc is 0, result_materialize->materialized_cursor is NULL.
-      This is possible if some command writes directly to the
-      network, bypassing Query_result mechanism. An example of
-      such command is SHOW VARIABLES or SHOW STATUS.
-  */
+  Materialized_cursor *materialized_cursor =
+      result_materialize != nullptr ? result_materialize->materialized_cursor
+                                    : nullptr;
+
+  if (*pcursor == nullptr) *pcursor = materialized_cursor;
+
   if (rc) {
-    if (result_materialize->materialized_cursor) {
-      /* Rollback metadata in the client-server protocol. */
+    /*
+      Execution ended in error. Notice that a cursor may have been
+      created, in this case metadata in client-server protocol is rolled
+      back and the cursor is closed (if it is open).
+    */
+    if (materialized_cursor != nullptr) {
       result_materialize->abort_result_set(thd);
-
-      delete result_materialize->materialized_cursor;
-      result_materialize->materialized_cursor = nullptr;
+      materialized_cursor->close();
     }
-
     return true;
   }
 
-  if (result_materialize != nullptr &&
-      result_materialize->materialized_cursor) {
-    Materialized_cursor *materialized_cursor =
-        result_materialize->materialized_cursor;
-
+  /*
+    Execution was successful. For most queries, a cursor has been created
+    and must be opened, however for some queries, no cursor is used.
+    This is possible if some command writes directly to the
+    network, bypassing Query_result mechanism. An example of
+    such command is SHOW PRIVILEGES.
+  */
+  if (materialized_cursor != nullptr) {
     /*
       NOTE: close_thread_tables() has been called in
       mysql_execute_command(), so all tables except from the cursor
       temporary table have been closed.
     */
-
     if (materialized_cursor->open(thd)) {
-      delete materialized_cursor;
       return true;
     }
-
-    if (*pcursor == nullptr) *pcursor = materialized_cursor;
   }
 
   return false;

sql/sql_prepare.cc

@@ -2288,7 +2288,7 @@ Prepared_statement::Prepared_statement(THD *thd_arg)
 
 void Prepared_statement::close_cursor() {
   if (cursor == nullptr) return;
-  if (cursor->is_open()) cursor->close();
+  cursor->close();
 }
 
 void Prepared_statement::setup_set_params() {
@@ -2347,7 +2347,7 @@ Prepared_statement::~Prepared_statement() {
   DBUG_TRACE;
   DBUG_PRINT("enter", ("stmt: %p  cursor: %p", this, cursor));
   if (cursor != nullptr) {
-    if (cursor->is_open()) close_cursor();
+    close_cursor();
     if (result != nullptr) destroy(result);
     cursor = nullptr;
     result = nullptr;
@@ -3277,6 +3277,10 @@ void Prepared_statement::swap_prepared_statement(Prepared_statement *copy) {
   std::swap(m_name, copy->m_name);
   /* Ditto */
   std::swap(m_db, copy->m_db);
+  // Need a new cursor-specific query result after repreparation
+  std::swap(result, copy->result);
+  // Need a new cursor, if requested
+  std::swap(cursor, copy->cursor);
 
   DBUG_ASSERT(thd == copy->thd);
 }
@@ -3439,9 +3443,11 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) {
       if (!result) {
         error = true;  // OOM
       } else if ((error = mysql_open_cursor(thd, result, &cursor))) {
-        // cursor is freed inside mysql_open_cursor
-        destroy(result);
-        result = nullptr;
+        // Destroy result if cursor was never created
+        if (cursor == nullptr) {
+          destroy(result);
+          result = nullptr;
+        }
       } else {
         lex->cleanup(thd, true);
       }
@@ -3502,9 +3508,6 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) {
   if (cur_db_changed)
     mysql_change_db(thd, to_lex_cstring(saved_cur_db_name), true);
 
-  // Assert that if an error, the cursor and the result are deallocated.
-  DBUG_ASSERT(!error || (cursor == nullptr && result == nullptr));
-
   cleanup_stmt();
 
   /*