maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sachin!

I agree with the patch, I think it's correct.
Good tests too.

I wasn't able to understand the bug from the commit comment though,
see below.

Ok to push (preferrably with the improved commit message).

On Jan 19, Sachin Setiya wrote:
> Hi serg , please review the latest patch
> 
> commit d9641478bb8f6045039a3d942f2925ada334907e
> Author: Sachin Setiya <sachin.setiya@xxxxxxxxxx>
> Date:   Fri Jan 19 19:33:45 2018 +0530
> 
>     MDEV-14586 Assertion `0' failed in retrieve_auto_increment ...
> 
>     Problem:-
>      If we create table using myisam/aria then this crashes the server.
>       CREATE TABLE t1(a bit(1), b int auto_increment , index(a,b));
>       insert into t1 values(1,1);
>      Or this query
>       CREATE TABLE t1 (b BIT(1), pk INTEGER AUTO_INCREMENT PRIMARY KEY);
>       ALTER TABLE t1 ADD INDEX(b,pk);
>       INSERT INTO t1 VALUES (1,b'1');
>       ALTER TABLE t1 DROP PRIMARY KEY;
> 
>     Reason:-
>      The reason for this is
>      1st- Since bit field is stored in record null bits itself , so
>       find_ref_key in open_binary_frm will wrongly set share->next_number_key_offset
>       and share->next_number_keypart (both set to 0).

I would say that

  find_ref_key() finds what key an auto_increment field belongs to by
  comparing key_part->offset and field->ptr. But BIT fields might have
  zero length in the record, so a key might have many key parts with the
  same offset. That is, comparing offsets cannot uniquely identify the
  correct key part.

>      2nd- Since next_number_key_offset is zero it myisam/aria will think that
>       auto_increment is in first part of key.
>      3nd- myisam/aria will call retrieve_auto_key which will see first key_part
>       field as a bit field and call assert(0)
> 
>     Solution:-

  Many key parts might have the same offset, but BIT fields do not
  support auto_increment. So, we can skip all key parts over BIT fields,
  and then comparing offsets will be unambiguous.

>      Although the bit field is stored in record null_byte but in key file(.MYI)
>      this does not hold true. Bit(1-7) will take one byte of memory. So the correct
>      approach will be to never call retrieve_auto_increment in the case when
>      bit field is in the starting of index. So I have changed the find_ref_key
>      function to take account that if first N fields is Bit field then simply go
>      to next field which is not Bit field.
> 
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx