maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

On May 08, Alexey Botchkov wrote:
> revision-id: eb3e9cdbe86b5a68c3ec8a9f26165259e4ca7eec (mariadb-10.3.6-125-geb3e9cd)
> parent(s): bd09c5ca866e273b6cebbd9a15c51c82bfa3ac9b
> committer: Alexey Botchkov
> timestamp: 2018-05-08 14:19:55 +0400
> message:
> 
> MDEV-15813 ASAN use-after-poison in hp_hashnr upon HANDLER READ on a versioned HEAP table.
> 
> Returns an error if we try to use a key in a query it declared unable to
> handle.
> 
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index c55ac4f..7446a60 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7915,3 +7915,5 @@ ER_UPDATED_COLUMN_ONLY_ONCE
>          eng "The column %`s.%`s cannot be changed more than once in a single UPDATE statement"
>  ER_EMPTY_ROW_IN_TVC
>          eng "Row with no elements is not allowed in table value constructor in this context"
> +ER_KEY_DOESNT_SUPPORT
> +        eng "%s index %`s does not support this operation"

I think this can be safely pushed into 10.2.

But not earlier, we can only add new error messages in the latest GA
version, not in previos GA's.

> diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc
> index 09883d8..12e4b5a 100644
> --- a/sql/sql_handler.cc
> +++ b/sql/sql_handler.cc
> @@ -661,12 +661,30 @@ mysql_ha_fix_cond_and_key(SQL_HANDLER *handler,
>        key_part_map keypart_map;
>        uint key_len;
>  
> +      if (ha_rkey_mode != HA_READ_KEY_EXACT &&
> +          (table->file->index_flags(handler->keyno, 0, TRUE) &
> +            (HA_READ_NEXT | HA_READ_PREV | HA_READ_RANGE)) == 0)

I suspect there can be hypothetical cases where an index supports some
specific ha_rkey_mode values and has some specific flags, but not all of
them, like you test here. But as the only real use case is HASH vs BTREE
now, your check is fine.

No need overengineer a complex test that we cannot possibly test now.

> +      {
> +        my_error(ER_KEY_DOESNT_SUPPORT, MYF(0),
> +                 table->file->index_type(handler->keyno), keyinfo->name);
> +        return 1;
> +      }
> +
>        if (key_expr->elements > keyinfo->user_defined_key_parts)
>        {
>          my_error(ER_TOO_MANY_KEY_PARTS, MYF(0),
>                   keyinfo->user_defined_key_parts);
>          return 1;
>        }
> +      else if (key_expr->elements < keyinfo->user_defined_key_parts &&
> +               (table->file->index_flags(handler->keyno, 0, TRUE) &
> +                HA_ONLY_WHOLE_INDEX))
> +      {
> +        my_error(ER_KEY_DOESNT_SUPPORT, MYF(0),
> +                 table->file->index_type(handler->keyno), keyinfo->name);
> +        return 1;

Good point!

> +      }

This is ok to push. Just two questions:

1. Why did you add it in sql_handler.cc and not in the class handler
   method, like ha_index_read_map or ha_index_next ?

2. Could you add tests for fulltext/rtree indexes too, please?

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx