maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Oleg,

> commit 4f41909c3ee1d7e42c56947c9a9156fb78c22d8a
> Author: Oleg Smirnov <olernov@xxxxxxxxx>
> Date:   Fri Mar 11 21:18:34 2022 +0700
>
>     MDEV-24560 SIGSEGV in st_join_table::cleanup
>
>     If JOIN::create_postjoin_aggr_table encounters errors during execution
>     then free_tmp_table() is then called twice for JOIN_TAB::aggr.
>     The solution is to initialize JOIN_TAB::aggr and JOIN_TAB::table only
>     on successful completion of JOIN::create_postjoin_aggr_table
>

> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index c044656e916..8c07014d981 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -4037,10 +4037,6 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields,
>                !tables_list);
>    if (tab > join_tab)
>      (tab - 1)->next_select= sub_select_postjoin_aggr;
> -  if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab)))
> -    goto err;
> -  tab->table= table;
> -  table->reginfo.join_tab= tab;
>
>    /* if group or order on first table, sort first */
>    if ((group_list && simple_group) ||
> @@ -4089,7 +4085,10 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields,
>        order= NULL;
>      }
>    }
> -
> +  if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab)))
> +    goto err;
> +  tab->table= table;
> +  table->reginfo.join_tab= tab;
>    DBUG_RETURN(false);

I see a potential issue in the patch:

The moved code does this assignment:

>  tab->table= table;

Let's look inside this if-branch:

>>  if ((group_list && simple_group) ||
>>      (implicit_grouping && select_lex->have_window_funcs()))

The code inside if() { ... } part seems to be fine: it does not touch the contents 
of 'tab', in particular tab->table.

Let's look at the coed inside the else {... } part:

It has this condition:

>>   if (!group_list && !table->distinct && order && simple_order &&
>>       tab == join_tab + const_tables)
>>   {

Note that tab==join_tab+const_tables.

Then, a few lines below:

>>          add_sorting_to_table(join_tab + const_tables, order))

This is essentially add_sorting_to_table(tab, order).

Inside JOIN::add_sorting_to_table one can find this:

  TABLE *table= tab->table;
  if ((tab == join_tab + const_tables) &&
       table->pos_in_table_list &&
       table->pos_in_table_list->is_sjm_scan_table())

that is, we access tab->table and tab->table->pos_in_table_list. Which are not assigned yet 
(because your patch moves the assignment to happen later).

I've tried to construct an example to demonstrate this issue, but was not successful.
I've made this change:

diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index c044656e916..5a9a7d7e73f 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -4080,6 +4080,8 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields,
         tab == join_tab + const_tables)
     {
       DBUG_PRINT("info",("Sorting for order"));
+      fprintf(stderr,"AAABBBCCC:%s\n", thd->query());
+      DBUG_ASSERT(0);
       THD_STAGE_INFO(thd, stage_sorting_for_order);
 
       if (ordered_index_usage != ordered_index_order_by &&

and ran the entire testsuite with it. There were no failures.

I'm not sure if this is deadcode or we just don't have enough test coverage.

I suggest to assume the latter and change the patch to avoid this issue.


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