Bug #26503 Illegal SQL exception handler code causes the server to crash
Submitted: 20 Feb 2007 17:06 Modified: 31 Mar 2007 23:03
Reporter: Peter Stuge Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Parser Severity:S1 (Critical)
Version:5.0.24-20070220bk, 5.1.15, 5.1.16BK, 5.2.3 OS:Linux (Linux 2.6.19.1+glibc 2.4, Windows)
Assigned to: Marc Alff CPU Architecture:Any
Tags: bfsm_2007_03_01, crashwork

[20 Feb 2007 17:06] Peter Stuge
Description:
I've created a 40-something table InnoDB schema with various triggers and stored procedures, some of which use transactions.

When stress testing the application I got deadlocks for transactions in stored procedures. I wrapped the transactions in a simple repeat until true construct and added two handlers for deadlock conditions so that the transaction would be retried until successful.

In the transaction I do an insert into a table that has a trigger, in which a select is performed on the same table being inserted to.

After having added the loop and deadlock handlers mysqld started to crash where it had previously failed with locking errors.

I have verified the crash with locally built 5.0.24, 5.0.32 and 5.0.33, as well as with mysql-standard-5.0.27-linux-i686 (static), mysql-5.1.15-beta-linux-i686-glibc23 and mysql-5.2.3-falcon-alpha-linux-i686-glibc23 from mysql.com, and bk://mysql.bkbits.net/mysql-5.0 as of 20070220.

I've tried to diagnose the problem as best as I could and it looks like an Item in Query_arena::free_list is getting corrupted. I added lots of couts to one build and could confirm that the Item::next for the particular instance was healthy in Item::Item() but corrupt in Query_arena::free_items()

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1330541664 (LWP 15484)]
0x00000031 in ?? ()
(gdb) bt
#0  0x00000031 in ?? ()
#1  0x08163e7a in Query_arena::free_items (this=0xb409b964) at item.h:823
#2  0x08163ef6 in THD::cleanup_after_query (this=0xb409b958)
    at sql_class.cc:577
#3  0x08194c64 in mysql_parse (thd=0xb409b958, 
    inBuf=0x8c104a8 "call gc_purchase(@tid,53,1)", length=27)
    at sql_parse.cc:5936
#4  0x0819574b in dispatch_command (command=COM_QUERY, thd=0xb409b958, 
    packet=0xb40ca0f1 "", packet_length=28) at sql_parse.cc:1786
#5  0x081969a5 in do_command (thd=0xb409b958) at sql_parse.cc:1568
#6  0x08197600 in handle_one_connection (arg=0xb409b958) at sql_parse.cc:1194
#7  0xb7f5e294 in start_thread () from /lib/libpthread.so.0
#8  0xb7da1e7e in clone () from /lib/libc.so.6
(gdb) f 1
#1  0x08163e7a in Query_arena::free_items (this=0xb409b964) at item.h:823
823         cleanup();
(gdb) p *this
$1 = {_vptr.Query_arena = 0x844f6dc, free_list = 0x8c12120, 
  mem_root = 0xb409b978, is_backup_arena = false, 
  state = CONVENTIONAL_EXECUTION}
(gdb) p *this->free_list
$2 = {_vptr.Item = 0x920cde8, rsize = 9, str_value = {
    Ptr = 0x2 <Address 0x2 out of bounds>, str_length = 4, Alloced_length = 0, 
    alloced = false, str_charset = 0x8525d20}, name = 0x84c733f "NULL", 
  orig_name = 0x0, next = 0x8c12060, max_length = 0, name_length = 0, 
  marker = 0 '\0', decimals = 0 '\0', maybe_null = 1 '\001', 
  null_value = 1 '\001', unsigned_flag = 0 '\0', with_sum_func = 0 '\0', 
  fixed = 1 '\001', is_autogenerated_name = 1 '\001', collation = {
    collation = 0x8525d20, derivation = DERIVATION_IGNORABLE}, 
  with_subselect = 0 '\0', cmp_context = 4294967295}
(gdb) 

I found another bug that featured memory corruption because a thd was reused when it wasn't appropriate but can't find the id now. That bug was fixed, so it's not a straight duplicate though.

If I remove the out tid parameter I can't reproduce the crash.
If I remove the 1205 handler I get a 1213 error instead of crash. (Strange!)
If I remove the '40001' handler I get a 1213 error instead of crash.

