Bug #19633 Stack corruption in fix_fields()/THD::rollback_item_tree_changes()
Submitted: 9 May 2006 11:17 Modified: 9 May 2006 21:09
Reporter: Kristian Nielsen Email Updates:
Status: Duplicate Impact on me:
Category:MySQL Server: Stored Routines Severity:S1 (Critical)
Version:5.1.10, 5.0.22 OS:Linux (Linux/All)
Assigned to: Assigned Account CPU Architecture:Any

[9 May 2006 11:17] Kristian Nielsen
I discovered a stack corruption bug (seen in pushbuild).

The corruption occurs in THD::rollback_item_tree_changes(). This function has the task of restoring Item values after execution of prepared statements or stored procedures. It keeps a list of Item_change_record,storing an Item * value and an Item ** location to restore to the value.

The problem is that other parts of the code store addresses of automatic variables (= stack locations) in the list. When THD::rollback_item_tree_changes() is later called, these automatic variables are long gone, and it will overwrite stack locations seemingly at random.

How to repeat:
The problem is seen on Linux when running the 'sp' test case, or just this minimal test:

    use test;
    create function bug2674() returns int  return @@sort_buffer_size;
    select bug2674();

When run in a build compiled --with-debug and with optimization, this is seen in the logs, because the dbug level counter is overwritten:

WARNING:  '/data0/pushbuild/mysql-5.1-new/push-serg@sergbook.mysql.com-20060509001439.info/mysql-5.1.10-beta-standard/sql/mysqld: missing DBUG_RETURN or DBUG_VOID_RETURN macro in function "rollback_item_tree_changes"'

But the problem is most easily seen from source inspection. In this particular case the call graph is the following from the sp_lex_keeper::reset_lex_and_exec_core() function:


The sp_eval_expr() function does this:

    sp_eval_expr(THD *thd, Field *result_field, Item *expr_item)
      if (!(expr_item= sp_prepare_func_item(thd, &expr_item)))

So it passes the address of the stack variable expr_item on to sp_prepare_func_item(), then to Item_func_get_system_var::fix_fields() to THD::change_item_tree() to THD::nocheck_register_item_tree_change() which stores the address away, later to corrupt the stack in THD::rollback_item_tree_changes().

But I believe the problem is more wide-spread than that, there are many places in which the address of an automatic variable is passed to fix_fields() (in event_timed.cc, for example).

Suggested fix:
As far as I can see, the parts of the code that call Item::fix_fields() with the address of an auto variable are wrong, there are several instances of that virtual method that use THD::change_item_tree().

I think one possibility is to go back up the call graph for these places and change it to pass along the original Item** location rather than the automatic variable. But I need to understand this code better first to be sure.
[9 May 2006 15:55] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

[9 May 2006 16:06] Kristian Nielsen
I committed a patch that seems to fix this.

The patch changes a few places in the stored procedure code to pass along the Item ** instead of the Item *, so that the correct, non-auto location can be registered with THD::nocheck_register_item_tree_change().

There are still places where fix_fields() is called with the address of an auto variable, in events_timed.cc and, in sp_head::execute_procedure(), and in Select_fetch_into_spvars::send_data(). As far as I can see, these only concern Item subclasses that do not pass the address to THD::nocheck_register_item_tree_change() in the fix_fields() virtual method, and thus do not have this problem. Not 100% sure though.

In any case, this is mighty fragile, and would benefit from refactoring.
[9 May 2006 19:26] Kristian Nielsen
From source inspection, it would appear that the same problem is present in 5.0. The code
looks substantially the same as the 5.1 code.
[9 May 2006 21:09] Konstantin Osipov
This is a duplicate of Bug#18037 "Server crash when returning system variable in stored procedures"