Skip to content

MDEV-40125: OLD_VALUE crashes on a view with an expression #5393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sanja-byelkin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MDEV-40125: OLD_VALUE crashes on a view with an expression #5393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions mysql-test/main/update.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,4 +1148,31 @@ UPDATE t1 SET a=NULL RETURNING OLD_VALUE(a) AS old, a AS new;
old new
NULL
DROP TABLE t1;
#
# MDEV-40125: OLD_VALUE crashes on a view with an expression
#
CREATE TABLE t ( id INT PRIMARY KEY, a INT );
INSERT INTO t VALUES (1, 1), (3, 3), (5, 5);
CREATE VIEW v AS SELECT id, a, a*2 AS a2 FROM t;
SELECT * FROM v;
id a a2
1 1 2
3 3 6
5 5 10
UPDATE v SET a = a+1 WHERE id = 1 RETURNING OLD_VALUE(id) as old_id, id,
OLD_VALUE(a) as old_a, a,
OLD_VALUE(a2)as old_a2, a2;
old_id id old_a a old_a2 a2
1 1 1 2 2 4
SELECT * FROM v;
id a a2
1 2 4
3 3 6
5 5 10
UPDATE v SET a = a+1 WHERE id = 20 RETURNING OLD_VALUE(id) as old_id, id,
OLD_VALUE(a) as old_a, a,
OLD_VALUE(a2)as old_a2, a2;
old_id id old_a a old_a2 a2
DROP TABLE t;
DROP VIEW v;
# End of 13.0 test
21 changes: 21 additions & 0 deletions mysql-test/main/update.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,4 +1005,25 @@ INSERT INTO t1 VALUES('') RETURNING *;
UPDATE t1 SET a=NULL RETURNING OLD_VALUE(a) AS old, a AS new;
DROP TABLE t1;

--echo #
--echo # MDEV-40125: OLD_VALUE crashes on a view with an expression
--echo #

CREATE TABLE t ( id INT PRIMARY KEY, a INT );
INSERT INTO t VALUES (1, 1), (3, 3), (5, 5);
CREATE VIEW v AS SELECT id, a, a*2 AS a2 FROM t;

SELECT * FROM v;
UPDATE v SET a = a+1 WHERE id = 1 RETURNING OLD_VALUE(id) as old_id, id,
OLD_VALUE(a) as old_a, a,
OLD_VALUE(a2)as old_a2, a2;
SELECT * FROM v;

UPDATE v SET a = a+1 WHERE id = 20 RETURNING OLD_VALUE(id) as old_id, id,
OLD_VALUE(a) as old_a, a,
OLD_VALUE(a2)as old_a2, a2;

DROP TABLE t;
DROP VIEW v;

--echo # End of 13.0 test
77 changes: 35 additions & 42 deletions sql/item.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3383,6 +3383,19 @@ void Item_field::set_field(Field *field_par)
if (field->table->s->tmp_table == SYSTEM_TMP_TABLE ||
field->table->s->tmp_table == INTERNAL_TMP_TABLE)
set_refers_to_temp_table();

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;
}
Comment on lines +3387 to +3398

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

}


Expand Down Expand Up @@ -7230,6 +7243,14 @@ void Item_field::make_send_field(THD *thd, Send_field *tmp_field)
tmp_field->db_name= db_name;
}

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);
}
Comment on lines +7246 to +7252

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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



/**
Save a field value in another field
Expand Down Expand Up @@ -8055,55 +8076,27 @@ bool Item_field::send(Protocol *protocol, st_value *buffer)

bool Item_old_field::send(Protocol *protocol, st_value *buffer)
{
bool result;

change_field_ptr();
result= Item_field::send(protocol, buffer);
change_field_ptr();

return result;
}

void Item_old_field::change_field_ptr()
{
std::swap(field->ptr_old, field->ptr);
std::swap(field->null_ptr, field->null_ptr_old);
}


bool Item_old_field::fix_fields(THD *thd, Item **reference)
{
if (Item_field::fix_fields(thd, reference))
return true;
/*
Store the pointer to where old values are store before update.
*/
bool result= false;
if (field)
{
field->ptr_old= field->table->record[1] +
field->offset(field->table->record[0]);
if (field->null_ptr)
{
field->null_ptr_old =
field->table->record[1] +
(field->null_ptr - field->table->record[0]);
}
else
{
field->null_ptr_old = nullptr;
}
(void) change_field_ptr(0);
result= Item_field::send(protocol, buffer);
(void) change_field_ptr(0);
}
else
{
/*
Field is NULL in case of view. But we need the information to field
to return its old value. Hence get the field from the reference item
and proceed.
*/
field= ((Item_old_field*)((*reference)->real_item()))->field;
field->ptr_old= field->table->record[1] + field->offset(field->table->record[0]);
expr->walk(&Item::change_field_ptr, 0, 0);
result= expr->send(protocol, buffer);
expr->walk(&Item::change_field_ptr, 0, 0);
}

return result;
}

bool Item_field::change_field_ptr(void *arg)
{
std::swap(field->ptr_old, field->ptr);
std::swap(field->null_ptr, field->null_ptr_old);
return false;
}

Expand Down
43 changes: 29 additions & 14 deletions sql/item.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,8 @@ class Item :public Value_source,
return rc;
}
public:
bool is_old_value_reference;
bool is_old_value_reference;
virtual Item *get_correct_item() { return this; }
Comment on lines +991 to +992

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Keep the API of custom wrapper classes minimal by avoiding or removing unused methods.

/*
Cache val_str() into the own buffer, e.g. to evaluate constant
expressions with subqueries in the ORDER/GROUP clauses.
Expand Down Expand Up @@ -2289,6 +2290,7 @@ class Item :public Value_source,
// FIXME reduce the number of "add field to bitmap" processors
virtual bool add_field_to_set_processor(void *arg) { return 0; }
virtual bool register_field_in_read_map(void *arg) { return 0; }
virtual bool change_field_ptr(void *arg) { return 0; }
virtual bool register_field_in_write_map(void *arg) { return 0; }
virtual bool register_field_in_bitmap(void *arg) { return 0; }
virtual bool update_table_bitmaps_processor(void *arg) { return 0; }
Expand Down Expand Up @@ -3768,6 +3770,7 @@ class Item_ident :public Item_result_field
const LEX_CSTRING &db_name_arg, const LEX_CSTRING &table_name_arg,
const LEX_CSTRING &field_name_arg);
Item_ident(THD *thd, Item_ident *item);
Item_ident(THD *thd):Item_result_field(thd) {}
Item_ident(THD *thd, TABLE_LIST *view_arg, const LEX_CSTRING &field_name_arg);
LEX_CSTRING full_name_cstring() const override;
void cleanup() override;
Expand Down Expand Up @@ -3796,7 +3799,7 @@ class Item_field :public Item_ident,
public Load_data_outvar
{
protected:
void set_field(Field *field);
virtual void set_field(Field *field);
public:
Field *field;
Item_equal *item_equal;
Expand Down Expand Up @@ -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;}

@sanja-byelkin sanja-byelkin Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Item_field(THD *thd, Name_resolution_context *context_arg)
:Item_field(thd, context_arg, null_clex_str, null_clex_str, null_clex_str)
{}
Expand Down Expand Up @@ -4054,6 +4058,7 @@ class Item_field :public Item_ident,
{ return field->max_display_length(); }
Item_field *field_for_view_update() override { return this; }
int fix_outer_field(THD *thd, Field **field, Item **reference);
bool change_field_ptr(void *arg) override;
Item *update_value_transformer(THD *thd, uchar *select_arg) override;
Item *derived_field_transformer_for_having(THD *thd, uchar *arg) override;
Item *derived_field_transformer_for_where(THD *thd, uchar *arg) override;
Expand Down Expand Up @@ -4095,34 +4100,44 @@ class Item_old_field: public Item_field
Name_resolution_context *context_arg,
const LEX_CSTRING &db_arg,
const LEX_CSTRING &table_name_arg,
const LEX_CSTRING &field_name_arg)
const LEX_CSTRING &field_name_arg, Item *item=nullptr)
: Item_field(thd, context_arg, db_arg, table_name_arg, field_name_arg)
{ is_old_value_reference= true; }
{ is_old_value_reference= true; expr= item;}

Item_old_field(THD *thd,
Name_resolution_context *context_arg,
const LEX_CSTRING &field_name_arg)
const LEX_CSTRING &field_name_arg,
Item *item=nullptr)
: Item_field(thd, context_arg, field_name_arg)
{ is_old_value_reference= true; }
{ is_old_value_reference= true; expr= item;}

Item_old_field(THD *thd, Item_field *item)
Item_old_field(THD *thd, Item* item= nullptr):Item_field(thd)
{ expr= item; }
Comment on lines +4114 to +4115

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

Comment on lines +4114 to +4115

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

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

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

bool send(Protocol *protocol, st_value *buffer) override;
Type type() const override { return FIELD_OLD_ITEM; }
bool fix_fields(THD *, Item **) override;
void change_field_ptr();
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;
Comment on lines +4133 to +4140

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;


private:
uchar *saved_row_ref;
Expand Down
32 changes: 22 additions & 10 deletions sql/sql_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6399,12 +6399,14 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
(*ref)->is_old_value_reference)
{
Item *real = item->real_item();
Item_old_field *old= nullptr;

if (real->type() == Item::FIELD_ITEM)
{
Item_old_field *old =
new (thd->mem_root) Item_old_field(thd, (Item_field*) real);
item = old;
}
old= new (thd->mem_root) Item_old_field(thd, (Item_field*) real);
else
old= new (thd->mem_root) Item_old_field(thd, real);

item = old;
}

/*
Expand Down Expand Up @@ -6825,15 +6827,25 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
{
if (!ref)
DBUG_RETURN(fld);

Item *it= (*ref)->real_item();

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

@sanja-byelkin sanja-byelkin Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

size_t len= (*ref)->name.length * sizeof((*ref)->name.str[0]);

@sanja-byelkin sanja-byelkin Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add assert that length is less than the buffer

strncpy(buffer, (*ref)->name.str, len);
Comment on lines +6837 to +6839
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);
}
Comment on lines 6836 to 6849

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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

}
else
Expand Down Expand Up @@ -8334,10 +8346,10 @@ bool setup_fields(THD *thd, Ref_ptr_array ref_pointer_array,
item->split_sum_func(thd, ref_pointer_array, *sum_func_list,
SPLIT_SUM_SELECT);
}
lex->current_select->select_list_tables|= item->used_tables();
lex->used_tables|= item->used_tables();
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();
Comment on lines +8349 to +8352

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

}
lex->current_select->is_item_list_lookup= save_is_item_list_lookup;
lex->current_select->cur_pos_in_select_list= UNDEF_POS;
Expand Down
1 change: 0 additions & 1 deletion sql/table.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7378,7 +7378,6 @@ const Lex_ident_column Field_iterator_view::name()
return ptr->name;
}


Item *Field_iterator_view::create_item(THD *thd)
{
return create_view_field(thd, view, &ptr->item, &ptr->name);
Expand Down