How to repeat:
Attached DDL is a heavily stripped down test case that will require more connections to trigger the problem. Create the database and run call proc1(@tid) in a couple of hundred connections in parallell. On a 2GHz CPU and using 400 clients I get the crash about 1 out of 10 times.

Suggested fix:
Make sure to not corrupt memory and let Query_arena::free_items() finish cleaning up.
[20 Feb 2007 17:28] Peter Stuge
attached DDL

Attachment: bug26503.mysql (application/octet-stream, text), 1.53 KiB.

[20 Feb 2007 17:36] MySQL Verification Team
verified using attached testcase.
crashes about 50% of the time, which stack the same on linux as on windows:

mysqld.exe!Query_arena::free_items()  + 0x2e bytes	C++
mysqld.exe!THD::cleanup_after_query()  + 0x2f bytes	C++
mysqld.exe!mysql_parse()  + 0x18e bytes	C++
mysqld.exe!dispatch_command()  + 0x5e5 bytes	C++
mysqld.exe!do_command()  + 0xdc bytes	C++
mysqld.exe!handle_one_connection()  + 0x326 bytes	C++
mysqld.exe!pthread_start()  + 0x56 bytes	C
mysqld.exe!_callthreadstart()  Line 295	C
mysqld.exe!_threadstart(void * ptd=0x0000000077d6b660)  Line 275 + 0x5 bytes
[20 Feb 2007 17:37] MySQL Verification Team
see header of file for host, user, port, gcc build instructions, etc.

Attachment: bug26503_testcase.c (text/plain), 7.56 KiB.

[20 Feb 2007 17:49] MySQL Verification Team
testcase for bug #26089 doesn't crash on my server, but this testcase still does.
[20 Feb 2007 19:25] MySQL Verification Team
5.1.16BK crashed also.  It was easier to crash the release builds than the debug builds - presumably due to the speed difference.
[28 Feb 2007 18:20] Marc Alff
ANALYSIS (I)

Reproduced with a single thread, by forcing the error in the code
to investigate:

With mysys/thr_lock.cc modified as below:

enum enum_thr_lock_result
thr_lock(THR_LOCK_DATA *data, THR_LOCK_OWNER *owner,
         enum thr_lock_type lock_type)
{
  static int debug = 0;

  debug++;

  if ((debug % 100) == 0)
    return THR_LOCK_DEADLOCK;

  return thr_lock_impl(data, owner, lock_type);
}

'random' (every 100 calls) failures will occur.

Calling the procedure in a loop using mysqltest scripts

let $loop=150;
while ($loop)
{
  set @tid=0;
  call proc1(@tid);
  select @tid;
  dec $loop;
}

exposes the bug.
[28 Feb 2007 18:22] Marc Alff
ANALYSIS (II)

Reproduced with the following test case script:

--disable_warnings
drop database if exists bug26503;
--enable_warnings

set SQL_MODE="ANSI";

create database bug26503;
use bug26503;

create table t1(a int);

delimiter //;

create procedure proc1(out tid int)
begin
  declare var int;
  set tid= null;
retry:
  repeat
    begin
      select 'Push';

      begin
        declare continue handler for 1329
        begin
          select 'Handler';
          insert into t1 values (1);
          iterate retry;
        end;

        select 'Before';
        ## Raises a warning 1329 the first time
        select a into var from t1;
        select 'After';

      end;
      select 'Pop';
    end;
  until true end repeat retry;
end//
delimiter ;//

show procedure code proc1;

set @tid=0;
call proc1(@tid);
select @tid;

drop database if exists bug26503;

The runtime execution is:

call proc1(@tid);
Push
Push
Before
Before
Handler
Handler
Push
Push
Before
Before
After
After
Pop
Pop
select @tid;
@tid
0

The crash is caused by the second push of the exception handler,
overflowing the exception handler stack size.

See
sp_rcontext::push_handler()
sp_rcontext::init()

The SQL code :

declare continue handler for xxx
iterate retry <-- leaving the exception handler here

cause the runtime to never pop the first handler, leading to the crash
[28 Feb 2007 23:16] Marc Alff
Dear Peter,

I would like to express our gratitude for the work you put together
by providing a bug report with a test case showing the problem.
It was instrumental in getting the issue reproduced, and narrowed down
to the root cause.

As for the crash itself, it's not related to queries causing deadlocks,
but is related to the way the exception handler loops.

In particular :

retry: <--------------------------------------
  repeat                                      |
    begin                                     |
      declare continue handler for xxx        |
      begin                                   |
        iterate retry; -----------------------
      end
    end
  until true end repeat retry;

cause the bug.

This construct happens to be illegal according to the SQL:2003 specification,
since the scope of the 'retry' label does not extend to the code located
within exception handlers.

While this is a confirmed bug in the parser, which should reject the code,
you probably need to rewrite the loop to use a valid (and working) syntax
instead. See below for an example.

create procedure proc1()
begin
  declare done boolean;

retry:
  repeat
    begin
      declare cond_wait_timeout condition for 1205;
      declare cond_deadlock condition for sqlstate '40001';
      declare continue handler for cond_wait_timeout, cond_deadlock
      begin
        -- Failed, trying again
        set done = false;
      end;

      set done = true;

      -- Attempt something that might fail
      ...
    end;
  until done end repeat retry;
end//

Regards,
Marc.
[1 Mar 2007 2:59] Peter Stuge
Marc,

Thanks for your excellent analysis and the work all of you put into MySQL. I'm glad to help improve it and I know how important decent bug reports are, especially for fun stuff like double frees with lots of threads.

Regarding label scope I only have access to the SQL:2003 wiscorp draft and it seems to me that a handler declaration would have to be considered a SQL schema statement to be excluded by repeat statement syntax rule 4, but I have found nothing to confirm that is the case. All mentions of handler declarations I can find are among the control statements. Could you point me to the right place in the documents?

If iterate-within-handlers is illegal, that should be documented in chapter 17 of the MySQL manual. Should I file a separate bug for documentation?

If the problem isn't really iterate but the label scope, then leave:ing within a handler is not legal either, is that correct?

Thanks again!
[13 Mar 2007 0:38] 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/21771

ChangeSet@1.2446, 2007-03-12 18:37:34-06:00, malff@weblab.(none) +9 -0
  Bug#26503 (Illegal SQL exception handler code causes the server to crash)
  
  Before this fix, the parser would accept illegal code in SQL exceptions
  handlers, that later causes the runtime to crash when executing the code,
  due to memory violations in the exception handler stack.
  
  The root cause of the problem is instructions within an exception handler
  that jumps to code located outside of the handler. This is illegal according
  to the SQL 2003 standard, since labels located outside the handler are not
  supposed to be visible (they are "out of scope"), so any instruction that
  jumps to these labels, like ITERATE or LEAVE, should not parse.
  
  The section of the standard that is relevant for this is :
    SQL:2003 SQL/PSM (ISO/IEC 9075-4:2003)
    section 13.1 <compound statement>,
    syntax rule 4
  <quote>
    The scope of the <beginning label> is CS excluding every <SQL schema
    statement> contained in CS and excluding every
    <local handler declaration list> contained in CS. <beginning label> shall
    not be equivalent to any other <beginning label>s within that scope.
  </quote>
  
  With this fix, the C++ class sp_pcontext, which represent the "parsing
  context" tree (a.k.a symbol table) of a stored procedure, has been changed
  as follows:
  - constructors have been cleaned up, so that only building a root node for
  the tree is public; building nodes inside a tree is not public.
  - a new member, m_exception_handler, indicates if a given syntactic context
  belongs to a DECLARE HANDLER block,
  - label resolution, in the method find_label(), has been changed to
  implement the restriction of scope regarding labels used in a compound
  statement.
  
  The actions in the parser, when parsing the body of a SQL exception handler,
  have been changed as follows:
  - the implementation of an exception handler (DECLARE HANDLER) now creates
  explicitly a new sp_pcontext, to isolate the code inside the handler from
  the containing compound statement context.
  - registering exception handlers as a result occurs in the parent context,
  see the rule sp_hcond_element
  - the code in sp_hcond_list has been cleaned up, to avoid code duplication
  
  In addition, the flags IN_SIMPLE_CASE and IN_HANDLER, declared in sp_head.h
  have been removed, since they are unused and broken by design (as seen with
  Bug 19194 (Right recursion in parser for CASE causes excessive stack usage,
  limitation), representing a stack in a single flag is not possible.
  
  Tests in sp-error have been added to show that illegal constructs are now
  rejected.
  
  Tests in sp have been added for code coverage, to show that ITERATE or LEAVE
  statements are legal when jumping to a label in scope, inside the body of
  an exception handler.
[13 Mar 2007 1:08] Marc Alff
Hi Peter

Please see the comments in the patch, for the reference to the SQL:2003 spec.

Iterate-within-handlers is legal ... as long as the label pointed to is in scope, which means that it must jump to code within the handler itself.

Thanks for noticing LEAVE, the test cases cover both ITERATE and LEAVE then.

-- Marc
[13 Mar 2007 20:59] Konstantin Osipov
Sent a review over email with  a few comments.
Approved (no second review needed).
[14 Mar 2007 18:17] 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/21930

ChangeSet@1.2446, 2007-03-14 12:02:32-06:00, malff@weblab.(none) +9 -0
  Bug#26503 (Illegal SQL exception handler code causes the server to crash)
  
  Before this fix, the parser would accept illegal code in SQL exceptions
  handlers, that later causes the runtime to crash when executing the code,
  due to memory violations in the exception handler stack.
  
  The root cause of the problem is instructions within an exception handler
  that jumps to code located outside of the handler. This is illegal according
  to the SQL 2003 standard, since labels located outside the handler are not
  supposed to be visible (they are "out of scope"), so any instruction that
  jumps to these labels, like ITERATE or LEAVE, should not parse.
  
  The section of the standard that is relevant for this is :
    SQL:2003 SQL/PSM (ISO/IEC 9075-4:2003)
    section 13.1 <compound statement>,
    syntax rule 4
  <quote>
    The scope of the <beginning label> is CS excluding every <SQL schema
    statement> contained in CS and excluding every
    <local handler declaration list> contained in CS. <beginning label> shall
    not be equivalent to any other <beginning label>s within that scope.
  </quote>
  
  With this fix, the C++ class sp_pcontext, which represent the "parsing
  context" tree (a.k.a symbol table) of a stored procedure, has been changed
  as follows:
  - constructors have been cleaned up, so that only building a root node for
  the tree is public; building nodes inside a tree is not public.
  - a new member, m_label_scope, indicates if a given syntactic context
  belongs to a DECLARE HANDLER block,
  - label resolution, in the method find_label(), has been changed to
  implement the restriction of scope regarding labels used in a compound
  statement.
  
  The actions in the parser, when parsing the body of a SQL exception handler,
  have been changed as follows:
  - the implementation of an exception handler (DECLARE HANDLER) now creates
  explicitly a new sp_pcontext, to isolate the code inside the handler from
  the containing compound statement context.
  - registering exception handlers as a result occurs in the parent context,
  see the rule sp_hcond_element
  - the code in sp_hcond_list has been cleaned up, to avoid code duplication
  
  In addition, the flags IN_SIMPLE_CASE and IN_HANDLER, declared in sp_head.h
  have been removed, since they are unused and broken by design (as seen with
  Bug 19194 (Right recursion in parser for CASE causes excessive stack usage,
  limitation), representing a stack in a single flag is not possible.
  
  Tests in sp-error have been added to show that illegal constructs are now
  rejected.
  
  Tests in sp have been added for code coverage, to show that ITERATE or LEAVE
  statements are legal when jumping to a label in scope, inside the body of
  an exception handler.
[22 Mar 2007 21:23] Konstantin Osipov
Fixed in 5.0.40 and 5.1.17
[31 Mar 2007 23:03] Paul Dubois
Noted in 5.0.40, 5.1.17 changelogs.

The parser accepted illegal code in SQL exception handlers, leading
to a crash at runtime when executing the code.
[19 Jul 2007 17:47] Paul Dubois
Revised changelog entry:

*Important note*
The parser accepted invalid code in SQL condition handlers,
leading to server crashes or unexpected execution behavior in
stored programs.  Specifically, the parser allowed a condition
handler to refer to labels for blocks that enclose the handler
declaration. This was incorrect because block label scope
does not include the code for handlers declared within the
labeled block.

The parser now rejects this invalid construct, but if you
upgrade in place (without dumping and reloading your databases),
existing handlers that contain the construct still are invalid
*even if they appear to function as you expect* and should
be rewritten.

To find affected handlers, use mysqldump to dump all stored
functions and procedures, triggers, and events. Then attempt
to reload them into an upgraded server. Handlers that contain
illegal label references will be rejected.

For more information about condition handlers and writing them
to avoid invalid jumps, see 
http://dev.mysql.com/doc/mysql/en/declare-handlers.html.
[21 Dec 2007 17:17] Marc Alff
See related Bug #33465 (Temporarily disable fix for bug#26503).