[MDEV-8565] [PATCH] COLUMN_CHECK fails on valid data Created: 2015-07-31  Updated: 2015-11-23  Resolved: 2015-10-19

Status: Closed
Project: MariaDB Server
Component/s: Dynamic Columns
Affects Version/s: 10.0.20, 10.1.7, 10.1.8, 10.0.21-galera
Fix Version/s: 10.0.22, 10.1.9

Type: Bug Priority: Critical
Reporter: Vincent Milum Jr Assignee: Oleksandr Byelkin
Resolution: Fixed Votes: 0
Labels: None
Environment:

Server version: 10.0.20-MariaDB-1~wheezy-wsrep-log


Issue Links:
Relates
relates to MDEV-9167 [PATCH] COLUMN_CHECK fails on valid d... Closed

 Description   

COLUMN_CHECK fails on valid data

SELECT COLUMN_CHECK(COLUMN_CREATE('a',0,'b','1'))
Result: 0 (failed)
 
SELECT COLUMN_CHECK(COLUMN_CREATE('a',1,'b','1'))
Result: 1 (correct)
 
SELECT COLUMN_JSON(COLUMN_CREATE('a',0,'b','1'))
Result: {"a":0,"b":"1"} (correct)
 
SELECT COLUMN_JSON(COLUMN_CREATE('a',1,'b','1'))
Result: {"a":1,"b":"1"} (correct)

It appears that COLUMN_CHECK fails when one of the dynamic column values is set to INTEGER 0, and only when there is more than 1 dynamic column present. COLUMN_JSON is able to parse the results properly, so the returned values from COLUMN_CREATE appears to be correct.



 Comments   
Comment by Elena Stepanova [ 2015-07-31 ]

Thanks for the report.

Comment by Vincent Milum Jr [ 2015-08-09 ]

SELECT
COLUMN_CHECK(COLUMN_CREATE(1,0)) AS 'r0',
COLUMN_CHECK(COLUMN_CREATE(1,1)) AS 'r1',
COLUMN_CHECK(COLUMN_CREATE(1,0,2,1)) AS 'r2',
COLUMN_CHECK(COLUMN_CREATE(1,1,2,0)) AS 'r3',
COLUMN_CHECK(COLUMN_CREATE(1,'',2,'x')) AS 'r4',
COLUMN_CHECK(COLUMN_CREATE(1,'x',2,'')) AS 'r5',
COLUMN_CHECK(COLUMN_CREATE(1,0,2,1,3,1,4,1)) AS 'r6',
COLUMN_CHECK(COLUMN_CREATE(1,1,2,0,3,1,4,1)) AS 'r7',
COLUMN_CHECK(COLUMN_CREATE(1,1,2,1,3,0,4,1)) AS 'r8',
COLUMN_CHECK(COLUMN_CREATE(1,1,2,1,3,1,4,0)) AS 'r9';
 
+------+------+------+------+------+------+------+------+------+------+
| r0   | r1   | r2   | r3   | r4   | r5   | r6   | r7   | r8   | r9   |
+------+------+------+------+------+------+------+------+------+------+
|    1 |    1 |    0 |    1 |    1 |    1 |    0 |    0 |    0 |    1 |
+------+------+------+------+------+------+------+------+------+------+

The error only occurs when the ZERO value is not in the first column. I think I found where this is occurring, too. I've only read over the code, but I don't have an active compile environment to this out though.

https://github.com/MariaDB/server/blob/10.1/mysys/ma_dyncol.c - Line 3722:

    if (prev_type != DYN_COL_NULL)
    {
      /* It is not first entry */
      if (prev_data_offset >= data_offset)
      {
        DBUG_PRINT("info", ("Field order: %u  Previous data offset: %u"
                            " >= Current data offset: %u",
                            (uint)i,
                            (uint)prev_data_offset,
                            (uint)data_offset));
        goto end;
      }

Looking at just the raw binary data from a few of these, a couple points instantly stand out. First, when integer ZERO is saved, it is saved as zero-length in the HEADER section and no data written at all in the DATA section. Then when COLUMN_CHECK is performed, is specifically checks to ensure that the length in the HEADER section is greater than zero bytes. Here is the raw HEX output of the last four items tested above. (note: added spaces to separate out sections)

000400  010000 020000 030008 040010 02 02 02
000400  010000 020008 030008 040010 02 02 02
000400  010000 020008 030010 040010 02 02 02
000400  010000 020008 030010 040018 02 02 02

1) column 1 header is bypassed (first), and then column 2 header is checked. column 1 and 2 data offsets compared = 0 length, fails.
2) column 2 header is checked. then column 3 header is checked. column 2 and 3 data offsets compared = 0 length, fails.
3) column 3 header is checked. then column 4 header is checked. column 3 and 4 data offsets compared = 0 length, fails.
4) column 4 header is checked. it has a data offset at the very end of the data section (beyond last byte of data), so it succeeds.

In the 4th condition, the data is then passed along to dynamic_column_uint_read() with a ZERO byte length. This particular function doesn't have a failure case at all, and will always return ER_DYNCOL_OK. This function also defaults to storing a value of ZERO before attempting to read from the data section. With a zero byte length, nothing is read, so this ZERO value is returned (and why the function still works with COLUMN_JSON and other functions that attempt to read the column data)

https://github.com/MariaDB/server/blob/10.1/mysys/ma_dyncol.c - Line 838:

static enum enum_dyncol_func_result
dynamic_column_uint_read(DYNAMIC_COLUMN_VALUE *store_it_here,
                         uchar *data, size_t length)
{
  ulonglong value= 0;
  size_t i;
 
  for (i= 0; i < length; i++)
    value+= ((ulonglong)data[i]) << (i*8);
 
  store_it_here->x.ulong_value= value;
  return ER_DYNCOL_OK;
}

So one of two solutions should happen here. Either

1) When integer values of ZERO are stored, they should actually be stored as 0x00 within the data section, with an appropriate length stored in the header. (looking at the code, it may be intentional to NOT store integer ZERO for space saving reasons tho)

OR

2) Update the check for zero-length data section to specifically support type integer (signed or unsigned) to not error out, and treat this as an intended condition.

Comment by Sergei Golubchik [ 2015-08-21 ]

pull request with a fix: https://github.com/MariaDB/server/pull/90

Comment by Vincent Milum Jr [ 2015-10-17 ]

As a note: This bug is STILL present in the 10.1 "Stable" release, despite there being a pull request which fixes this bug for a couple months now.

Comment by Oleksandr Byelkin [ 2015-10-19 ]

I put a bit different check: zero size can be int and uint and not other (except NULL, but it is processed).

Comment by Oleksandr Byelkin [ 2015-10-19 ]

Thank you a lot! I pushed it with changed check (more strict and takes into account almost impossible case of uint) and test suite.

Generated at Thu Feb 08 07:28:06 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.