Bug#36980896: Defining local variables within a TRIGGER/PROCEDURE

roylyseng · dahlerlend · commit 9c6fe1643740 · 2024-09-14T18:48:10.000+02:00
causes service to shut down

There are two problems here: With a production binary, we see failures
in check_column_privileges() or failing to obtain a virtual function
for an Item_func_eq object.

With a debug binary, execution fails earlier in an assert:
Query_expression::set_prepared(), Assertion `!is_prepared()' failed.

Due to the assert failing in debug mode, it is difficult to get to the
actual source of the problem, but we can narrow the debug issue down
to establishing a proper execution context for an independent item
to be evaluated in function sp_prepare_func_item(). The assumption
for this function is that the current LEX and query expression found
through thd-&gt;lex can be set to "prepared" and "executing" state.

However, sp_prepare_func_item() can be used for two kinds of items.
Some of these are standalone and have an associated LEX object and
a query expression object. Examples of these are the procedure
instructions sp_instr_set and sp_instr_jump_if_not. Others are
expressions that are created below a LEX that is already in an
executing state. Examples are items used to assign values to procedure
arguments after procedure execution, and assignment to local variables
after query execution. The latter is the problem in this case.

The solution to the first problem is to distinguish the two cases and
pass a bool argument for the standalone items and false for the
remaining ones. In sp_prepare_func_item(), we then ensure that
preparation and execution state is set properly for the standalone
items, whereas we ensure by inspecting the current LEX object that we
are already in an executing context for the dependent items.

The second problem is related to the first one. It started appearing
after the fix for bug#35856247 was pushed, which changes the way
base_ref_items are saved in Query_block::save_properties().
But the solution also fixes this problem, since we no longer save
properties for dependent items, which will interfere with the already
saved properties for the current query block.

We also had to set proper execution state in
Sql_cmd_get_diagnostics::execute() and Sql_cmd_load_table::execute()
because otherwise these functions would fail the newly added asserts.

Change-Id: Ib5d31c07994340006818e180eed74e1d6aa23280
diff --git a/sql/item.cc b/sql/item.cc
@@ -1965,7 +1965,7 @@ void Item_splocal::print(const THD *thd, String *str, enum_query_type) const {
 }
 
 bool Item_splocal::set_value(THD *thd, sp_rcontext *ctx, Item **it) {
-  return ctx->set_variable(thd, get_var_idx(), it);
+  return ctx->set_variable(thd, false, get_var_idx(), it);
 }
 
 /*****************************************************************************
@@ -9297,8 +9297,8 @@ bool Item_trigger_field::eq(const Item *item) const {
              down_cast<const Item_trigger_field *>(item)->field_name);
 }
 
-bool Item_trigger_field::set_value(THD *thd, sp_rcontext * /*ctx*/, Item **it) {
-  Item *item = sp_prepare_func_item(thd, it);
+bool Item_trigger_field::set_value(THD *thd, sp_rcontext *, Item **it) {
+  Item *item = sp_prepare_func_item(thd, true, it);
   if (item == nullptr) return true;
 
   if (!fixed) {
diff --git a/sql/query_result.cc b/sql/query_result.cc
@@ -724,7 +724,8 @@ bool Query_dumpvar::send_data(THD *thd, const mem_root_deque<Item *> &items) {
   while ((mv = var_li++) && it != VisibleFields(items).end()) {
     Item *item = *it++;
     if (mv->is_local()) {
-      if (thd->sp_runtime_ctx->set_variable(thd, mv->get_offset(), &item))
+      if (thd->sp_runtime_ctx->set_variable(thd, false, mv->get_offset(),
+                                            &item))
         return true;
     } else {
       Item_func_set_user_var *suv = new Item_func_set_user_var(mv->name, item);
diff --git a/sql/server_component/mysql_stored_program_imp.cc b/sql/server_component/mysql_stored_program_imp.cc
@@ -331,14 +331,14 @@ auto static set_variable(stored_program_runtime_context sp_runtime_context,
   if (index < 0) return MYSQL_FAILURE;
   auto runtime_context = reinterpret_cast<sp_rcontext *>(sp_runtime_context);
   if (runtime_context == nullptr) runtime_context = current_thd->sp_runtime_ctx;
-  return runtime_context->set_variable(current_thd, index, &item);
+  return runtime_context->set_variable(current_thd, false, index, &item);
 }
 
 auto static set_return_value(stored_program_runtime_context sp_runtime_context,
                              Item *item) -> int {
   auto runtime_context = reinterpret_cast<sp_rcontext *>(sp_runtime_context);
   if (runtime_context == nullptr) runtime_context = current_thd->sp_runtime_ctx;
-  return runtime_context->set_return_value(current_thd, &item);
+  return runtime_context->set_return_value(current_thd, false, &item);
 }
 
 auto static get_item(stored_program_runtime_context sp_runtime_context,
diff --git a/sql/sp.cc b/sql/sp.cc
@@ -2470,19 +2470,28 @@ bool sp_check_name(LEX_STRING *ident) {
 /**
   Prepare an Item for evaluation (call of fix_fields).
 
-  @param thd       thread handler
-  @param it_addr   pointer on item reference
-
-  @retval
-    NULL      error
-  @retval
-    non-NULL  prepared item
+  @param thd        thread handler
+  @param standalone if true, thd->lex is directly associated with item.
+                    Preparation and execution state will be changed and
+                    preparation data will be saved for later executions.
+                    If false, item is part of a larger query expression and
+                    no such state transitions should take place.
+                    It also means that such items cannot save preparation data.
+  @param it_addr    pointer to item reference
+
+  @returns pointer to prepared Item on success, NULL on error.
 */
-Item *sp_prepare_func_item(THD *thd, Item **it_addr) {
+Item *sp_prepare_func_item(THD *thd, bool standalone, Item **it_addr) {
+  LEX *const lex = standalone ? thd->lex : nullptr;
+
+  // If item is part of larger query expression, it must be in executing state:
+  assert(lex != nullptr ||
+         (thd->lex->unit->is_prepared() && thd->lex->is_exec_started()));
+
   it_addr = (*it_addr)->this_item_addr(thd, it_addr);
 
   if ((*it_addr)->fixed) {
-    thd->lex->set_exec_started();
+    if (lex != nullptr) lex->set_exec_started();
     return *it_addr;
   }
 
@@ -2493,37 +2502,40 @@ Item *sp_prepare_func_item(THD *thd, Item **it_addr) {
     DBUG_PRINT("info", ("fix_fields() failed"));
     return nullptr;
   }
-  thd->lex->unit->set_prepared();
-  thd->lex->save_cmd_properties(thd);
-  thd->lex->set_exec_started();
-
+  // If item is a separate query expression, set it's state to executing
+  if (lex != nullptr) {
+    lex->unit->set_prepared();
+    lex->save_cmd_properties(thd);
+    lex->set_exec_started();
+  }
   return *it_addr;
 }
 
 /**
   Evaluate an expression and store the result in the field.
 
-  @param thd                    current thread object
-  @param result_field           the field to store the result
-  @param expr_item_ptr          the root item of the expression
+  @param thd            current thread object
+  @param standalone     if true, thd->lex contains preparation and execution
+                        state of item, otherwise item is part of
+                        a query expression that is already in executing state.
+  @param result_field   the field to store the result
+  @param expr_item_ptr  the root item of the expression
 
-  @retval
-    false  on success
-  @retval
-    true   on error
+  @returns false on success, true on error
 */
-bool sp_eval_expr(THD *thd, Field *result_field, Item **expr_item_ptr) {
-  Item *expr_item;
+bool sp_eval_expr(THD *thd, bool standalone, Field *result_field,
+                  Item **expr_item_ptr) {
   Strict_error_handler strict_handler(
       Strict_error_handler::ENABLE_SET_SELECT_STRICT_ERROR_HANDLER);
   const enum_check_fields save_check_for_truncated_fields =
       thd->check_for_truncated_fields;
   const unsigned int stmt_unsafe_rollback_flags =
       thd->get_transaction()->get_unsafe_rollback_flags(Transaction_ctx::STMT);
 
-  if (!*expr_item_ptr) goto error;
+  assert(*expr_item_ptr != nullptr);
 
-  if (!(expr_item = sp_prepare_func_item(thd, expr_item_ptr))) goto error;
+  Item *expr_item = sp_prepare_func_item(thd, standalone, expr_item_ptr);
+  if (expr_item == nullptr) goto error;
 
   /*
     Set THD flags to emit warnings/errors in case of overflow/type errors
diff --git a/sql/sp.h b/sql/sp.h
@@ -407,9 +407,10 @@ bool sp_check_name(LEX_STRING *ident);
 Table_ref *sp_add_to_query_tables(THD *thd, LEX *lex, const char *db,
                                   const char *name);
 
-Item *sp_prepare_func_item(THD *thd, Item **it_addr);
+Item *sp_prepare_func_item(THD *thd, bool standalone, Item **it_addr);
 
-bool sp_eval_expr(THD *thd, Field *result_field, Item **expr_item_ptr);
+bool sp_eval_expr(THD *thd, bool standalone, Field *result_field,
+                  Item **expr_item_ptr);
 
 String *sp_get_item_value(THD *thd, Item *item, String *str);
 
diff --git a/sql/sp_head.cc b/sql/sp_head.cc
@@ -2799,8 +2799,8 @@ bool sp_head::execute_function(THD *thd, Item **argp, uint argcount,
     /* Arguments must be fixed in Item_func_sp::fix_fields */
     assert(argp[arg_no]->fixed);
 
-    err_status = func_runtime_ctx->set_variable(thd, arg_no, &(argp[arg_no]));
-
+    err_status =
+        func_runtime_ctx->set_variable(thd, false, arg_no, &(argp[arg_no]));
     if (err_status) goto err_with_cleanup;
   }
 
@@ -2983,11 +2983,12 @@ bool sp_head::execute_procedure(THD *thd, mem_root_deque<Item *> *args) {
   sp_rcontext *proc_runtime_ctx =
       sp_rcontext::create(thd, m_root_parsing_ctx, nullptr);
 
-  if (!proc_runtime_ctx) {
+  if (proc_runtime_ctx == nullptr) {
     thd->sp_runtime_ctx = sp_runtime_ctx_saved;
 
-    if (sp_runtime_ctx_saved != nullptr) ::destroy_at(parent_sp_runtime_ctx);
-
+    if (sp_runtime_ctx_saved != nullptr) {
+      ::destroy_at(parent_sp_runtime_ctx);
+    }
     return true;
   }
 
@@ -3000,33 +3001,33 @@ bool sp_head::execute_procedure(THD *thd, mem_root_deque<Item *> *args) {
 
     for (uint i = 0; i < params; ++i, ++it_args) {
       Item *arg_item = *it_args;
-      if (!arg_item) break;
+      if (arg_item == nullptr) break;
 
       sp_variable *spvar = m_root_parsing_ctx->find_variable(i);
-
-      if (!spvar) continue;
+      if (spvar == nullptr) continue;
 
       if (spvar->mode != sp_variable::MODE_IN) {
         Settable_routine_parameter *srp =
             arg_item->get_settable_routine_parameter();
-
-        if (!srp) {
+        if (srp == nullptr) {
           my_error(ER_SP_NOT_VAR_ARG, MYF(0), i + 1, m_qname.str);
           err_status = true;
           break;
         }
       }
 
       if (spvar->mode == sp_variable::MODE_OUT) {
-        Item_null *null_item = new Item_null();
-
-        if (!null_item ||
-            proc_runtime_ctx->set_variable(thd, i, (Item **)&null_item)) {
+        Item *null_item = new Item_null();
+        if (null_item == nullptr) {
+          err_status = true;
+          break;
+        }
+        if (proc_runtime_ctx->set_variable(thd, false, i, &null_item)) {
           err_status = true;
           break;
         }
       } else {
-        if (proc_runtime_ctx->set_variable(thd, i, &*it_args)) {
+        if (proc_runtime_ctx->set_variable(thd, false, i, &*it_args)) {
           err_status = true;
           break;
         }
diff --git a/sql/sp_instr.cc b/sql/sp_instr.cc
@@ -1123,12 +1123,13 @@ PSI_statement_info sp_instr_set::psi_info = {
 bool sp_instr_set::exec_core(THD *thd, uint *nextp) {
   *nextp = get_ip() + 1;
 
-  if (!thd->sp_runtime_ctx->set_variable(thd, m_offset, &m_value_item))
+  // LEX of instruction keeps execution state of the assignment operation
+  if (!thd->sp_runtime_ctx->set_variable(thd, true, m_offset, &m_value_item))
     return false;
 
   /* Failed to evaluate the value. Reset the variable to NULL. */
 
-  if (thd->sp_runtime_ctx->set_variable(thd, m_offset, nullptr)) {
+  if (thd->sp_runtime_ctx->set_variable(thd, true, m_offset, nullptr)) {
     /* If this also failed, let's abort. */
     my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));
   }
@@ -1275,11 +1276,11 @@ PSI_statement_info sp_instr_jump_if_not::psi_info = {
 #endif
 
 bool sp_instr_jump_if_not::exec_core(THD *thd, uint *nextp) {
-  assert(m_expr_item);
+  assert(m_expr_item != nullptr);
 
-  Item *item = sp_prepare_func_item(thd, &m_expr_item);
-
-  if (!item) return true;
+  // LEX of instruction keeps execution state of the expression evaluation
+  Item *item = sp_prepare_func_item(thd, true, &m_expr_item);
+  if (item == nullptr) return true;
 
   *nextp = item->val_bool() ? get_ip() + 1 : m_dest;
 
@@ -1358,11 +1359,11 @@ PSI_statement_info sp_instr_jump_case_when::psi_info = {
 #endif
 
 bool sp_instr_jump_case_when::exec_core(THD *thd, uint *nextp) {
-  assert(m_eq_item);
-
-  Item *item = sp_prepare_func_item(thd, &m_eq_item);
+  assert(m_eq_item != nullptr);
 
-  if (!item) return true;
+  // LEX of instruction keeps execution state of the case expression
+  Item *item = sp_prepare_func_item(thd, true, &m_eq_item);
+  if (item == nullptr) return true;
 
   *nextp = item->val_bool() ? get_ip() + 1 : m_dest;
 
@@ -1445,7 +1446,7 @@ bool sp_instr_freturn::exec_core(THD *thd, uint *nextp) {
     do it in scope of execution the current context/block.
   */
 
-  return thd->sp_runtime_ctx->set_return_value(thd, &m_expr_item);
+  return thd->sp_runtime_ctx->set_return_value(thd, true, &m_expr_item);
 }
 
 void sp_instr_freturn::print(const THD *thd, String *str) {
@@ -1866,21 +1867,21 @@ bool sp_instr_set_case_expr::exec_core(THD *thd, uint *nextp) {
 
   sp_rcontext *rctx = thd->sp_runtime_ctx;
 
-  if (rctx->set_case_expr(thd, m_case_expr_id, &m_expr_item)) {
+  // LEX of instruction keeps execution state of the case expression
+  if (rctx->set_case_expr(thd, true, m_case_expr_id, &m_expr_item)) {
     if (!rctx->get_case_expr(m_case_expr_id)) {
       // Failed to evaluate the value, the case expression is still not
       // initialized. Set to NULL so we can continue.
       Item *null_item = new Item_null();
 
-      if (!null_item || rctx->set_case_expr(thd, m_case_expr_id, &null_item)) {
+      if (null_item == nullptr ||
+          rctx->set_case_expr(thd, true, m_case_expr_id, &null_item)) {
         // If this also failed, we have to abort.
         my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));
       }
     }
-
     return true;
   }
-
   return false;
 }
 
diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc
@@ -146,12 +146,13 @@ bool sp_rcontext::init_var_items(THD *thd) {
   return false;
 }
 
-bool sp_rcontext::set_return_value(THD *thd, Item **return_value_item) {
+bool sp_rcontext::set_return_value(THD *thd, bool standalone,
+                                   Item **return_value_item) {
   assert(m_return_value_fld);
 
   m_return_value_set = true;
 
-  return sp_eval_expr(thd, m_return_value_fld, return_value_item);
+  return sp_eval_expr(thd, standalone, m_return_value_fld, return_value_item);
 }
 
 bool sp_rcontext::push_cursor(sp_instr_cpush *i) {
@@ -402,13 +403,13 @@ bool sp_rcontext::handle_sql_condition(THD *thd, uint *ip,
   return true;
 }
 
-bool sp_rcontext::set_variable(THD *thd, Field *field, Item **value) {
-  if (!value) {
+bool sp_rcontext::set_variable(THD *thd, bool standalone, Field *field,
+                               Item **value) {
+  if (value == nullptr) {
     field->set_null();
     return false;
   }
-
-  return sp_eval_expr(thd, field, value);
+  return sp_eval_expr(thd, standalone, field, value);
 }
 
 Item_cache *sp_rcontext::create_case_expr_holder(THD *thd,
@@ -425,10 +426,11 @@ Item_cache *sp_rcontext::create_case_expr_holder(THD *thd,
   return holder;
 }
 
-bool sp_rcontext::set_case_expr(THD *thd, int case_expr_id,
+bool sp_rcontext::set_case_expr(THD *thd, bool standalone, int case_expr_id,
                                 Item **case_expr_item_ptr) {
-  Item *case_expr_item = sp_prepare_func_item(thd, case_expr_item_ptr);
-  if (!case_expr_item) return true;
+  Item *case_expr_item =
+      sp_prepare_func_item(thd, standalone, case_expr_item_ptr);
+  if (case_expr_item == nullptr) return true;
 
   if (!m_case_expr_holders[case_expr_id] ||
       m_case_expr_holders[case_expr_id]->result_type() !=
@@ -547,7 +549,7 @@ bool sp_cursor::Query_fetch_into_spvars::send_data(
   */
   while ((spvar = spvar_iter++)) {
     Item *item = *item_iter++;
-    if (thd->sp_runtime_ctx->set_variable(thd, spvar->offset, &item))
+    if (thd->sp_runtime_ctx->set_variable(thd, false, spvar->offset, &item))
       return true;
   }
   return false;
diff --git a/sql/sp_rcontext.h b/sql/sp_rcontext.h
diff --git a/sql/sql_get_diagnostics.cc b/sql/sql_get_diagnostics.cc
diff --git a/sql/sql_load.cc b/sql/sql_load.cc

Original file line number	Diff line number	Diff line change
`@@ -1965,7 +1965,7 @@ void Item_splocal::print(const THD thd, String str, enum_query_type) const {`
`1965`	`1965`	`}`
`1966`	`1966`
`1967`	`1967`	`bool Item_splocal::set_value(THD thd, sp_rcontext ctx, Item **it) {`
`1968`		`- return ctx->set_variable(thd, get_var_idx(), it);`
	`1968`	`+ return ctx->set_variable(thd, false, get_var_idx(), it);`
`1969`	`1969`	`}`
`1970`	`1970`
`1971`	`1971`	`/*****************************************************************************`
`@@ -9297,8 +9297,8 @@ bool Item_trigger_field::eq(const Item *item) const {`
`9297`	`9297`	`down_cast<const Item_trigger_field *>(item)->field_name);`
`9298`	`9298`	`}`
`9299`	`9299`
`9300`		`-bool Item_trigger_field::set_value(THD thd, sp_rcontext /ctx/, Item **it) {`
`9301`		`- Item *item = sp_prepare_func_item(thd, it);`
	`9300`	`+bool Item_trigger_field::set_value(THD thd, sp_rcontext , Item **it) {`
	`9301`	`+ Item *item = sp_prepare_func_item(thd, true, it);`
`9302`	`9302`	`if (item == nullptr) return true;`
`9303`	`9303`
`9304`	`9304`	`if (!fixed) {`