From c894d97781b54fbf074677c9d67f81cd60a162ac Mon Sep 17 00:00:00 2001 From: Rex Johnston Date: Mon, 19 Oct 2026 09:35:25 +1300 Subject: [PATCH] MDEV-41631 Check the class of the copy made by Item::shallow_copy() Item::shallow_copy() was meant to create a copy of the same class as the item it is called on, but nothing enforced that. A class that does not declare its own override inherits the one of its nearest concrete parent, and get_item_copy() then silently creates an object of the parent class, losing the child's virtual behaviour (see MDEV-38801 for an earlier instance with Item_cache_year). Add the ITEM_SHALLOW_COPY(CLASS) macro, which declares the override, and Item::dbug_check_shallow_copy_type(), a debug-only check called from Query_arena::free_items() for every item freed. Once per dynamic type it creates a shallow copy and asserts that it has the same typeid as the original. Then, the copy is destroyed, which provides additional coverage. The check found these classes copied as their parent class, now fixed with ITEM_SHALLOW_COPY(): Item_join_operator_plus, Item_null_result, Item_old_field, Item_datetime, Item_empty_string, Item_return_int, Item_trigger_type_of_statement, in_string::Item_string_for_in_vector, Item_is_not_null_test, Item_interval_DDhhmmssff_typecast, Item_string_xml_non_const, Item_func_trt_trx_sees_eq, Item_field_refcursor (type_cursor plugin), Item_field_packable_scalar (type_assoc_array plugin). These two classes do not support copying so the patch makes their shallow_copy() to return NULL: Item_cache_wrapper, Item_sum_sp Item_field_packable_row::shallow_copy() has assert(0). The patch makes it return NULL. This commit was prepared with help of Claude Code (Opus 5). Sergei Petrunia added "delete item_copy" and did other minor edits. --- .../type_assoc_array/sql_type_assoc_array.cc | 8 ++-- plugin/type_cursor/plugin.cc | 2 + sql/item.cc | 47 +++++++++++++++++++ sql/item.h | 34 +++++++++++++- sql/item_cmpfunc.h | 4 ++ sql/item_sum.h | 2 +- sql/item_timefunc.h | 2 + sql/item_vers.h | 2 + sql/item_xmlfunc.cc | 2 + sql/sql_class.cc | 5 ++ 10 files changed, 101 insertions(+), 7 deletions(-) diff --git a/plugin/type_assoc_array/sql_type_assoc_array.cc b/plugin/type_assoc_array/sql_type_assoc_array.cc index 8b144e0fd1a..92e0afa02f5 100644 --- a/plugin/type_assoc_array/sql_type_assoc_array.cc +++ b/plugin/type_assoc_array/sql_type_assoc_array.cc @@ -244,6 +244,8 @@ public: unpack(); return Item_field::save_in_field(field, no_conversions); } +protected: + ITEM_SHALLOW_COPY(Item_field_packable_scalar) }; @@ -280,11 +282,7 @@ public: return false; } - Item *shallow_copy(THD *thd) const override - { - DBUG_ASSERT(0); - return get_item_copy(thd, this); - } + Item *shallow_copy(THD *thd) const override { return nullptr; } static uint packed_col_length(Field *field) { diff --git a/plugin/type_cursor/plugin.cc b/plugin/type_cursor/plugin.cc index 51f9675180d..f850fbe6aa0 100644 --- a/plugin/type_cursor/plugin.cc +++ b/plugin/type_cursor/plugin.cc @@ -367,6 +367,8 @@ public: DBUG_ASSERT(fc); return fc->resolve_spvar_cursor_rowtype(thd, cursor); } +protected: + ITEM_SHALLOW_COPY(Item_field_refcursor) }; diff --git a/sql/item.cc b/sql/item.cc index 6afda43f5b9..77ec8c0367a 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -12608,3 +12608,50 @@ bool ignored_list_includes_table(ignored_tables_list_t list, TABLE_LIST *tbl) } return false; } + + +#ifndef DBUG_OFF +#include +#include +#include + +/* + Check that this item has implemented shallow_copy() +*/ +void Item::dbug_check_shallow_copy_type(THD *thd) const +{ + /* + Maintain a set of Item_xxx types that were already checked and don't check + them again. + */ + static std::unordered_set checked_types; + static std::mutex checked_mutex; + { + std::lock_guard lock(checked_mutex); + if (!checked_types.insert(typeid(*this)).second) + return; + } + + Item *item_copy= shallow_copy(thd); + if (!item_copy) + return; /* This item cannot be cloned */ + + /* + We don't want cleanup() to be called for the item_copy, as it is not + fixed. + To achieve this, unlink it from thd->free_list and delete ourselves. + item_copy is the last item that was created, so it is the first in + thd->free_list. + */ + DBUG_ASSERT(thd->free_list == item_copy); + if (thd->free_list == item_copy) + thd->free_list= item_copy->next; + if (typeid(*item_copy) != typeid(*this)) + { + fprintf(stderr, "SHALLOW_COPY_TYPE_MISMATCH: %s copied as %s\n", + typeid(*this).name(), typeid(*item_copy).name()); + DBUG_ASSERT(0); + } + delete item_copy; +} +#endif diff --git a/sql/item.h b/sql/item.h index 4707db64ac2..dfa91d7ba88 100644 --- a/sql/item.h +++ b/sql/item.h @@ -1971,6 +1971,15 @@ public: return clone; } +#ifndef DBUG_OFF + /* + Check, once per dynamic type, that shallow_copy() creates an object of + the same class as this item, i.e. that the class did not inherit + shallow_copy() from a concrete parent. + */ + void dbug_check_shallow_copy_type(THD *thd) const; +#endif + /* Clones the constant item (not necessary returning the same item type) @@ -2912,6 +2921,15 @@ inline T* get_item_copy (THD *thd, const T* item) return copy; } +/* + Every instantiable Item class must declare its own shallow_copy(): one + inherited from a concrete parent creates an object of the parent class. + Item::dbug_check_shallow_copy_type() asserts that in debug builds. +*/ +#define ITEM_SHALLOW_COPY(CLASS) \ + Item *shallow_copy(THD *thd) const override \ + { return get_item_copy(thd, this); } + #ifndef DBUG_OFF /** @@ -4214,6 +4232,8 @@ public: private: uchar *saved_row_ref; +protected: + ITEM_SHALLOW_COPY(Item_old_field) }; /** @@ -4330,6 +4350,8 @@ public: str->append("(+)"_LEX_CSTRING); } static List *make_as_item_list(THD *thd); +protected: + ITEM_SHALLOW_COPY(Item_join_operator_plus) }; @@ -4360,6 +4382,8 @@ public: { return mark_unsupported_function(full_name(), arg, VCOL_IMPOSSIBLE); } +protected: + ITEM_SHALLOW_COPY(Item_null_result) }; /* @@ -5048,6 +5072,8 @@ public: return false; } void print(String *str, enum_query_type query_type) override; +protected: + ITEM_SHALLOW_COPY(Item_datetime) }; @@ -5459,6 +5485,8 @@ public: length * cs->mbmaxlen, cs) { } void make_send_field(THD *thd, Send_field *field) override; +protected: + ITEM_SHALLOW_COPY(Item_empty_string) }; @@ -5478,6 +5506,8 @@ public: Type_handler::get_handler_by_field_type(int_field_type); return unsigned_flag ? h->type_handler_unsigned() : h; } +protected: + ITEM_SHALLOW_COPY(Item_return_int) }; @@ -6763,7 +6793,7 @@ public: } protected: Item *shallow_copy(THD *thd) const override - { return get_item_copy(thd, this); } + { return NULL; /* Not supported */ } Item *deep_copy(THD *) const override { return nullptr; } bool add_maybe_null_after_ora_join_processor(void *arg) override { @@ -7983,6 +8013,8 @@ public: private: THD *m_thd; active_dml_stmt m_trigger_stmt_type; +protected: + ITEM_SHALLOW_COPY(Item_trigger_type_of_statement) }; diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h index cf3418401f0..31501be6ffa 100644 --- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -1699,6 +1699,8 @@ class in_string :public in_vector str_value= *str; collation.set(str->charset()); } + protected: + ITEM_SHALLOW_COPY(Item_string_for_in_vector) }; public: in_string(THD *thd, uint elements, qsort_cmp2 cmp_func, CHARSET_INFO *cs); @@ -3005,6 +3007,8 @@ public: table_map used_tables() const override { return used_tables_cache | RAND_TABLE_BIT; } bool const_item() const override { return FALSE; } +protected: + ITEM_SHALLOW_COPY(Item_is_not_null_test) }; diff --git a/sql/item_sum.h b/sql/item_sum.h index b9c6e4d89de..1f94e71b92f 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -1586,7 +1586,7 @@ public: protected: Item *shallow_copy(THD *thd) const override - { return get_item_copy(thd, this); } + { return NULL; /* not supported */ } }; /* Items to get the value of a stored sum function */ diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h index 49b082d3421..97705071ca1 100644 --- a/sql/item_timefunc.h +++ b/sql/item_timefunc.h @@ -1505,6 +1505,8 @@ public: null_value= !it.is_valid_interval_DDhhmmssff(); return it.to_string(to, m_fsp); } +protected: + ITEM_SHALLOW_COPY(Item_interval_DDhhmmssff_typecast) }; diff --git a/sql/item_vers.h b/sql/item_vers.h index ca77e8761c5..38056db0a0d 100644 --- a/sql/item_vers.h +++ b/sql/item_vers.h @@ -148,6 +148,8 @@ public: static LEX_CSTRING name= {STRING_WITH_LEN("trt_trx_sees_eq") }; return name; } +protected: + ITEM_SHALLOW_COPY(Item_func_trt_trx_sees_eq) }; #endif /* ITEM_VERS_INCLUDED */ diff --git a/sql/item_xmlfunc.cc b/sql/item_xmlfunc.cc index e78796c684e..1634f3c261e 100644 --- a/sql/item_xmlfunc.cc +++ b/sql/item_xmlfunc.cc @@ -614,6 +614,8 @@ public: */ return this; } +protected: + ITEM_SHALLOW_COPY(Item_string_xml_non_const) }; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index cfa574eca03..a0c98987d8b 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4352,6 +4352,11 @@ void Query_arena::free_items() { Item *next; DBUG_ENTER("Query_arena::free_items"); +#ifndef DBUG_OFF + if (THD *thd= current_thd) + for (Item *item= free_list; item; item= item->next) + item->dbug_check_shallow_copy_type(thd); +#endif /* This works because items are allocated on THD::mem_root */ for (; free_list; free_list= next) {