| 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: | |
| 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
[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
