Re: bk commit into 5.0 tree (malff:1.2606) BUG#35577

[Sergei Golubchik]

Hi!

On Mar 28, marc.alff@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of malff.  When malff does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-03-28 11:32:08-06:00, malff@stripped. +12
> -0
>   Bug#35577 (CREATE PROCEDURE causes either crash or syntax error depending on
>   build)
>   
>   The crash was caused by freeing the internal parser stack during the parser
>   execution.
>   This occured only for complex stored procedures, after reallocating the parser
>   stack using my_yyoverflow(), with the following C call stack:
>   - MYSQLparse()
>   - any rule calling sp_head::restore_lex()
>   - lex_end()
>   - x_free(lex->yacc_yyss), xfree(lex->yacc_yyvs)
>   
>   The root cause is the implementation of stored procedures, which breaks the
>   assumption from 4.1 that there is only one LEX structure per parser call.
>   
>   The solution is to separate the LEX structure into:
>   - attributes that represent a statement (the current LEX structure),
>   - attributes that relate to the parser itself (Parser_state),
>   so that parsing multiple statements in stored programs can create multiple
>   LEX structures while not changing the unique Parser_state.
> 
>   mysql-test/r/parser_stack.result@stripped, 2008-03-28 11:32:04-06:00,
> malff@stripped. +191 -0
>     Bug#35577 (CREATE PROCEDURE causes either crash or syntax error depending on
>     build)
>     
> diff -Nrup a/sql/sp.cc b/sql/sp.cc
> --- a/sql/sp.cc	2008-02-19 08:27:17 -07:00
> +++ b/sql/sp.cc	2008-03-28 11:32:03 -06:00
> @@ -443,6 +443,8 @@ db_load_routine(THD *thd, int type, sp_n
>    {
>      Lex_input_stream lip(thd, defstr.c_ptr(), defstr.length());
>      thd->m_lip= &lip;
> +    Parser_state parser_state;
> +    thd->m_parser_state= &parser_state;

Add a comment here that thd->m_parser_state will be set to NULL in the parser
END_OF_INPUT rule.

>      lex_start(thd);
>      thd->spcont= NULL;
>      ret= MYSQLparse(thd);
> diff -Nrup a/sql/sql_lex.h b/sql/sql_lex.h
> --- a/sql/sql_lex.h	2007-11-19 09:59:43 -07:00
> +++ b/sql/sql_lex.h	2008-03-28 11:32:03 -06:00
> @@ -1280,6 +1279,39 @@ typedef struct st_lex : public Query_tab
>      return FALSE;
>    }
>  } LEX;
> +
> +
> +/**
> +  The internal state of the parser.
> +  This object is only available during parsing,
> +  and is private to the parser implementation.
> +*/
> +class Parser_state
> +{
> +public:
> +  Parser_state()
> +    : yacc_yyss(NULL), yacc_yyvs(NULL)
> +  {}
> +
> +  ~Parser_state();
> +
> +  /**
> +    Bison internal state stack, yyss, when dynamically allocated using
> +    my_yyoverflow().
> +  */
> +  gptr yacc_yyss;
> +
> +  /**
> +    Bison internal semantic value stack, yyvs, when dynamically allocated using
> +    my_yyoverflow().
> +  */
> +  gptr yacc_yyvs;
> +
> +  /*
> +    TODO: move more attributes from the LEX structure here.
> +  */
> +};

Ok, I agree that in principle is the correct approach, the parser state
should not be in the Lex. But why did you create a second class for the
parser state ? There is already Lex_input_stream class - which contain
parser state variables, such as tok_start, tok_end, next_state, yylval,
etc. Surely, there's not much sense in splitting parser state in two
classes - yyss and yyvs belong to the same class where yylval and
next_state do.
 
Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
/_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
       <___/                  Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring