maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

On Mon, Nov 05, 2018 at 07:36:17PM +0530, Varun wrote:
> revision-id: 0187bb03884a34ca8de96e47f4cb1f3d860e6db9 (mariadb-10.2.16-224-g0187bb03884)
> parent(s): 8a346f31b913daa011085afec2b2d38450c73e00
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2018-11-05 19:32:26 +0530
> message:
> 
> MDEV_-17589: Stack-buffer-overflow with indexed varchar (utf8) field
> 
> Create a new constant MAX_DATA_LENGTH_FOR_KEY.
> Replace the value of MAX_KEY_LENGTH to also include the LENGTH and NULL BYTES.
> 
...

> diff --git a/mysql-test/r/partition_datatype.result b/mysql-test/r/partition_datatype.result
> index 2e518c194f0..66d003c97cb 100644
> --- a/mysql-test/r/partition_datatype.result
> +++ b/mysql-test/r/partition_datatype.result
> @@ -314,7 +314,7 @@ bbbb
>  drop table t1;
>  set sql_mode='';
>  create table t1 (a varchar(3070)) partition by key (a);
> -ERROR HY000: The total length of the partitioning fields is too large
> +drop table t1;

I don't think it's a good idea that after this patch we allow the table
definitions that we didn't allow before. This raises the question of
datadir downgrades, etc.
I think we should only remove the buffer overrun.
Here's patch how to avoid the above:
https://gist.github.com/spetrunia/7e7e3fcc60a81a6cb854e77c12c367d3
>  create table t1 (a varchar(65532) not null) partition by key (a);
>  ERROR HY000: The total length of the partitioning fields is too large
>  create table t1 (a varchar(65533)) partition by key (a);
...

> diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result
> index c55e8d138f8..c76c8613c57 100644
> --- a/mysql-test/r/innodb_ext_key.result
> +++ b/mysql-test/r/innodb_ext_key.result
> @@ -1168,8 +1168,8 @@ EXPLAIN
>        "access_type": "range",
>        "possible_keys": ["f2"],
>        "key": "f2",
> -      "key_length": "3070",
> -      "used_key_parts": ["f2", "pk1"],
> +      "key_length": "3074",
> +      "used_key_parts": ["f2", "pk1", "pk2"],

Same reaction as above:
This change shows that "Extended keys" feature now allows bigger set of key
extensions than before. Can we avoid making this change by changing
MAX_KEY_LENGTH to MAX_DATA_LENGTH_FOR_KEY somewhere?

>        "rows": 1,
>        "filtered": 100,
>        "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'",

> diff --git a/sql/handler.h b/sql/handler.h
> index ed2ef822c88..75f72bdfb53 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -378,6 +378,10 @@ enum enum_alter_inplace_result {
>  #define HA_KEY_NULL_LENGTH	1
>  #define HA_KEY_BLOB_LENGTH	2
>  
> +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \
> +                         +(MAX_REF_PARTS \
> +                          *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH)))
> +
I think this requires a comment. Please add something like

/* Maximum length of any index lookup key, in bytes */

>  #define HA_LEX_CREATE_TMP_TABLE	1U
>  #define HA_CREATE_TMP_ALTER     8U
>  
> @@ -3421,14 +3425,14 @@ class handler :public Sql_alloc
>    uint max_key_parts() const
>    { return MY_MIN(MAX_REF_PARTS, max_supported_key_parts()); }
>    uint max_key_length() const
> -  { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_length()); }
> +  { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_length()); }
>    uint max_key_part_length() const
> -  { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_part_length()); }
> +  { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_part_length()); }
>  
>    virtual uint max_supported_record_length() const { return HA_MAX_REC_LENGTH; }
>    virtual uint max_supported_keys() const { return 0; }
>    virtual uint max_supported_key_parts() const { return MAX_REF_PARTS; }
> -  virtual uint max_supported_key_length() const { return MAX_KEY_LENGTH; }
> +  virtual uint max_supported_key_length() const { return MAX_DATA_LENGTH_FOR_KEY; }
>    virtual uint max_supported_key_part_length() const { return 255; }
>    virtual uint min_record_length(uint options) const { return 1; }
>  
> diff --git a/sql/sql_const.h b/sql/sql_const.h
> index 7395ae3c08a..0be63a2a5ad 100644
> --- a/sql/sql_const.h
> +++ b/sql/sql_const.h
> @@ -33,7 +33,7 @@
>  #define MAX_SYS_VAR_LENGTH 32
>  #define MAX_KEY MAX_INDEXES                     /* Max used keys */
>  #define MAX_REF_PARTS 32			/* Max parts used as ref */

Please add a comment, something like
/*
  Maximum length of the data part of an index lookup key. 
  
  The "data part" is defined as the value itself, not including the
  NULL-indicator bytes or varchar length bytes ("the Extras"). We need this 
  value because there was a bug where length of the Extras were not counted.

  You probably need MAX_KEY_LENGTH, not this constant.
*/

> -#define MAX_KEY_LENGTH 3072			/* max possible key */
> +#define MAX_DATA_LENGTH_FOR_KEY 3072
>  #if SIZEOF_OFF_T > 4
>  #define MAX_REFLENGTH 8				/* Max length for record ref */
>  #else

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