diff options
author | Guilhem Bichot <guilhem.bichot@oracle.com> | 2011-02-11 15:00:09 +0100 |
---|---|---|
committer | Guilhem Bichot <guilhem.bichot@oracle.com> | 2011-02-11 15:00:09 +0100 |
commit | 1756d087cd93dd7e4b4963b9b235daaf5279c8dc (patch) | |
tree | d9d6129bd645413a78561d42f1fdc9c9b94cb638 /sql-common/client.c | |
parent | 654dd03f2a5e1b532169e435602c92c1da2062b8 (diff) | |
download | mariadb-git-1756d087cd93dd7e4b4963b9b235daaf5279c8dc.tar.gz |
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).
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.
Diffstat (limited to 'sql-common/client.c')
-rw-r--r-- | sql-common/client.c | 13 |
1 files changed, 8 insertions, 5 deletions
diff --git a/sql-common/client.c b/sql-common/client.c index 0ff03f6609b..90d07f3e409 100644 --- a/sql-common/client.c +++ b/sql-common/client.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2000-2003 MySQL AB +/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -1145,10 +1145,10 @@ enum option_id { OPT_ssl_key, OPT_ssl_cert, OPT_ssl_ca, OPT_ssl_capath, OPT_character_sets_dir, OPT_default_character_set, OPT_interactive_timeout, OPT_connect_timeout, OPT_local_infile, OPT_disable_local_infile, - OPT_replication_probe, OPT_enable_reads_from_master, OPT_repl_parse_query, OPT_ssl_cipher, OPT_max_allowed_packet, OPT_protocol, OPT_shared_memory_base_name, OPT_multi_results, OPT_multi_statements, OPT_multi_queries, OPT_secure_auth, OPT_report_data_truncation, OPT_plugin_dir, OPT_default_auth, + OPT_keep_this_one_last }; static TYPELIB option_types={array_elements(default_options)-1, @@ -1198,6 +1198,9 @@ void mysql_read_default_options(struct st_mysql_options *options, DBUG_ENTER("mysql_read_default_options"); DBUG_PRINT("enter",("file: %s group: %s",filename,group ? group :"NULL")); + compile_time_assert(OPT_keep_this_one_last == + array_elements(default_options)); + argc=1; argv=argv_buff; argv_buff[0]= (char*) "client"; groups[0]= (char*) "client"; groups[1]= (char*) group; groups[2]=0; @@ -1222,7 +1225,7 @@ void mysql_read_default_options(struct st_mysql_options *options, /* Change all '_' in variable name to '-' */ for (end= *option ; *(end= strcend(end,'_')) ; ) *end= '-'; - switch (find_type(*option+2,&option_types,2)) { + switch (find_type(*option + 2, &option_types, FIND_TYPE_BASIC)) { case OPT_port: if (opt_arg) options->port=atoi(opt_arg); @@ -1338,8 +1341,8 @@ void mysql_read_default_options(struct st_mysql_options *options, options->max_allowed_packet= atoi(opt_arg); break; case OPT_protocol: - if ((options->protocol= find_type(opt_arg, - &sql_protocol_typelib,0)) <= 0) + if ((options->protocol= find_type(opt_arg, &sql_protocol_typelib, + FIND_TYPE_BASIC)) <= 0) { fprintf(stderr, "Unknown option to protocol: %s\n", opt_arg); exit(1); |