maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

The patch is ok to push.

One last question: why is it made against 10.2? I see the MDEV issue has
AffectsVersion to start from 10.2, but to my knowledge the code and constants 
involved were not changed in 10.2, so the issue should be present in the
earlier versions as well?


On Mon, Nov 12, 2018 at 04:27:08PM +0530, Varun wrote:
> revision-id: a9c1420233f3f65aee00a8e7bdd1c8481b0bbc37 (mariadb-10.2.18-74-ga9c1420233f)
> parent(s): b290ef8c76e2d7dfbae7a85766694a6fd4648eac
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2018-11-12 16:25:25 +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.
> 
> ---
>  mysql-test/r/func_group_innodb.result | 23 +++++++++++++++++++++++
>  mysql-test/t/func_group_innodb.test   | 18 ++++++++++++++++++
>  sql/handler.h                         | 12 +++++++++---
>  sql/partition_info.cc                 |  4 ++--
>  sql/sql_const.h                       | 13 ++++++++++++-
>  sql/table.cc                          |  2 +-
>  6 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/mysql-test/r/func_group_innodb.result b/mysql-test/r/func_group_innodb.result
> index 52d5922df95..c2bfe9bf81c 100644
> --- a/mysql-test/r/func_group_innodb.result
> +++ b/mysql-test/r/func_group_innodb.result
> @@ -246,4 +246,27 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b;
>  id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>  1	SIMPLE	t1	range	NULL	b	263	NULL	3	Using index for group-by
>  DROP TABLE t1;
> +#
> +# MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field
> +#
> +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8;
> +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go');
> +explain
> +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu';
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Select tables optimized away
> +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu';
> +MIN(t1.v1)
> +king
> +drop table t1;
> +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB;
> +INSERT INTO t1 VALUES ('king'), ('bad');
> +explain
> +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	NULL	No matching min/max row
> +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x;
> +MIN(x.v1)
> +NULL
> +drop table t1;
>  End of 5.5 tests
> diff --git a/mysql-test/t/func_group_innodb.test b/mysql-test/t/func_group_innodb.test
> index c62d3d08496..0d24f37363b 100644
> --- a/mysql-test/t/func_group_innodb.test
> +++ b/mysql-test/t/func_group_innodb.test
> @@ -192,4 +192,22 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b;
>  
>  DROP TABLE t1;
>  
> +--echo #
> +--echo # MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field
> +--echo #
> +
> +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8;
> +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go');
> +explain
> +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu';
> +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu';
> +drop table t1;
> +
> +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB;
> +INSERT INTO t1 VALUES ('king'), ('bad');
> +explain
> +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x;
> +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x;
> +drop table t1;
> +
>  --echo End of 5.5 tests
> diff --git a/sql/handler.h b/sql/handler.h
> index ed2ef822c88..6745312a17d 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -378,6 +378,12 @@ enum enum_alter_inplace_result {
>  #define HA_KEY_NULL_LENGTH	1
>  #define HA_KEY_BLOB_LENGTH	2
>  
> +/* Maximum length of any index lookup key, in bytes */
> +
> +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \
> +                         +(MAX_REF_PARTS \
> +                          *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH)))
> +
>  #define HA_LEX_CREATE_TMP_TABLE	1U
>  #define HA_CREATE_TMP_ALTER     8U
>  
> @@ -3421,14 +3427,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/partition_info.cc b/sql/partition_info.cc
> index f96882552fa..8be3f878f8d 100644
> --- a/sql/partition_info.cc
> +++ b/sql/partition_info.cc
> @@ -1713,12 +1713,12 @@ bool partition_info::check_partition_field_length()
>  
>    for (i= 0; i < num_part_fields; i++)
>      store_length+= get_partition_field_store_length(part_field_array[i]);
> -  if (store_length > MAX_KEY_LENGTH)
> +  if (store_length > MAX_DATA_LENGTH_FOR_KEY)
>      DBUG_RETURN(TRUE);
>    store_length= 0;
>    for (i= 0; i < num_subpart_fields; i++)
>      store_length+= get_partition_field_store_length(subpart_field_array[i]);
> -  if (store_length > MAX_KEY_LENGTH)
> +  if (store_length > MAX_DATA_LENGTH_FOR_KEY)
>      DBUG_RETURN(TRUE);
>    DBUG_RETURN(FALSE);
>  }
> diff --git a/sql/sql_const.h b/sql/sql_const.h
> index 7395ae3c08a..0c781855302 100644
> --- a/sql/sql_const.h
> +++ b/sql/sql_const.h
> @@ -33,7 +33,18 @@
>  #define MAX_SYS_VAR_LENGTH 32
>  #define MAX_KEY MAX_INDEXES                     /* Max used keys */
>  #define MAX_REF_PARTS 32			/* Max parts used as ref */
> -#define MAX_KEY_LENGTH 3072			/* max possible key */
> +
> +/*
> +  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_DATA_LENGTH_FOR_KEY 3072
>  #if SIZEOF_OFF_T > 4
>  #define MAX_REFLENGTH 8				/* Max length for record ref */
>  #else
> diff --git a/sql/table.cc b/sql/table.cc
> index 2b278288784..5b163d501d1 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -2169,7 +2169,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
>              uint pk_part_length= key_first_info->key_part[i].store_length;
>              if (keyinfo->ext_key_part_map & 1<<i)
>              {
> -              if (ext_key_length + pk_part_length > MAX_KEY_LENGTH)
> +              if (ext_key_length + pk_part_length > MAX_DATA_LENGTH_FOR_KEY)
>                {
>                  add_keyparts_for_this_key= i;
>                  break;
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

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