MySQL Bugs: #57928: More Valgrind warnings in print

Bug #57928	More Valgrind warnings in print_keyuse() with --debug
Submitted:	2 Nov 2010 13:04	Modified:	11 Dec 2010 17:48
Reporter:	Guilhem Bichot	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Optimizer	Severity:	S3 (Non-critical)
Version:	next-mr-bugfixing	OS:	Linux (64bit)
Assigned to:	Guilhem Bichot	CPU Architecture:	Any

Description:
I'm using next-mr-bugfixing, revision-id:georgi.kodinov@oracle.com-20101028192516-18jzf6bf6qfpnplo
compiled with BUILD/compile-pentium64-valgrind-max .
Note that this tree contains the fix for BUG#56689.

Then I do
./mtr subquery_none --debug --valgrind
and I get several warnings, the first one being:

==23299== Thread 13:
==23299== Use of uninitialised value of size 8
==23299==    at 0x5B6FD13: (within /lib/libc-2.8.90.so)
==23299==    by 0x5B72F53: vfprintf (in /lib/libc-2.8.90.so)
==23299==    by 0x5B7B007: fprintf (in /lib/libc-2.8.90.so)
==23299==    by 0x687D04: print_keyuse(keyuse_t*) (sql_test.cc:262)
==23299==    by 0x687D9D: print_keyuse_array(st_dynamic_array*) (sql_test.cc:275)
==23299==    by 0x63FF23: _ZL19update_ref_and_keysP3THDP16st_dynamic_arrayP13st_join_tablejP4ItemP10COND_EQUALyP13st_select_lexPP17st_sargable_param (sql_select.cc:6247)
==23299==    by 0x6441A3: _ZL20make_join_statisticsP4JOINP10TABLE_LISTP4ItemP16st_dynamic_array (sql_select.cc:4669)
==23299==    by 0x645F6E: JOIN::optimize() (sql_select.cc:1961)
==23299==    by 0x64AB95: mysql_select(THD*, Item***, TABLE_LIST*, unsigned, List<Item>&, Item*, unsigned, st_order*, st_order*, Item*, st_order*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*) (sql_select.cc:3494)
==23299==    by 0x650824: handle_select(THD*, LEX*, select_result*, unsigned long) (sql_select.cc:322)
==23299==    by 0x5F1C53: _ZL21execute_sqlcom_selectP3THDP10TABLE_LIST (sql_parse.cc:4494)
==23299==    by 0x5F4179: mysql_execute_command(THD*) (sql_parse.cc:2092)
==23299==    by 0x5FB07D: mysql_parse(THD*, char*, unsigned, Parser_state*) (sql_parse.cc:5537)
==23299==    by 0x5FB8E2: dispatch_command(enum_server_command, THD*, char*, unsigned) (sql_parse.cc:1056)
==23299==    by 0x5FCDEE: do_command(THD*) (sql_parse.cc:796)
==23299==    by 0x6D4FD6: do_handle_one_connection(THD*) (sql_connect.cc:745)

How to repeat:
see description

Hello Miguel. I didn't wait for the test to finish: I started it, waited for a few minutes (10?), then looked at var/log/mysqld.1.trace, and the warning was there.

Thank you for the feedback.

The first query which gives a Valgrind warning uses a fulltext index. I see that it creates a KEYUSE object in add_ft_keys() ("ft"->fulltext); it initializes only certain members of this object (seems at least 3 of them are left to their default value i.e. undefined, random). Then the object is inserted into JOIN::keyuse, which print_keyuse_array() later prints.
Proposed fix:
- make KEYUSE a class (not a typedef'd struct)
- give it a constructor which takes many parameters: one per member of the class, and use each parameter to initialize the corresponding member; like:
KEYUSE::KEYUSE(TABLE *table_arg, Item *val_arg, etc).
- this constructor will have the consequence that this:
KEYUSE keyuse;
keyuse.x= y;
will not compile anymore; it will ensure that all members are initialized.
- KEYUSE objects are currently created at only 3 places; two such places will be fixed to use the new constructor; the third place ("key_end" in update_ref_and_keys()) will be replaced by a global object "key_end" initialized once and for all with all-0 members.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/122686

3330 Guilhem Bichot	2010-11-03
      Fix for BUG#57928 "More Valgrind warnings in print_keyuse() with --debug":
      for a fulltext index, the KEYUSE object had some uninitialized members.
     @ include/my_sys.h
        insert_dynamic() and set_dynamic() treat "element" as bytes and uses it as
        the source of memcpy(). As memcpy() accepts void* it sounds natural to
        use void* as type for "element". It then avoids casts in calling code.
        Before the change, we had to write:
        insert_dynamic(array, (uchar*)keyuse);
        because conversion from KEYUSE* to uchar* requires an explicit cast.
        After the change we can write
        insert_dynamic(array, keyuse);
        because conversion from KEYUSE* to void* is always allowed.
        If reviewers like this change, I will commit it separately from the bugfix
        and also remove all now-unneeded casts in calls to insert_dynamic()
        and set_dynamic().
     @ mysys/array.c
        void* is more natural.
     @ sql/sql_select.cc
        use constructor which sets all members. The second change is the bugfix: we forgot
        to set ref_table_rows, null_rejecting and cond_guard. Now there is the question
        of: what value to pick for them, for a fulltext index?
        * ref_table_rows: we now set it to ~(ha_rows)0 which seems to be a default
        value (see optimize_keyuse() and add_key_part()).
        * null_rejecting: it's not used for a fulltext index, and it serves
        only for the so-called early-NULL-filtering optimization, so we play
        safe and set it to "false"; setting it to "true" would require
        intelligent code determining when the optimization is allowed.
        * cond_guard: it's not used for a fulltext index (see create_ref_for_key()),
        so we set it to NULL.
     @ sql/sql_select.h
        constructor, comments. With that,
        "KEYUSE keyuse;" will not compile anymore.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

http://lists.mysql.com/commits/122862

3331 Guilhem Bichot 2010-11-04
Fix for BUG#57928 "More Valgrind warnings in print_keyuse() with --debug":
for a fulltext index, the KEYUSE object had some uninitialized members.
@ sql/sql_select.cc
use constructor which sets all members. The second change is the bugfix: we forgot
to set ref_table_rows, null_rejecting and cond_guard. Now there is the question
of: what value to pick for them, for a fulltext index?
* ref_table_rows: we now set it to ~(ha_rows)0 which seems to be a default
value (see optimize_keyuse() and add_key_part()).
* null_rejecting: it's not used for a fulltext index, and it serves
only for the so-called early-NULL-filtering optimization, so we play
safe and set it to "false"; setting it to "true" would require
intelligent code determining when the optimization is allowed.
* cond_guard: it's not used for a fulltext index (see create_ref_for_key()),
so we set it to NULL.
@ sql/sql_select.h
constructor, comments. With that,
"KEYUSE keyuse;" will not compile anymore.
KEYUSE is now a class so renamed to Key_use to follow our convention
for naming classes. As some lines are changed as a consequence,
we fix them to match the coding style (spacing etc).

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/123004

3277 Guilhem Bichot	2010-11-05
      Fix for BUG#57928 "More Valgrind warnings in print_keyuse() with --debug":
      for a fulltext index, the KEYUSE object had some uninitialized members.
     @ sql/sql_select.cc
        use constructor which sets all members. We forgot
        to set ref_table_rows, null_rejecting and cond_guard. Now there is the question
        of: what value to pick for them, for a fulltext index?
        * ref_table_rows: we now set it to ~(ha_rows)0 which seems to be a default
        value (see optimize_keyuse() and add_key_part()).
        * null_rejecting: it's not used for a fulltext index, and it serves
        only for the so-called early-NULL-filtering optimization, so we play
        safe and set it to "false"; setting it to "true" would require
        intelligent code determining when the optimization is allowed.
        * cond_guard: it's not used for a fulltext index (see create_ref_for_key()),
        so we set it to NULL.
     @ sql/sql_select.h
        constructor, comments. With that,
        "KEYUSE keyuse;" will not compile anymore.
        KEYUSE is now a class so renamed to Key_use to follow our convention
        for naming classes. As some lines are changed as a consequence,
        we fix them to match the coding style (spacing etc).

queued to next-mr-opt-backporting

Pushed into mysql-trunk 5.6.1 (revid:alexander.nozdrin@oracle.com-20101205122447-6x94l4fmslpbttxj) (version source revid:alexander.nozdrin@oracle.com-20101205122447-6x94l4fmslpbttxj) (merge vers: 5.6.1) (pib:23)

Bug does not appear in any released 5.6.x version. No changelog entry needed.