maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Monty,

Please find my comments below. Anything not commented on seems to be ok to me.

> commit 7d9c24f6790c66d2f457837661852bf5e870b605
> Author: Michael Widenius <monty@xxxxxxxxxxx>
> Date:   Tue Nov 14 07:47:58 2017 +0200
>
>   Handle failures from malloc
>   
>   Most "new" failures fixed in the following files:
>   - sql_select.cc
>   - item.cc
>   - opt_subselect.cc
>   


> diff --git a/sql/item.cc b/sql/item.cc
> index 2285ae28041..baae2a9b6fb 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -1261,7 +1261,7 @@ Item *Item::safe_charset_converter(THD *thd, CHARSET_INFO *tocs)
>    if (!needs_charset_converter(tocs))
>      return this;
>    Item_func_conv_charset *conv= new (thd->mem_root) Item_func_conv_charset(thd, this, tocs, 1);
> -  return conv->safe ? conv : NULL;
> +  return conv && conv->safe ? conv : NULL;
>  }
>  
I am not fully convinced that returning NULL from this function is called
everywhere.
It is of course better to return NULL and hope for the best than to crash right
away.

> @@ -3296,6 +3301,8 @@ void Item_field::fix_after_pullout(st_select_lex *new_parent, Item **ref,
>      }
>  
>      Name_resolution_context *ctx= new Name_resolution_context();
> +    if (ctx)
> +      return;                                   // Fatal error set
>      if (context->select_lex == new_parent)
>      {
>        /*

I assume this is a typo it should be "if (!ctx)"

> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index abbf2616537..7e42890143b 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -713,7 +713,7 @@ JOIN::prepare(TABLE_LIST *tables_init,
>    union_part= unit_arg->is_unit_op();
>  
>    if (select_lex->handle_derived(thd->lex, DT_PREPARE))
> -    DBUG_RETURN(1);
> +    DBUG_RETURN(-1);
>  
>    thd->lex->current_select->context_analysis_place= NO_MATTER;
>    thd->lex->current_select->is_item_list_lookup= 1;

I assume this is just for the sake of consistency with other DBUG_RETURN
statements in this function?

> @@ -14422,7 +14446,7 @@ static COND* substitute_for_best_equal_field(THD *thd, JOIN_TAB *context_tab,
>          This works OK with PS/SP re-execution as changes are made to
>          the arguments of AND/OR items only
>        */
> -      if (new_item != item)
> +      if (new_item && new_item != item)
>          li.replace(new_item);
>      }

new_item was returned from a substitute_for_best_equal_field() call. That one
doesn't ever return NULL. It will return without substitution if it encounters
an error.
So this change is not needed.
 
> @@ -25044,8 +25103,11 @@ int JOIN::save_explain_data_intern(Explain_query *output,
>  
>    if (message)
>    {
> -    explain= new (output->mem_root) Explain_select(output->mem_root, 
> -                                                   thd->lex->analyze_stmt);
> +    if (!(explain= new (output->mem_root)
> +          Explain_select(output->mem_root,
> +                         thd->lex->analyze_stmt)))
> +      DBUG_RETURN(1);
> +
>      join->select_lex->set_explain_type(true);
>  
>      explain->select_id= join->select_lex->select_number;

The return value of this function is not checked.
I assume the above statement is there so that we dont try to continue to
execute the function if we've got an OOM error?

In this case, probably JOIN::save_explain_data_intern() should also have a
similar check for its 

      tab->save_explain_data(eta, used_tables, distinct_arg, first_top_tab);

call?
 

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