MDEV-40125: OLD_VALUE crashes on a view with an expression by sanja-byelkin · Pull Request #5393 · MariaDB/server

sanja-byelkin · 2026-07-16T10:25:26Z

Analysis:

When OLD_VALUE() is used on a view, field resolution creates an Item_direct_view_ref instead of an Item_ref where real_item() points to the expression. But Item_old_field::fix_fields() continues assuming that a Field object was available, but view references do not have a field pointer, which resulted in a crash.

Fix:
Store the real referenced item in Item_old_field::expr and use it when the OLD_VALUE() reference does not have a Field object. This allows OLD_VALUE() to work with view fields and avoids dereferencing a NULL field pointer. Fix relevant methods accordingly.

Analysis: When OLD_VALUE() is used on a view, field resolution creates an Item_direct_view_ref instead of an Item_ref where real_item() points to the expression. But Item_old_field::fix_fields() continues assuming that a Field object was available, but view references do not have a field pointer, which resulted in a crash. Fix: Store the real referenced item in Item_old_field::expr and use it when the OLD_VALUE() reference does not have a Field object. This allows OLD_VALUE() to work with view fields and avoids dereferencing a NULL field pointer. Fix relevant methods accordingly.

CLAassistant · 2026-07-16T10:25:37Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

gemini-code-assist

Code Review

This pull request addresses MDEV-40125, fixing a crash when using OLD_VALUE on a view with an expression, by updating Item_old_field to support expressions and propagate old values. The review feedback identifies several critical issues and improvement opportunities: a potential stack buffer overflow and dead code in sql_base.cc due to an unused buffer populated with strncpy; potential crashes from pointer arithmetic on a null table->record[1] in Item_field::set_field; and a missing initialization of is_old_value_reference in one of the Item_old_field constructors. Additionally, the reviewer suggests avoiding the introduction of the get_correct_item() virtual method in the base Item class by directly overriding used_tables(), with_rownum_func(), and walk() in Item_old_field.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-16T10:28:54Z

          {
+            char buffer[128];
+            size_t len= (*ref)->name.length * sizeof((*ref)->name.str[0]);
+            strncpy(buffer, (*ref)->name.str, len);
+            if (it->type() == Item::FIELD_OLD_ITEM &&
+                ((Item_old_field*)(it))->expr)
+            {
+              it->get_correct_item()->set_name(thd, (*ref)->name);
+            }
            if (thd->column_usage == MARK_COLUMNS_READ)
-              it->walk(&Item::register_field_in_read_map, 0, 0);
+              it->get_correct_item()->walk(&Item::register_field_in_read_map, 0, 0);
            else
-              it->walk(&Item::register_field_in_write_map, 0, 0);
+              it->get_correct_item()->walk(&Item::register_field_in_write_map, 0, 0);
          }


The local buffer char buffer[128] is declared and populated using strncpy with an unbounded length len = (*ref)->name.length, which can lead to a stack buffer overflow if the identifier name is very long. Furthermore, buffer is completely unused in the rest of the function, making this dead code.

Additionally, we can avoid mutating the shared view expression's name by setting the name on the Item_old_field itself and overriding col_name in Send_field during make_send_field. This allows us to completely remove the get_correct_item() calls here.

{ if (it->type() == Item::FIELD_OLD_ITEM) { it->set_name(thd, (*ref)->name); } if (thd->column_usage == MARK_COLUMNS_READ) it->walk(&Item::register_field_in_read_map, 0, 0); else it->walk(&Item::register_field_in_write_map, 0, 0); }

gemini-code-assist · 2026-07-16T10:28:54Z

+  field->ptr_old= field_par->table->record[1] +
+                  field_par->offset(field->table->record[0]);
+  if (field_par->null_ptr)
+  {
+    field->null_ptr_old= field_par->table->record[1] +
+                         (field_par->null_ptr -
+                          field_par->table->record[0]);
+  }
+  else
+  {
+    field->null_ptr_old = nullptr;
+  }


Item_field::set_field is called for every column reference in every query (including read-only SELECT statements). However, table->record[1] is only allocated for tables opened for update/write operations. For read-only queries or certain storage engines/temporary tables, table->record[1] is nullptr. Performing pointer arithmetic on nullptr (e.g., nullptr + offset) is undefined behavior in C++, and swapping these invalid pointers in change_field_ptr will lead to crashes when the field is evaluated. We must guard this with a check for field_par->table->record[1].

if (field_par->table->record[1]) { field->ptr_old= field_par->table->record[1] + field_par->offset(field_par->table->record[0]); if (field_par->null_ptr) { field->null_ptr_old= field_par->table->record[1] + (field_par->null_ptr - field_par->table->record[0]); } else { field->null_ptr_old= nullptr; } } else { field->ptr_old= nullptr; field->null_ptr_old= nullptr; }

gemini-code-assist · 2026-07-16T10:28:54Z

+void Item_old_field::make_send_field(THD *thd, Send_field *tmp_field)
+{
+  if (field)
+    Item_field::make_send_field(thd, tmp_field);
+  else
+    expr->make_send_field(thd, tmp_field);
+}


Instead of mutating the shared view expression's name (which can have side effects on other parts of the query or subsequent executions), we can set the name on the Item_old_field itself and override the col_name in Send_field during make_send_field.

void Item_old_field::make_send_field(THD *thd, Send_field *tmp_field) { if (field) Item_field::make_send_field(thd, tmp_field); else { expr->make_send_field(thd, tmp_field); tmp_field->col_name= name; } }

gemini-code-assist · 2026-07-16T10:28:54Z

+  Item_old_field(THD *thd, Item* item= nullptr):Item_field(thd)
+  { expr= item; }


In this constructor, is_old_value_reference is not initialized to true, unlike all other constructors of Item_old_field. This can cause the item to not be recognized as an old value reference in some contexts.

Item_old_field(THD *thd, Item* item= nullptr):Item_field(thd) { is_old_value_reference= true; expr= item; }

gemini-code-assist · 2026-07-16T10:28:54Z

+  Item *get_correct_item() override { return expr ? expr : this; }
+  void make_send_field(THD *thd, Send_field *tmp_field) override;
+  const Type_handler *type_handler() const override
+  {
+    const Type_handler *handler= field ? field->type_handler() : expr->type_handler();
+    return handler->type_handler_for_item_field();
+  }
+  Item *expr;


Instead of introducing a new virtual method get_correct_item() to the base Item class and calling it in multiple places in sql_base.cc, we can override the relevant virtual methods (used_tables(), with_rownum_func(), and walk()) directly in Item_old_field. This keeps Item_old_field self-contained and avoids leaking its implementation details.

table_map used_tables() const override { return expr ? expr->used_tables() : Item_field::used_tables(); } bool with_rownum_func() const override { return expr ? expr->with_rownum_func() : Item_field::with_rownum_func(); } bool walk(Item_processor processor, bool walk_subquery, void *arg) override { if (expr && expr->walk(processor, walk_subquery, arg)) return true; return (this->*processor)(arg); } void make_send_field(THD *thd, Send_field *tmp_field) override; const Type_handler *type_handler() const override { const Type_handler *handler= field ? field->type_handler() : expr->type_handler(); return handler->type_handler_for_item_field(); } Item *expr;

gemini-code-assist · 2026-07-16T10:28:54Z

+  bool is_old_value_reference;
+  virtual Item *get_correct_item() { return this; }


If we override used_tables(), with_rownum_func(), and walk() in Item_old_field, the virtual method get_correct_item() is no longer needed in the base Item class and can be removed.

bool is_old_value_reference;

References

Keep the API of custom wrapper classes minimal by avoiding or removing unused methods.

gemini-code-assist · 2026-07-16T10:28:54Z

+    lex->current_select->select_list_tables|= item->get_correct_item()->used_tables();
+    lex->used_tables|= item->get_correct_item()->used_tables();
    lex->current_select->cur_pos_in_select_list++;
-    lex->current_select->rownum_in_field_list |= item->with_rownum_func();
+    lex->current_select->rownum_in_field_list |= item->get_correct_item()->with_rownum_func();


