Bug #48053 | String::c_ptr has a race and/or does an invalid memory reference | ||
---|---|---|---|
Submitted: | 14 Oct 2009 19:09 | Modified: | 30 Mar 2011 16:56 |
Reporter: | Mark Callaghan | Email Updates: | |
Status: | Closed | Impact on me: | |
Category: | MySQL Server: General | Severity: | S2 (Serious) |
Version: | 5.0, 5.1 | OS: | Any |
Assigned to: | Magne Mæhre | CPU Architecture: | Any |
Tags: | c_ptr, valgrind |
[14 Oct 2009 19:09]
Mark Callaghan
[14 Oct 2009 20:02]
Harrison Fisk
There is a thread on internals@ regarding this at: http://lists.mysql.com/internals/37371
[14 Oct 2009 20:04]
Harrison Fisk
Drizzle has changed the code to the following: inline char *c_ptr() { if (str_length == Alloced_length) (void) realloc(str_length); else Ptr[str_length]= 0; return Ptr; } inline char *c_ptr_quick() { if (Ptr && str_length < Alloced_length) Ptr[str_length]=0; return Ptr; } inline char *c_ptr_safe() { if (Ptr && str_length < Alloced_length) Ptr[str_length]=0; else (void) realloc(str_length); return Ptr; }
[29 Sep 2010 8:19]
Alexander Barkov
Hi Mark, can you please post instructions (SQL queries, etc) how we can repeat valgrind warnings? Thanks.
[1 Oct 2010 0:06]
Mark Callaghan
I don't know if I ever got a valgrind warning. But the problem was obvious from reading the code.
[4 Oct 2010 12:10]
Alexander Barkov
Hi Mark, > The problem is that the allocation for Ptr might have size str_length and in > that case > Ptr[str_length] is one byte after the allocation... In many cases using Ptr[str_length] is safe. For example, if a string was created with one of these constructors, using c_ptr() is safe: String() String(const char *str, CHARSET_INFO *cs) It is also safe to call c_ptr() after calling realloc(), alloc(), copy(), append() and set*() methods (at least after *some* of them). I checked c_ptr() just in a few pieces of the code, and they seem to be used after "safe" constructors or methods. However, reading the code is really hard. One needs to track down how a string was created and modified to know if using c_ptr() is safe in a particular place. Perhaps it would be easier to have a method which always guarantees to return a 0-terminated string. Perhaps even having methods c_ptr(), c_ptr_quick() and c_ptr_safe() is over-engineering, and having a single, safe, method would be enough. Something like this should do: inline char *c_ptr() { if (str_length >= Alloced_length) (void) realloc(str_length); // Realloc adds extra '\0' else Ptr[str_length]= 0; return Ptr; } But before doing this change, all individual calls for c_ptr(), c_ptr_safe() and c_ptr_quick() should be carefully analysed, to avoid any possible side effects. There are about 220 calls in the code. We cannot do that in 5.5, which is a near-GA version. For the version 5.5 we can fix only those pieces of the code where c_ptr() is used in unsafe way. Please tell us if you find any. Thanks.
[5 Oct 2010 19:07]
Mark Callaghan
I agree with you that this isn't a trivial change. Can you add a debug assert statement to detect mis-use. I don't think there are any when mtr is run today. I think the assert below will catch problems if they are added in the future. inline char *c_ptr() { DBUG_ASSERT(!alloced || !Ptr || !Alloced_length || (Alloced_length >= (str_length + 1))); if (!Ptr || Ptr[str_length]) /* Should be safe */ (void) realloc(str_length); return Ptr; }
[6 Oct 2010 18:24]
Konstantin Osipov
Bar, I think it should be safe to add an assert to 5.1. Based on that lowering the risk evaluation. Please ask to re-triage if you disagree.
[8 Oct 2010 14:20]
Omer Barnir
triage setting to SR51MRU for the 'limited' fix as described by kostja above
[22 Dec 2010 0:39]
Mark Callaghan
We had another instance of this using 5.1.52 after switching to gcc 4.4. The warning is intermittent and occurs on the test main.variables when this is run: set global sql_mode=repeat('a',80); valgrind reports: ==32014== Conditional jump or move depends on uninitialised value(s) ==32014== at 0x57D2DF: String::c_ptr() (sql_string.h:105) ==32014== by 0x683A68: sys_var::check_set(THD*, set_var*, st_typelib*) (set_var.cc:2021) ==32014== by 0x68FDDD: sys_var_thd_sql_mode::check(THD*, set_var*) (set_var.h:568) ==32014== by 0x687BAF: set_var::check(THD*) (set_var.cc:3743) ==32014== by 0x687795: sql_set_variables(THD*, List<set_var_base>*) (set_var.cc:3660) ==32014== by 0x67382B: mysql_execute_command(THD*, unsigned long long*) (sql_parse.cc:3851) ==32014== by 0x67BB00: mysql_parse(THD*, char*, unsigned, char const**, unsigned long long*) (sql_parse.cc:6470) ==32014== by 0x66C448: dispatch_command(enum_server_command, THD*, char*, unsigned) (sql_parse.cc:1321) ==32014== by 0x66B2FE: do_command(THD*) (sql_parse.cc:935) ==32014== by 0x667C4D: handle_one_connection (sql_connect.cc:1184) ==32014== by 0x3750E062F6: start_thread (in /lib64/libpthread-2.5.so) ==32014== by 0x37502D1E3C: clone (in /lib64/libc-2.5.so) The problem is inside sys_var::check_set() the full 80 bytes of buff are used. The call to res->c_ptr() then checks one byte past the end of buff for a null byte. This is an undefined operation. ------- I confirmed that c_ptr reads one past the end of the string by adding printfs to sys_var::check_set: fprintf(stderr, "in check_set allocate %p to %p for length %d\n", buff, buff + sizeof(buff), (int) sizeof(buff)); and to c_ptr in sql_string.h: inline char *c_ptr() { if (Ptr) { fprintf(stderr, "c_ptr use %p and %p for length %d\n", Ptr, Ptr+str_length, (int) str_length); } if (!Ptr || Ptr[str_length]) /* Should be safe */ (void) realloc(str_length); return Ptr; } And then I ran ./mysql-test-run.pl main.variables and this is in the error log: in check_set allocate 0x41ede390 to 0x41ede3e0 for length 80 c_ptr use 0x41ede390 and 0x41ede3e0 for length 80 This means that check_set used a buffer of length 80, that pointer was assigned to Ptr, and then c_ptr read Ptr[80] which is one past the end of the array. This pattern for using sql_string.h occurs in many places in mysqld source: char buff[STRING_BUFFER_USUAL_SIZE]; String str(buff, sizeof(buff), system_charset_info);
[2 Feb 2011 15:36]
MySQL Verification Team
Mark, Magne, here's a testcase. run mysqld in valgrind, then: set global LC_MESSAGES=convert((@@global.log_bin_trust_function_creators) using cp1250); Version: '5.5.10-valgrind-max-debug' socket: 'sock' port: 3306 Source distribution Thread 17: Conditional jump or move depends on uninitialised value(s) at: String::c_ptr (sql_string.h:116) by: check_locale (sys_vars.cc:3085) by: sys_var::check (set_var.cc:226) by: set_var::check (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) Uninitialised value was created by a heap allocation at: malloc (vg_replace_malloc.c:195) by: my_malloc (my_malloc.c:38) by: String::real_alloc (sql_string.cc:44) by: String::alloc (sql_string.h:231) by: String::copy (sql_string.cc:320) by: Item_func_conv_charset::val_str (item_strfunc.cc:2970) by: check_locale( (sys_vars.cc:3083) by: sys_var::check (set_var.cc:226) by: set_var::check (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)
[2 Feb 2011 15:52]
MySQL Verification Team
Another: set global LC_TIME_NAMES=convert((-8388608) using macroman);
[2 Feb 2011 17:21]
MySQL Verification Team
please fix this also: set global LC_TIME_NAMES=convert((convert((0x63) using eucjpms)) using big5)
[9 Feb 2011 9:11]
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/130806 3584 Magne Mahre 2011-02-09 Bug#48053 String::c_ptr has a race and/or does an invalid memory reference There are two issues present here. 1) There is a possibility that we test a byte beyond the allocated buffer 2) We compare a byte that might never have been initalized to see if it's 0. The first issue is not triggered by existing code, but an ASSERT has been added to safe-guard against introducing new code that triggers it. The second issue is what triggers the Valgrind warnings reported in the bug report. A buffer is allocated in class String to hold the value. This buffer is populated by the character data constituting the string, but is not zero-terminated in most cases. Testing if it is indeed zero-terminated means that we check a byte that has never been explicitly set, thus causing Valgrind to trigger. Note that issue 2 is not a serious problem. The variable is read, and if it's not zero, we will set it to zero. There are no further consequences. Note that this patch does not fix the underlying problems with issue 1, as it is deemed too risky to fix at this point (as noted in the bug report). As discussed in the report, the c_ptr() method should probably be replaced, but this requires a thorough analysis of the ~200 calls to the method. @ sql/set_var.cc These two cases have been reported to fail with Valgrind.
[30 Mar 2011 16:56]
Paul DuBois
Noted in 5.1.57 changelog. A potential invalid memory access discovered by Valgrind was fixed. CHANGESET - http://lists.mysql.com/commits/130806