* 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