With used_tables() and with_rownum_func() overridden in Item_old_field, we can revert these lines back to their original form, eliminating the need for get_correct_item().

lex->current_select->select_list_tables|= item->used_tables(); lex->used_tables|= item->used_tables(); lex->current_select->cur_pos_in_select_list++; lex->current_select->rownum_in_field_list |= item->with_rownum_func();

Copilot

Pull request overview

This PR fixes a server crash when OLD_VALUE() is used against view columns that resolve to expressions (e.g. a*2 AS a2) by allowing Item_old_field to operate on the underlying referenced Item when no Field* is available (as is common with view references).

Changes:

Extend Item_old_field to keep a referenced expression item (expr) and use it for metadata/sending when field == NULL.
Update column marking / used_tables() accounting to operate on the “correct” underlying item when handling OLD_VALUE() on view expressions.
Add an MTR regression test for OLD_VALUE() on view expressions (MDEV-40125).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
sql/table.cc	Removes an extraneous whitespace line in `Field_iterator_view`.
sql/sql_base.cc	Updates `OLD_VALUE()` wrapping for view fields and adjusts column-usage marking to walk the underlying expression item.
sql/item.h	Adds `Item::get_correct_item()` and extends `Item_old_field` to carry an expression item and override metadata/type handling accordingly.
sql/item.cc	Implements expression-aware `Item_old_field::send()` and adds a `change_field_ptr` processor to swap in “old” row buffers for expression trees.
mysql-test/main/update.test	Adds a regression test covering `OLD_VALUE()` on a view expression column during `UPDATE ... RETURNING`.
mysql-test/main/update.result	Adds the expected output for the new regression test section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            char buffer[128];
+            size_t len= (*ref)->name.length * sizeof((*ref)->name.str[0]);
+            strncpy(buffer, (*ref)->name.str, len);


+  Item_old_field(THD *thd, Item* item= nullptr):Item_field(thd)
+  { expr= item; }


sanja-byelkin

Please fix my findings

sanja-byelkin · 2026-07-16T10:28:39Z

          if (it->type() == Item::FIELD_ITEM)
            field_to_set= ((Item_field*)it)->field;
          else
          {
+            char buffer[128];


again magic numbers! where 128 came from?

NAME_LEN=256 so it is way to memory overrun. Please check maximum name length (if my guess is correct and fix it)

sanja-byelkin · 2026-07-16T10:29:32Z

          if (it->type() == Item::FIELD_ITEM)
            field_to_set= ((Item_field*)it)->field;
          else
          {
+            char buffer[128];
+            size_t len= (*ref)->name.length * sizeof((*ref)->name.str[0]);


please add assert that length is less than the buffer

sanja-byelkin · 2026-07-16T10:36:15Z

@@ -3831,6 +3834,7 @@ class Item_field :public Item_ident,
             const LEX_CSTRING &field_name_arg)
   :Item_field(thd, context_arg, null_clex_str, null_clex_str, field_name_arg)
  {}
+  Item_field(THD *thd):Item_ident(thd){field= nullptr;}


IMHO here is too many uninitialized value left. set have_privileges have_privileges any_privileges or comment where it will be set (fix_field? is it call from inherited Item) and why it will be not used before

sanja-byelkin requested a review from Copilot July 16, 2026 10:25

Copilot started reviewing on behalf of sanja-byelkin July 16, 2026 10:26 View session

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

Copilot AI reviewed Jul 16, 2026

View reviewed changes

sanja-byelkin commented Jul 16, 2026

View reviewed changes

		Item_old_field(THD thd, Item item= nullptr):Item_field(thd)
		{ expr= item; }

		bool is_old_value_reference;
		virtual Item *get_correct_item() { return this; }

Uh oh!

Uh oh!

Conversation

sanja-byelkin commented Jul 16, 2026

Uh oh!

CLAassistant commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

sanja-byelkin left a comment

Choose a reason for hiding this comment

Uh oh!

sanja-byelkin Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

sanja-byelkin Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

sanja-byelkin Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants