Re: bk commit into 5.0 tree (malff:1.2583) BUG#33618

[Konstantin Osipov]

* marc.alff@stripped <marc.alff@stripped> [08/01/10 03:00]:

The patch is OK to push.

Thanks for cracking this hard nut.

Minor comments below:

> ChangeSet@stripped, 2008-01-09 16:48:32-07:00, malff@stripped. +9 -0
>   Bug#33618 (Crash in sp_rcontext)
>   
>   There are several layers of issues that caused this crash.
>   
>   1)
>   The server crash in sp_rcontext, due to a stack overflow in the sp_rcontext
>   handler stack.
>   
>   2)
>   The stack overflow was caused by incorrect generated code, where a hpop
>   instruction was missing for a LEAVE. Repeated execution of this LEAVE
>   statement in a loop caused wide spread memory corruption.
>   
>   3)
>   (See the bug report for the code exposing the bug)
>   The missing hpop was caused by the call to sp_head::diff_handlers(),
>   which considered that there is no handler to pop since exclusive is true.
>   Code generation for the LEAVE statement assumed that the handler will be
>   removed by the code generated at the end of a begin-end block,
>   which did not happen when jumping to a REPEAT-UNTIL "rep1:" label.
>   
>   4)
>   Resolution of forward jumps with the backpacth list (sp_head::backpatch)
>   was not working properly, and generated jumps to the wrong instruction.
>   This is because backpatches for begin-end blocks label were resolved
>   *twice*, and pointed to different locations:
>   - before the end hpop/cpop instructions ending a block
>   - after the end hpop/cpop instructions ending a block
>   This caused the jump for the LEAVE statement to the "rep1:" label to
>   point *after* the hpop instruction.
>   
>   5)
>   Issue 4) in the code itself was hidden by a poor implementation
>   of sp_instr_jump:backpatch(), which accepted multiple destination for the
>   same instruction.
>   
>   6)
>   Iteration labels like "rep1:" were initially typed as SP_LAB_ITER,
>   but later corrupted to SP_LAB_BEGIN in the parser grammar rules,
>   adding to the confusion when generating code.
>   
>   With this fix,
>   
>   (1) is fixed by enforcing integrity of every internal stack in sp_rcontext
>   with DBUG_ASSERT statements.
>   
>   (2) and (3) are fixed by this line of code:
>     bool exclusive= (lab->type == SP_LAB_BEGIN);
>   
>   (4) is fixed by implementing the following grammar rules in the parser:
>   - sp_unlabeled_block
>   - sp_labeled_block
>   these rules resolve the sp_head::backpatch() *inside* sp_block_content,
>   while rules for control flow operations do that *ousdide*
>   sp_unlabeled_control
>   In all cases, backpatch is done only once for a given label.
>   
>   (5) is fixed in sp_instr_jump:backpatch() with a DBUG_ASSERT.
>   
>   (6) is fixed by assigning SP_LAB_BEGIN only to block labels

The above is useful for understanding the patch, but let's try to
produce something shorter for a changeset comment, something like:

The server used to crash when REPEAT or other control instruction
was used in conjunction with labels and LEAVE instruction.

The crash was caused by a memory corruption which happened due to
a double "pop" of cursors and continue handlers (double memory
free), which was in turn caused by a design bug in the stored
procedure grammar (the one pass compiler).

Please feel free to edit the above :)

> +          sp_block_content sp_opt_label
> +          {
> +            LEX *lex= Lex;
> +
> +            if ($5.str)
> +            {
> +              sp_label_t *lab= lex->spcont->find_label($5.str);
> +
> +              if (!lab ||
> +                  my_strcasecmp(system_charset_info, $5.str, lab->name) != 0)
> +              {
> +                my_error(ER_SP_LABEL_MISMATCH, MYF(0), $5.str);
> +                MYSQL_YYABORT;
> +              }
> +            }
> +            lex->spcont->pop_label();
> +          }
> +        ;

While you're at it, could you please fix the incorrect name
resolution here? 

I reported it as a separate bug:
Bug#33983 Stored Procedures: wrong end <label> syntax is accepted

Thanks again for looking at this!

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY