Bug #59894 set optimizer_switch to e or d causes invalid memory writes/valgrind warnings
Submitted: 2 Feb 2011 13:58 Modified: 21 Apr 2011 1:19
Reporter: Shane Bester (Platinum Quality Contributor) Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Options Severity:S1 (Critical)
Version:5.5.10, 5.6.2 OS:Any
Assigned to: Guilhem Bichot CPU Architecture:Any
Tags: regression, valgrind

[2 Feb 2011 13:58] Shane Bester
Description:
Version: '5.5.10-valgrind-max-debug'  socket: 'sock'  port: 3306  Source distribution
Thread 17:
Invalid write of size 1
at: strcpy (mc_replace_strmem.c:311)
by: find_type (typelib.c:120)
by: parse_name (typelib.c:279)
by: find_set_from_flags (typelib.c:338)
by: Sys_var_flagset::do_check (sys_vars.h:951)
by: sys_var::check(THD*, set_var*) (set_var.cc:226)
by: set_var::check(THD*) (set_var.cc:626)
by: sql_set_variables (set_var.cc:570)
by: mysql_execute_command (sql_parse.cc:3053)
by: mysql_parse (sql_parse.cc:5509)
by: dispatch_command (sql_parse.cc:1035)
by: do_command((sql_parse.cc:772)
by: do_handle_one_connection (sql_connect.cc:748)
by: handle_one_connection (sql_connect.cc:684)
by: start_thread (pthread_create.c:301)

 Address 0xf767362 is 0 bytes after a block of size 18 alloc'd
at: malloc (vg_replace_malloc.c:195)
by: my_malloc (my_malloc.c:38)
by: alloc_root (my_alloc.c:166)
by: Query_arena::alloc (sql_class.h:670)
by: get_text (sql_lex.cc:636)
by: lex_one_token (sql_lex.cc:1361)
by: MYSQLlex (sql_lex.cc:875)
by: MYSQLparse (sql_yacc.cc:16774)
by: parse_sql (sql_parse.cc:7234)
by: mysql_parse (sql_parse.cc:5464)
by: dispatch_command (sql_parse.cc:1035)
by: do_command (sql_parse.cc:772)
by: do_handle_one_connection (sql_connect.cc:748)
by: handle_one_connection (sql_connect.cc:684)
by: start_thread (pthread_create.c:301)

How to repeat:
#these both trigger valgrind problems:

set global optimizer_switch = 'd';
set global optimizer_switch = 'e';
[2 Feb 2011 14:16] Valeriy Kravchuk
Verified on Mac OS X:

macbook-pro:trunk openxs$ bin/mysql -uroot test
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 16
Server version: 5.6.2-m5-debug Source distribution

Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights reserved.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> set global optimizer_switch = 'd';
Query OK, 0 rows affected (0.02 sec)

mysql> set global optimizer_switch = 'e';
ERROR 2013 (HY000): Lost connection to MySQL server during query
mysql> 110202 16:14:35 mysqld_safe mysqld restarted

mysql> exit
Bye
macbook-pro:trunk openxs$ tail -80 data/macbook-pro.err 
110131 16:17:10 [ERROR] Native table 'performance_schema'.'table_lock_waits_summary_by_table' has the wrong structure
110131 16:17:10 [Note] Event Scheduler: Loaded 0 events
110131 16:17:10 [Note] /Users/openxs/dbs/trunk/bin/mysqld: ready for connections.
Version: '5.6.2-m5-debug'  socket: '/tmp/mysql.sock'  port: 3306  Source distribution
110202 16:14:34 - mysqld got signal 11 ;
This could be because you hit a bug. It is also possible that this binary
or one of the libraries it was linked against is corrupt, improperly built,
or misconfigured. This error can also be caused by malfunctioning hardware.
We will try our best to scrape up some info that will hopefully help diagnose
the problem, but since we have already crashed, something is definitely wrong
and this may fail.

key_buffer_size=8388608
read_buffer_size=131072
max_used_connections=3
max_threads=151
thread_count=1
connection_count=1
It is possible that mysqld could use up to 
key_buffer_size + (read_buffer_size + sort_buffer_size)*max_threads = 337971 K
bytes of memory
Hope that's ok; if not, decrease some variables in the equation.

Thread pointer: 0x1152800
Attempting backtrace. You can use the following information to find out
where mysqld died. If you see no messages after this, something went
terribly wrong...
stack_bottom = 0xb0800f30 thread_stack 0x30000
0   mysqld                              0x003aaed9 my_print_stacktrace + 44
1   mysqld                              0x001037e2 handle_segfault + 884
2   libSystem.B.dylib                   0x940472bb _sigtramp + 43
3   ???                                 0xffffffff 0x0 + 4294967295
4   mysqld                              0x00197437 _ZN11Query_arena10free_itemsEv + 79
5   mysqld                              0x0019750d _ZN3THD19cleanup_after_queryEv + 169
6   mysqld                              0x001de48e _Z11mysql_parseP3THDPcjP12Parser_state + 878
7   mysqld                              0x001def6c _Z16dispatch_command19enum_server_commandP3THDPcj + 2686
8   mysqld                              0x001e044c _Z10do_commandP3THD + 664
9   mysqld                              0x002c9257 _Z24do_handle_one_connectionP3THD + 1095
10  mysqld                              0x002c9345 handle_one_connection + 37
11  libSystem.B.dylib                   0x9400c095 _pthread_start + 321
12  libSystem.B.dylib                   0x9400bf52 thread_start + 34

Trying to get some variables.
Some pointers may be invalid and cause the dump to abort.
Query (0x115c810): set global optimizer_switch = 'e'
Connection ID (thread ID): 16
Status: NOT_KILLED
[9 Feb 2011 21:38] Bugs System
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/130928

3315 Guilhem Bichot	2011-02-09
      Fix for BUG#59894
      "set optimizer_switch to e or d causes invalid memory writes/valgrind warnings":
      due to prefix support, the argument "e" was overwritten with its full value
      "engine_condition_pushdown", which caused a buffer overrun.
      This was wrong usage of find_type(); other wrong usages are fixed here too.
      Start reading with the comment of typelib.c.
     @ client/mysqldump.c
        A bug: find_type() expects a bitmap as 3rd argument
        (each bit is a flag controlling a behaviour of the function);
        here it was instead passed the length of the string to search!
        That could give random behaviour of find_type()
        depending on the string.
        We rather need to pass a correct flag to find_type().
        The correct flag is 2 (i.e. don't copy the full name into buff,
        as we don't need to know the full name). Flag 8 is not needed
        as buff cannot have a comma (see how buff is filled).
        Flag 1 looks like a superfluous restriction.
        Flag 4 is not user-friendly (why use
        --compatible=2 rather than --compatible=mysql40 ?, and
        we probably not commit to "2" always meaning "mysql40"
        until the end of times).
     @ mysql-test/r/mysqldump.result
        correct result (see the two requested modes in SQL_MODE)
     @ mysql-test/suite/sys_vars/t/optimizer_switch_basic.test
        test for BUG#59894. The second SET used to crash.
     @ mysql-test/t/mysqldump.test
        we had no test for multiple modes in --compatible, which is
        supported according to --help
     @ mysys/typelib.c
        Fix for BUG#59894. parse_name() is asked to match "e" with a row
        of the TYPELIB (the TYPELIB lists permitted flags of optimizer_switch;
        and comes from optimizer_switch_names[] of sys_vars.cc).
        find_type() is capable of supporting prefixes, but if it is not
        passed flag 2 in third argument, it will overwrite its first
        argument (the string to search for) with the complete name,
        here overwriting "e" with "engine_condition_pushdown". But
        as this "e" was a buffer allocated in an Item, it was not big
        enough to host the longer name, thus the crash.
        The fix is to pass flag 2 to find_type(): we don't need
        to know the complete flag's name; the output used from find_type()
        is just the flag's number (== function's return code).
        With this and the other fixes in this patch, all usages of find_type()
        are now with flag 2, because in most usages the string to search for,
        is not guaranteed to be long enough to host the complete name
        (it is either directly from argv, or from alloc_root/my_malloc
        done in an earlier call).
        A more radical proposition for reviewers:
        see how parse_name() is fishy: it takes a const char**, and then
        the "const" is removed before calling find_type(). Because
        now find_type() is passed flag 2, this is "ok" (with flag 2,
        find_type() doesn't modify its input). The proposal is
        to change find_type() so that it always assumes flag 2, i.e.
        delete
          if (!(full_name & 2))
            (void) strmov(x,typelib->type_names[findpos]);
        from its code, and then change its input argument 'x' to
        be "const char*", and remove the const cast in parse_name(),
        and eliminate all "2" flags in callers of find_type().
        This way coders won't have to remember that they need
        to pass "2" or their data will change, which is expecially
        dangerous with --some_option=prefix1,prefix2
        (imagine prefix1 would be overwritten by the longer
        complete name: prefix2 would be destroyed...). Let
        me know your opinion.
     @ sql-common/client.c
        Two bugs:
        1) The enum was not in sync with the array (due to a bad porting of WL 1054;
        the extra OPT_ values are about options present in 5.1 and deleted in 5.5);
        added a compile_time_assert() to make sure this doesn't happen again
        2) find_type() was writing past the end of opt_arg; as opt_arg was allocated
        with alloc_root() with no extra space, this was an overrun; it could be seen
        when
        ** building with -DWITH_VALGRIND -DHAVE_purify -DEXTRA_DEBUG
        ** making execution go through the faulty code; this faulty
        code is executed only if the client asks to read a configuration
        file like this:
          mysql_options(mysql, MYSQL_READ_DEFAULT_FILE, "/tmp/cnf.cnf");
        so by adding such line to the start of mysql_client_test.c::client_connect(),
        we could see the valgrind warning:
        ==30548== Invalid write of size 1
        ==30548==    at 0x4C2624C: strcpy (mc_replace_strmem.c:303)
        ==30548==    by 0x48DC29: find_type (typelib.c:120)
        ==30548==    by 0x465686: mysql_read_default_options (client.c:1344)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        ==30548==  Address 0x61875ad is 0 bytes after a block of size 29 alloc'd
        ==30548==    at 0x4C25153: malloc (vg_replace_malloc.c:195)
        ==30548==    by 0x49BFF1: my_malloc (my_malloc.c:38)
        ==30548==    by 0x49C65C: alloc_root (my_alloc.c:166)
        ==30548==    by 0x48EF97: handle_default_option (default.c:381)
        ==30548==    by 0x49068C: search_default_file_with_ext (default.c:992)
        ==30548==    by 0x48F929: search_default_file (default.c:670)
        ==30548==    by 0x48EDC4: my_search_option_files (default.c:312)
        ==30548==    by 0x48F4B1: my_load_defaults (default.c:576)
        ==30548==    by 0x46517A: mysql_read_default_options (client.c:1207)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        This is fixed by passing flag "2" to find_type().
[10 Feb 2011 14:00] Bugs System
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/131044

3315 Guilhem Bichot	2011-02-10
      Fix for BUG#59894
      "set optimizer_switch to e or d causes invalid memory writes/valgrind warnings":
      due to prefix support, the argument "e" was overwritten with its full value
      "engine_condition_pushdown", which caused a buffer overrun.
      This was wrong usage of find_type(); other wrong usages are fixed here too.
      Please start reading with the comment of typelib.c.
     @ client/mysqldump.c
        A bug: find_type() expects a bitmap as 3rd argument
        (each bit is a flag controlling a behaviour of the function);
        here it was instead passed the length of the string to search!
        That could give random behaviour of find_type()
        depending on the string.
        We rather need to pass a correct flag to find_type().
        The correct flag is FIND_TYPE_BASIC (0).
        Flag 8 is not needed as buff cannot have a comma (see how buff is filled).
        Flag 1 looks like a superfluous restriction.
        Flag 4 is not user-friendly (why use
        --compatible=2 rather than --compatible=mysql40 ?, and
        we probably not commit to "2" always meaning "mysql40"
        until the end of times).
        Copyright: fix year and FSF address.
     @ include/mysql.h.pp
        This isn't a problematic API change as we go from char* to const char*:
        existing code will run unchanged.
     @ include/typelib.h
        named constants. Not an enum to not significantly change
        the declaration of find_type() which would be an API change
        (typelib.h is included in mysql.h).
     @ mysql-test/r/mysqldump.result
        correct result (see the two requested modes in SQL_MODE)
     @ mysql-test/suite/sys_vars/t/optimizer_switch_basic.test
        test for BUG#59894. The second SET used to crash.
     @ mysql-test/t/mysqldump.test
        we had no test for multiple modes in --compatible, which is
        supported according to --help
     @ mysys/typelib.c
        Fix for BUG#59894. parse_name() is asked to match "e" with a row
        of the TYPELIB (the TYPELIB lists permitted flags of optimizer_switch;
        and comes from optimizer_switch_names[] of sys_vars.cc).
        find_type() is capable of supporting prefixes, but if it is not
        passed flag 2 in third argument, it will overwrite its first
        argument (the string to search for) with the complete name,
        here overwriting "e" with "engine_condition_pushdown". But
        as this "e" was a buffer allocated in an Item, it was not big
        enough to host the longer name, thus the crash.
        We don't need to know the complete flag's name; the output used
        from find_type() is just the flag's number (== function's return
        code). So we can pass flag 2 to find_type() in parse_name().
        After doing this fix and the other fixes in this patch, all usages
        of find_type() were using flag 2; in most usages the string to search for,
        is not guaranteed to be long enough to host the complete name
        (it is either directly from argv, or from alloc_root/my_malloc
        done in an earlier call).
        Thus, flag 2 is here made implicit: callers need not pass it anymore,
        it is always automatically turned on.
        This allows to eliminate an oddity: parse_name() took a const char**,
        and then removed "const" before calling find_type(), which could
        theoretically modify the pointed data, thus lying on constness.
        Last, constants for find_type() are now named.
     @ sql-common/client.c
        Two bugs:
        1) The enum was not in sync with the array (due to a bad porting of WL 1054;
        the extra OPT_ values are about options present in 5.1 and deleted in 5.5);
        added a compile_time_assert() to make sure this doesn't happen again
        2) find_type() was writing past the end of opt_arg; as opt_arg was allocated
        with alloc_root() with no extra space, this was an overrun; it could be seen
        when
        ** building with -DWITH_VALGRIND -DHAVE_purify -DEXTRA_DEBUG
        ** making execution go through the faulty code; this faulty
        code is executed only if the client asks to read a configuration
        file like this:
          mysql_options(mysql, MYSQL_READ_DEFAULT_FILE, "/tmp/cnf.cnf");
        so by adding such line to the start of mysql_client_test.c::client_connect(),
        we could see the valgrind warning:
        ==30548== Invalid write of size 1
        ==30548==    at 0x4C2624C: strcpy (mc_replace_strmem.c:303)
        ==30548==    by 0x48DC29: find_type (typelib.c:120)
        ==30548==    by 0x465686: mysql_read_default_options (client.c:1344)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        ==30548==  Address 0x61875ad is 0 bytes after a block of size 29 alloc'd
        ==30548==    at 0x4C25153: malloc (vg_replace_malloc.c:195)
        ==30548==    by 0x49BFF1: my_malloc (my_malloc.c:38)
        ==30548==    by 0x49C65C: alloc_root (my_alloc.c:166)
        ==30548==    by 0x48EF97: handle_default_option (default.c:381)
        ==30548==    by 0x49068C: search_default_file_with_ext (default.c:992)
        ==30548==    by 0x48F929: search_default_file (default.c:670)
        ==30548==    by 0x48EDC4: my_search_option_files (default.c:312)
        ==30548==    by 0x48F4B1: my_load_defaults (default.c:576)
        ==30548==    by 0x46517A: mysql_read_default_options (client.c:1207)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        This is fixed by having find_type() not overwrite anymore.
     @ sql/sql_help.cc
        cast not needed anymore.
     @ sql/table.cc
        cast not needed anymore.
[10 Feb 2011 16:04] Bugs System
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/131063

3315 Guilhem Bichot	2011-02-10
      Fix for BUG#59894
      "set optimizer_switch to e or d causes invalid memory writes/valgrind warnings":
      due to prefix support, the argument "e" was overwritten with its full value
      "engine_condition_pushdown", which caused a buffer overrun.
      This was wrong usage of find_type(); other wrong usages are fixed here too.
      Please start reading with the comment of typelib.c.
     @ client/mysqldump.c
        A bug: find_type() expects a bitmap as 3rd argument
        (each bit is a flag controlling a behaviour of the function);
        here it was instead passed the length of the string to search!
        That could give random behaviour of find_type()
        depending on the string.
        We rather need to pass a correct flag to find_type().
        The correct flag is FIND_TYPE_BASIC (0).
        Flag 8 is not needed as buff cannot have a comma (see how buff is filled).
        Flag 1 looks like a superfluous restriction.
        Flag 4 is not user-friendly (why use
        --compatible=2 rather than --compatible=mysql40 ?, and
        we probably not commit to "2" always meaning "mysql40"
        until the end of times).
        Copyright: fix year and FSF address.
     @ include/mysql.h.pp
        This isn't a problematic API change as we go from char* to const char*:
        existing code will run unchanged.
     @ include/typelib.h
        named constants. Not an enum to not significantly change
        the declaration of find_type() which would be an API change
        (typelib.h is included in mysql.h).
     @ mysql-test/r/mysqldump.result
        correct result (see the two requested modes in SQL_MODE)
     @ mysql-test/suite/sys_vars/t/optimizer_switch_basic.test
        test for BUG#59894. The second SET used to crash.
     @ mysql-test/t/mysqldump.test
        we had no test for multiple modes in --compatible, which is
        supported according to --help
     @ mysys/typelib.c
        Fix for BUG#59894. parse_name() is asked to match "e" with a row
        of the TYPELIB (the TYPELIB lists permitted flags of optimizer_switch;
        and comes from optimizer_switch_names[] of sys_vars.cc).
        find_type() is capable of supporting prefixes, but if it is not
        passed flag 2 in third argument, it will overwrite its first
        argument (the string to search for) with the complete name,
        here overwriting "e" with "engine_condition_pushdown". But
        as this "e" was a buffer allocated in an Item, it was not big
        enough to host the longer name, thus the crash.
        We don't need to know the complete flag's name; the output used
        from find_type() is just the flag's number (== function's return
        code). So we can pass flag 2 to find_type() in parse_name().
        After doing this fix and the other fixes in this patch, all usages
        of find_type() were using flag 2; in most usages the string to search for,
        is not guaranteed to be long enough to host the complete name
        (it is either directly from argv, or from alloc_root/my_malloc
        done in an earlier call).
        Thus, flag 2 is here made implicit: callers need not pass it anymore,
        it is always automatically turned on.
        This allows to eliminate an oddity: parse_name() took a const char**,
        and then removed "const" before calling find_type(), which could
        theoretically modify the pointed data, thus lying on constness.
        Last, constants for find_type() are now named.
     @ sql-common/client.c
        Two bugs:
        1) The enum was not in sync with the array (due to a bad porting of WL 1054;
        the extra OPT_ values are about options present in 5.1 and deleted in 5.5);
        added a compile_time_assert() to make sure this doesn't happen again
        2) find_type() was writing past the end of opt_arg; as opt_arg was allocated
        with alloc_root() with no extra space, this was an overrun; it could be seen
        when
        ** building with -DWITH_VALGRIND -DHAVE_purify -DEXTRA_DEBUG
        ** making execution go through the faulty code; this faulty
        code is executed only if the client asks to read a configuration
        file like this:
          mysql_options(mysql, MYSQL_READ_DEFAULT_FILE, "/tmp/cnf.cnf");
        so by adding such line to the start of mysql_client_test.c::client_connect(),
        we could see the valgrind warning:
        ==30548== Invalid write of size 1
        ==30548==    at 0x4C2624C: strcpy (mc_replace_strmem.c:303)
        ==30548==    by 0x48DC29: find_type (typelib.c:120)
        ==30548==    by 0x465686: mysql_read_default_options (client.c:1344)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        ==30548==  Address 0x61875ad is 0 bytes after a block of size 29 alloc'd
        ==30548==    at 0x4C25153: malloc (vg_replace_malloc.c:195)
        ==30548==    by 0x49BFF1: my_malloc (my_malloc.c:38)
        ==30548==    by 0x49C65C: alloc_root (my_alloc.c:166)
        ==30548==    by 0x48EF97: handle_default_option (default.c:381)
        ==30548==    by 0x49068C: search_default_file_with_ext (default.c:992)
        ==30548==    by 0x48F929: search_default_file (default.c:670)
        ==30548==    by 0x48EDC4: my_search_option_files (default.c:312)
        ==30548==    by 0x48F4B1: my_load_defaults (default.c:576)
        ==30548==    by 0x46517A: mysql_read_default_options (client.c:1207)
        ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
        ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
        ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
        This is fixed by having find_type() not overwrite anymore.
     @ sql/sql_help.cc
        cast not needed anymore.
     @ sql/table.cc
        cast not needed anymore.
[21 Apr 2011 1:19] Paul Dubois
Noted in 5.5.10, 5.6.2 changelogs.

Setting the optimizer_switch system value to an invalid value caused
a server crash. 

CHANGESET - http://lists.mysql.com/commits/131063