| 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 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: http://lists.mysql.com/commits/6156
[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"
Description: 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: sp_lex_keeper::reset_lex_and_exec_core() sp_instr_freturn::exec_core() sp_rcontext::set_return_value() sp_eval_expr() sp_prepare_func_item() Item_func_get_system_var::fix_fields() THD::change_item_tree() THD::nocheck_register_item_tree_change() THD::rollback_item_tree_changes() The sp_eval_expr() function does this: bool 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.