maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

Please find my input below.

I would like again to voice my dissatisfaction that I have to point out coding
style violations in pretty much every patch I get.

The other error in the patch is also pretty basic, ok to have it once but I
think it is reasonable to expect more polished patches to be submitted for code
review.

On Sat, May 20, 2017 at 01:50:42AM +0530, Varun wrote:
> revision-id: b400696386529fa63e79f723bbb06c05f7bae820 (mariadb-10.2.2-416-gb400696)
> parent(s): fff61e31eca83a0a9af7c30e2bcda8309e9c695a
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2017-05-20 01:49:34 +0530
> message:
> 
> MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
> was corrupted, server crashes in opt_sum_query
> 
> extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN
> 
> ---
>  mysql-test/r/innodb_ext_key.result | 45 ++++++++++++++++++++++++++++++++++++++
>  mysql-test/t/innodb_ext_key.test   | 31 ++++++++++++++++++++++++++
>  sql/table.cc                       | 36 +++++++++++++++++++++++++++++-
>  3 files changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result
> index 1305be8..66391d6 100644
> --- a/mysql-test/r/innodb_ext_key.result
> +++ b/mysql-test/r/innodb_ext_key.result
> @@ -1133,5 +1133,50 @@ where index_date_updated= 10 and index_id < 800;
>  id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>  1	SIMPLE	t2	range	index_date_updated	index_date_updated	13	NULL	#	Using index condition
>  drop table t0,t1,t2;
> +#
> +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
> +# was corrupted, server crashes in opt_sum_query
> +CREATE TABLE t1 (
> +pk INT,
> +f1 VARCHAR(3),
> +f2 VARCHAR(1024),
> +PRIMARY KEY (pk),
> +KEY(f2)
> +) ENGINE=InnoDB CHARSET utf8;
> +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def');
> +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4;
> +MAX(t2.pk)
> +2
> +drop table t1;
> +CREATE TABLE t1 (
> +pk1 INT,
> +pk2 INT,
> +f1 VARCHAR(3),
> +f2 VARCHAR(1021),
> +PRIMARY KEY (pk1,pk2),
> +KEY(f2)
> +) ENGINE=InnoDB CHARSET utf8;
> +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def');
> +explain format= json
> +select * from t1 force index(f2)  where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3';
> +EXPLAIN
> +{
> +  "query_block": {
> +    "select_id": 1,
> +    "table": {
> +      "table_name": "t1",
> +      "access_type": "range",
> +      "possible_keys": ["f2"],
> +      "key": "f2",
> +      "key_length": "3070",
> +      "used_key_parts": ["f2", "pk1"],
> +      "rows": 1,
> +      "filtered": 100,
> +      "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'",
> +      "attached_condition": "t1.f1 <= '3'"
> +    }
> +  }
> +}
> +drop table t1;
>  set optimizer_switch=@save_ext_key_optimizer_switch;
>  SET SESSION STORAGE_ENGINE=DEFAULT;
...

> diff --git a/sql/table.cc b/sql/table.cc
> index 398383e..7d88eba 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -2104,6 +2104,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
>      for (uint key=0 ; key < keys ; key++,keyinfo++)
>      {
>        uint usable_parts= 0;
> +      KEY* key_first_info;
This variable may be used un-initialized.  If I change the above line to be 
  KEY* key_first_info=(KEY*)0x1;

and run this statement:

CREATE TABLE t1a (
  pk INT,
  f1 VARCHAR(3),
  f2 VARCHAR(1024),
  f2a VARCHAR(1024),
  PRIMARY KEY (pk),
  KEY(f2),
  KEY(f2a)
) ENGINE=InnoDB CHARSET utf8;

Then I get a crash.

>        keyinfo->name=(char*) share->keynames.type_names[key];
>        keyinfo->name_length= strlen(keyinfo->name);
>        keyinfo->cache_name=
> @@ -2118,19 +2119,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
>                 keyinfo->name_length+1);
>        }
>  
> +      if(!key)
Coding style: 'if (' ...

> +        key_first_info= keyinfo;
> +
>        if (ext_key_parts > share->key_parts && key)
>        {
>          KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part +
>                                       (keyinfo-1)->ext_key_parts;
>          uint add_keyparts_for_this_key= add_first_key_parts;
> +        uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0;
> +        Field *field;
>  
>          /* 
>            Do not extend the key that contains a component
>            defined over the beginning of a field.
>  	*/ 
>          for (i= 0; i < keyinfo->user_defined_key_parts; i++)
> -	{
> +        {
Coding style: wrong identation.

>            uint fieldnr= keyinfo->key_part[i].fieldnr;
> +          field= share->field[keyinfo->key_part[i].fieldnr-1];
> +
> +          if (field->null_ptr)
> +            len_null_byte= HA_KEY_NULL_LENGTH;
> +
> +          if (field->type() == MYSQL_TYPE_BLOB ||
> +             field->real_type() == MYSQL_TYPE_VARCHAR ||
> +             field->type() == MYSQL_TYPE_GEOMETRY)
> +          {
> +            length_bytes= HA_KEY_BLOB_LENGTH;
> +          }
> +
> +          ext_key_length+= keyinfo->key_part[i].length + len_null_byte
> +                            + length_bytes;
>            if (share->field[fieldnr-1]->key_length() !=
>                keyinfo->key_part[i].length)
>  	  {
> @@ -2139,6 +2159,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
>            }
>          }
>  
> +          if (add_keyparts_for_this_key)
> +          {
> +            for (i= 0; i < add_keyparts_for_this_key; i++)
> +            {
> +              uint pk_part_length= key_first_info->key_part[i].store_length;
> +              if (ext_key_length + pk_part_length > MAX_KEY_LENGTH)
> +              {
> +                add_keyparts_for_this_key= i;
> +                break;
> +              }
> +              ext_key_length+= pk_part_length;
> +            }
> +          }
> +
>          if (add_keyparts_for_this_key < (keyinfo->ext_key_parts -
>                                          keyinfo->user_defined_key_parts))
>  	{

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