maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Sanja,

On Wed, Jun 22, 2016 at 02:17:06PM +0200, Oleksandr Byelkin wrote:
> revision-id: 475cab835fb48c91d5cca649ab93917ec1718d75 (mariadb-5.5.50-6-g475cab8)
> parent(s): a482e76e65a4fee70479e877929381c86b1ec62f
> committer: Oleksandr Byelkin
> timestamp: 2016-06-22 14:17:06 +0200
> message:
> 
> MDEV-10050: Crash in subselect
> 
> thd should not be taken earlier then fix_field and reset on fix_fields if it is needed.
> 
> ---
>  sql/item_subselect.cc | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
> index ba67474..60cdd3f 100644
> --- a/sql/item_subselect.cc
> +++ b/sql/item_subselect.cc
> @@ -79,7 +79,9 @@ void Item_subselect::init(st_select_lex *select_lex,
>    DBUG_PRINT("enter", ("select_lex: 0x%lx  this: 0x%lx",
>                         (ulong) select_lex, (ulong) this));
>    unit= select_lex->master_unit();
> -  thd= unit->thd;
> +#ifndef DBUG_OFF
> +  thd= 0;
> +#endif

So I've applied the patch, and I'm debugging this statement: 

prepare s from 'select a, (select max(one_k.a) from one_k where one_k.a <ten.a) from ten';

(this is just the first subquery I tried, nothing special about it).

We arrive at the above #ifndef, and I have thd=0xa5a5a5a5a5, that is, it's
uninitialized data.

I let it execute further...
>  if (unit->item)
>  {
>    /*
>      Item can be changed in JOIN::prepare while engine in JOIN::optimize
>      => we do not copy old_engine here
>    */
>    engine= unit->item->engine;
>    own_engine= FALSE;
>    parsing_place= unit->item->parsing_place;
>    unit->thd->change_item_tree((Item**)&unit->item, this);
>    engine->change_result(this, result, TRUE);
>  }
>  else
>  {
>    SELECT_LEX *outer_select= unit->outer_select();
>    /*
>      do not take into account expression inside aggregate functions because
>      they can access original table fields
>    */
>    parsing_place= (outer_select->in_sum_expr ?
>                    NO_MATTER :
>                    outer_select->parsing_place);
>    if (unit->is_union())
>      engine= new subselect_union_engine(thd, unit, result, this);
>    else
>      engine= new subselect_single_select_engine(thd, select_lex, result, this);

and it arrives here.
Stepping into this call, I eventually reach where this stack trace shows:

(gdb) wher
  #0  select_result::set_thd (this=0x7fffb002c130, thd_arg=0x0) at /home/psergey/dev-git/5.5/sql/sql_class.h:3316
  #1  0x000000000086fd30 in subselect_engine::set_thd (this=0x7fffb002c150, thd_arg=0x0) at /home/psergey/dev-git/5.5/sql/item_subselect.cc:2892
  #2  0x00000000007648e9 in subselect_engine::subselect_engine (this=0x7fffb002c150, thd_arg=0x0, si=0x7fffb002c010, res=0x7fffb002c130) at /home/psergey/dev-git/5.5/sql/item_subselect.h:726
  #3  0x000000000086fd66 in subselect_single_select_engine::subselect_single_select_engine (this=0x7fffb002c150, thd_arg=0x0, select=0x7fffb002a8e0, result_arg=0x7fffb002c130, item_arg=0x7fffb002c010) at /home/psergey/dev-git/5.5/sql/item_subselect.cc:2902
  #4  0x0000000000868232 in Item_subselect::init (this=0x7fffb002c010, select_lex=0x7fffb002a8e0, result=0x7fffb002c130) at /home/psergey/dev-git/5.5/sql/item_subselect.cc:111
  #5  0x000000000086a0d1 in Item_singlerow_subselect::Item_singlerow_subselect (this=0x7fffb002c010, select_lex=0x7fffb002a8e0) at /home/psergey/dev-git/5.5/sql/item_subselect.cc:906
  #6  0x000000000077d36f in MYSQLparse (thd=0x2fd99e0) at /home/psergey/dev-git/5.5/sql/sql_yacc.yy:8366
  #7  0x00000000006214f2 in parse_sql (thd=0x2fd99e0, parser_state=0x7ffff7f81800, creation_ctx=0x0) at /home/psergey/dev-git/5.5/sql/sql_parse.cc:7818
  #8  0x0000000000633dda in Prepared_statement::prepare (this=0x7fffb00167f0, packet=0x7fffb00063f8 "select a, (select max(one_k.a) from one_k where one_k.a <ten.a) from ten", packet_len=72) at /home/psergey/dev-git/5.5/sql/sql_prepare.cc:3353
  #9  0x0000000000631a89 in mysql_sql_stmt_prepare (thd=0x2fd99e0) at /home/psergey/dev-git/5.5/sql/sql_prepare.cc:2458
  #10 0x000000000061324d in mysql_execute_command (thd=0x2fd99e0) at /home/psergey/dev-git/5.5/sql/sql_parse.cc:2239
  #11 0x000000000061d58d in mysql_parse (thd=0x2fd99e0, rawbuf=0x7fffb00062c8 "prepare s from 'select a, (select max(one_k.a) from one_k where one_k.a <ten.a) from ten'", length=89, parser_state=0x7ffff7f82540) at /home/psergey/dev-git/5.5/sql/sql_parse.cc:5934


Note that thd_arg is passing around un-initialized data all of this time.

Is this how it is intended to work? 

This looks scary, a lot of functions are passed garbage as parameter. 

I assume the test suite passed with this patch, so these function don't do
anything with the parameter (they probably save away the THD pointer, and then
somebody assigns the pointer a valid value before it is used).

Still, this looks really scary, we need to either fix this (strongly
preferable), or add a lot of comments about it (acceptable as a last resort
measure).

>    if (unit->item)
>    {
> @@ -90,7 +92,7 @@ void Item_subselect::init(st_select_lex *select_lex,
>      engine= unit->item->engine;
>      own_engine= FALSE;
>      parsing_place= unit->item->parsing_place;
> -    thd->change_item_tree((Item**)&unit->item, this);
> +    unit->thd->change_item_tree((Item**)&unit->item, this);
>      engine->change_result(this, result, TRUE);
>    }
>    else
> @@ -220,6 +222,10 @@ bool Item_subselect::fix_fields(THD *thd_param, Item **ref)
>    uint8 uncacheable;
>    bool res;
>  
> +  thd= thd_param;
> +
> +  DBUG_ASSERT(unit->thd == thd);
> +
>    status_var_increment(thd_param->status_var.feature_subquery);
>  
>    DBUG_ASSERT(fixed == 0);
> @@ -2651,7 +2657,10 @@ bool Item_in_subselect::fix_fields(THD *thd_arg, Item **ref)
>  {
>    uint outer_cols_num;
>    List<Item> *inner_cols;
> -  char const *save_where= thd->where;
> +  char const *save_where= thd_arg->where;
> +
> +  thd= thd_arg;
> +  DBUG_ASSERT(unit->thd == thd);
>  
>    if (test_strategy(SUBS_SEMI_JOIN))
>      return !( (*ref)= new Item_int(1));
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

-- 
BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog