Valgrind warning in MyISAM in mysql-55-eb-blobs

[Philip Stoev]

The test case below causes the following warning to be produced. It seems that a blob field is required for the warning to occur. Maybe the INSERT ... SELECT statement is executed via a temporary MyISAM table, which in turn gets invalid data from the heap storage engine?

Warnings:

==22774== Thread 3:
==22774== Syscall param write(buf) points to uninitialised byte(s)
==22774== at 0x35A720E48D: ??? (in /lib64/libpthread-2.12.2.so)
==22774== by 0x8D717D: my_write (my_write.c:41)
==22774== by 0x8B900C: inline_mysql_file_write (mysql_file.h:1134)
==22774== by 0x8BC0E7: my_b_flush_io_cache (mf_iocache.c:1775)
==22774== by 0x8BC383: end_io_cache (mf_iocache.c:1846)
==22774== by 0x90E450: mi_extra (mi_extra.c:156)
==22774== by 0x8EC229: ha_myisam::extra(ha_extra_function) (ha_myisam.cc:1765)
==22774== by 0x64050A: do_select(JOIN*, List<Item>*, TABLE*, Procedure*) (sql_select.cc:11475)
==22774== by 0x6269B0: JOIN::exec() (sql_select.cc:1975)
==22774== by 0x628A86: mysql_select(THD*, Item***, TABLE_LIST*, unsigned int, List<Item>&, Item*, unsigned int, st_order*, st_order*, Item*, st_order*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*) (sql_select.cc:2575)
==22774== by 0x621093: handle_select(THD*, LEX*, select_result*, unsigned long) (sql_select.cc:297)
==22774== by 0x5F7835: mysql_execute_command(THD*) (sql_parse.cc:2844)
==22774== by 0x5FEDFA: mysql_parse(THD*, char*, unsigned int, Parser_state*) (sql_parse.cc:5503)
==22774== by 0x5F2D74: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1034)
==22774== by 0x5F1FD1: do_command(THD*) (sql_parse.cc:771)
==22774== by 0x6D7C7E: do_handle_one_connection(THD*) (sql_connect.cc:776)
==22774== Address 0x774fd18 is 8 bytes inside a block of size 131,072 alloc'd
==22774== at 0x4A0515D: malloc (vg_replace_malloc.c:195)
==22774== by 0x8D3379: my_malloc (my_malloc.c:38)
==22774== by 0x8B986C: init_io_cache (mf_iocache.c:232)
==22774== by 0x90E3B6: mi_extra (mi_extra.c:137)
==22774== by 0x8EC229: ha_myisam::extra(ha_extra_function) (ha_myisam.cc:1765)
==22774== by 0x640176: do_select(JOIN*, List<Item>*, TABLE*, Procedure*) (sql_select.cc:11410)
==22774== by 0x6269B0: JOIN::exec() (sql_select.cc:1975)
==22774== by 0x628A86: mysql_select(THD*, Item***, TABLE_LIST*, unsigned int, List<Item>&, Item*, unsigned int, st_order*, st_order*, Item*, st_order*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*) (sql_select.cc:2575)
==22774== by 0x621093: handle_select(THD*, LEX*, select_result*, unsigned long) (sql_select.cc:297)
==22774== by 0x5F7835: mysql_execute_command(THD*) (sql_parse.cc:2844)
==22774== by 0x5FEDFA: mysql_parse(THD*, char*, unsigned int, Parser_state*) (sql_parse.cc:5503)
==22774== by 0x5F2D74: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1034)
==22774== by 0x5F1FD1: do_command(THD*) (sql_parse.cc:771)
==22774== by 0x6D7C7E: do_handle_one_connection(THD*) (sql_connect.cc:776)
==22774== by 0x6D78C5: handle_one_connection (sql_connect.cc:724)
==22774== by 0x95B229: pfs_spawn_thread (pfs.cc:1015)

bzr version-info:

revision-id: <email address hidden>
date: 2011-05-27 13:09:02 +0400
build-date: 2011-05-27 16:18:16 +0300
revno: 3475
branch-nick: mysql-55-eb-blobs

test case:

CREATE TABLE IF NOT EXISTS t1 ( f1 VARCHAR ( 128 ) NOT NULL , f2 VARCHAR ( 128 ) NOT NULL DEFAULT 'uhopqdvzwlasckehliobscplb' , f3 VARBINARY ( 512 ) , f4 TEXT ( 65525 ) COLLATE utf8_bin NOT NULL , f5 VARCHAR ( 128 ) , KEY ( f1 ( 1 ) ) USING BTREE ) ENGINE=HEAP ;
INSERT IGNORE INTO t1 VALUES ( 'o' , REPEAT( 'fadihqxaombnrbrpvdhvcqoqdjbvbdbdlvnegcihplusssamauhopqdvzwlasc' , 7 ) , NULL , REPEAT('x', 81 * 1024) , 0 ) , ( NULL , REPEAT( 'lfadihqxaombnrbrpvdhvcqoqdjbvbdbdlvnegcihplusssamauhopqdvzwlasckeh' , 5 ) , REPEAT('x', 11 * 1024) , REPEAT( 'qlfadihqx' , 2 ) , 'f' ) , ( REPEAT('x', 886 * 1024) , NULL , NULL , NULL , REPEAT( 'rqlfadihqxaombnrbrpvdhvcqoqdjbvbdbdlvnegcihplusssamauhopqdvzwlasckehliobscplbwznryjnchnhaipdzeaeazyuuzmlnbby' , 5 ) ) , ( 1 , NULL , 'so' , 0 , 'what' );
INSERT IGNORE INTO t1 SELECT * FROM t1;

[Philip Stoev (pstoev-askmonty)]

Not reproducible if the table is MyISAM. Not reproducible if f4 is VARCHAR(255)

[Alexey Kopytov (akopytov)]

The Valgrind warning is actually safe to ignore in this case. The reason is how the fixed-size record prefix is handled by the dynamic rows patch.

First fixed-size columns and key columns are copied with a single memcpy() call when copying a record from the table record buffer and vice versa for performance reasons (see hp_extract_record() and hp_process_record_data_to_chunkset()). Which means if there are keys on VARCHAR fields, all bytes are stored into the storage engine (and then read back in) even if the actual data is shorter. Which in turn means some uninitialized bytes from the table record buffer may be stored.

When such records are then read from the HEAP storage engine and written to a disk-based table (for example, when creating an internal MyISAM temporary table), those bytes will be stored on disk as well. They will never be accessed, since both the server and storage engine know the actual length of VARCHAR values. But Valgrind was unhappy about uninitialized bytes being written to disk.

One solution to this would be to remove the optimization from the HEAP engine and copy fixed-size fields one by one rather than with a single memcpy() call. Which would imply a performance overhead for every record.

Another solution, which I like more, is to initialize the table record buffer with zeroes right after allocating it, so that no uninitialized data is ever passed to a storage engine. Which only means a very minor performance overhead when opening the table.

[Alexey Kopytov (akopytov)]

Reduced test case:

DROP TABLE t1;
CREATE TABLE IF NOT EXISTS t1 ( f1 VARCHAR ( 128 ), f2 VARCHAR ( 128 ) , f3 VARBINARY ( 512 ) , f4 TEXT ( 65525 ) , f5 VARCHAR ( 128 ) , KEY ( f1 ( 1 ) )) ENGINE=HEAP ;
INSERT IGNORE INTO t1 VALUES
( 'o' , "" , NULL , "" , 0 ) ,
(NULL, "" , "" , "" , 'f' ) ;
INSERT IGNORE INTO t1 SELECT * FROM t1;

[Stewart Smith (stewart)]

On Sat, 28 May 2011 14:29:06 -0000, Alexey Kopytov <email address hidden> wrote:
> Another solution, which I like more, is to initialize the table record
> buffer with zeroes right after allocating it, so that no uninitialized
> data is ever passed to a storage engine. Which only means a very minor
> performance overhead when opening the table.

We would have had to fix this forever ago in Drizzle... and we likely
did it that way.

--
Stewart Smith

[Laurynas Biveinis (laurynas-biveinis)]

Maybe it is possible to mark those bytes with Valgrind client access request as defined?

[Alexey Kopytov (akopytov)]

It turns out the bug is not repeatable on a valgrind-enabled build. Going to revert my fix and close the bug as invalid. Will reopen if the bug proves to be repeatable with a valgrind build.

[Stewart Smith (stewart)]

On Mon, 30 May 2011 09:15:47 -0000, Alexey Kopytov <email address hidden> wrote:
> It turns out the bug is not repeatable on a valgrind-enabled build.
> Going to revert my fix and close the bug as invalid. Will reopen if the
> bug proves to be repeatable with a valgrind build.

It's been a while for me, but I wonder what the different options are
for valgrind build? Maybe there is code in there to zero out the row
buffer (but built only for valgrind builds)?

--
Stewart Smith

[Alexey Kopytov (akopytov)]

On 31.05.11 9:22, Stewart Smith wrote:
> On Mon, 30 May 2011 09:15:47 -0000, Alexey Kopytov <email address hidden> wrote:
>> It turns out the bug is not repeatable on a valgrind-enabled build.
>> Going to revert my fix and close the bug as invalid. Will reopen if the
>> bug proves to be repeatable with a valgrind build.
>
> It's been a while for me, but I wonder what the different options are
> for valgrind build? Maybe there is code in there to zero out the row
> buffer (but built only for valgrind builds)?
>

Basically building with compile-pentium-valgrind-max defines HAVE_purify
among other things.

The record buffer did look like zeroed out when examined in a debugger,
but I could not find a quick way to verify that originally, and I didn't
want to spend too much time on it, because I knew the warning was fake.

I have now taken a closer look and found this code right next to the
code allocating the record buffer in sql/table.cc:

#ifdef HAVE_purify
  /*
    We need this because when we read var-length rows, we are not updating
    bytes after end of varchar
  */
  if (records > 1)
  {
    memcpy(outparam->record[0], share->default_values,
share->rec_buff_length);
    memcpy(outparam->record[1], share->default_values, share->null_bytes);
    if (records > 2)
      memcpy(outparam->record[1], share->default_values,
             share->rec_buff_length);
  }
#endif

So yes, it is initialized with default values (which apparently have
zeros for unused bytes in variable-length fields) in valgrind builds.

[Roel Van de Paar (roel11)]

See bug 1083942 to include a suppression