maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

Hi!

On Wed, Jul 29, 2020 at 2:39 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Oleksandr!
>
> On Jul 29, Oleksandr Byelkin wrote:
> > revision-id: 7311586ca25 (mariadb-10.2.31-317-g7311586ca25)
> > parent(s): a1e52e7f32c
> > author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> > committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> > timestamp: 2020-07-17 13:47:26 +0200
> > message:
> >
> > MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT
> DEFAULT ?)' USING DEFAULT
> >
> > Part 1: make better asserts.
> >
> > diff --git a/sql/field.cc b/sql/field.cc
> > index 65bd9d22857..cf0cc655888 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -11138,6 +11138,16 @@ bool Field::save_in_field_default_value(bool
> view_error_processing)
> >  {
> >    THD *thd= table->in_use;
> >
> > +  if ( // table is not opened properly
> > +      table == NULL || table->pos_in_table_list == NULL ||
> > +      table->pos_in_table_list->select_lex == NULL ||
> > +      // we are in subquery/derived/CTE
> > +
> !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> > +  {
> > +    DBUG_ASSERT(0); // shoud not happened
> > +    my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> > +    return false;
> > +  }
>
> that's kind of questionable. if this condition can never be true, why do
> you have my_error() there? Just do asserts, like
>
>   DBUG_ASSERT(table);
>   DBUG_ASSERT(table->pos_in_table_list);
>   DBUG_ASSERT(table->pos_in_table_list->select_lex);
>
> DBUG_ASSERT(table->pos_in_table_list->top_table()->select_lex->is_top_level_select());
>

It was the first solution to catch the problem on the low level, now we
catch it earlier and in many places, but if we forget something  on
non-debug builds it will just work.


>
> >    if (flags & NO_DEFAULT_VALUE_FLAG &&
> >        real_type() != MYSQL_TYPE_ENUM)
> >    {
> > @@ -11177,12 +11187,29 @@ bool Field::save_in_field_default_value(bool
> view_error_processing)
> >  bool Field::save_in_field_ignore_value(bool view_error_processing)
> >  {
> >    enum_sql_command com= table->in_use->lex->sql_command;
> > +
> > +  if ( // table is not opened properly
> > +      table == NULL || table->pos_in_table_list == NULL ||
> > +      table->pos_in_table_list->select_lex == NULL ||
> > +      // we are in subquery/derived/CTE
> > +
> !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> > +  {
> > +    DBUG_ASSERT(0); // shoud not happened
> > +    my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> > +    return false;
> > +  }
>
> ditto
>

the same as above


>
> >    // All insert-like commands
> >    if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE ||
> >        com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT ||
> >        com == SQLCOM_LOAD)
> >      return save_in_field_default_value(view_error_processing);
> > -  return 0; // ignore
> > +  if (com == SQLCOM_UPDATE || com == SQLCOM_UPDATE_MULTI)
> > +    return 0; // ignore
> > +
> > +  // unexpected command
> > +  DBUG_ASSERT(0); // shoud not happened
>
> ditto
>
> but why it should not happen? What if one does SELECT DEFAULT(f) FROM t?
>

we think that we caught all cases with:
1) calls of Item_(param|default|ignore)::val_*  (it is old way, was here
for long time)
2) checks Item::is_evaluable_expression() before save_in_fields() where it
is possible


>
> > +  my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> > +  return false;
> >  }
> >
> >
> > diff --git a/sql/item.cc b/sql/item.cc
> > index 9451d4203ca..382eb3ac47a 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -3930,13 +3930,19 @@ int Item_param::save_in_field(Field *field, bool
> no_conversions)
> >    case NULL_VALUE:
> >      return set_field_to_null_with_conversions(field, no_conversions);
> >    case DEFAULT_VALUE:
> > -    return
> field->save_in_field_default_value(field->table->pos_in_table_list->
> > +    return field->save_in_field_default_value((field->table &&
> > +
>  field->table->pos_in_table_list)?
>
> in what case can field->table be NULL here?
>

probably impossible (I thought it is for fake fields in SP, but it looks
like they have fake table also)


> > +
> field->table->pos_in_table_list->
> >                                                top_table() !=
> > -
> field->table->pos_in_table_list);
> > +
> field->table->pos_in_table_list:
> > +                                              FALSE);
> >    case IGNORE_VALUE:
> > -    return
> field->save_in_field_ignore_value(field->table->pos_in_table_list->
> > +    return field->save_in_field_ignore_value((field->table &&
> > +
>  field->table->pos_in_table_list)?
> > +
>  field->table->pos_in_table_list->
> >                                               top_table() !=
> > -
>  field->table->pos_in_table_list);
> > +
>  field->table->pos_in_table_list:
> > +                                             FALSE);
> >    case NO_VALUE:
> >      DBUG_ASSERT(0); // Should not be possible
> >      return true;
> >
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>