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:
None 
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
Description:
Valgrind generates warnings because c_ptr references memory beyond the end of the allocation done for Ptr.

How to repeat:
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. What happens when:
* Alloced_length == str_length
* Ptr[str_length] == 0 in c_ptr()
* Ptr[str_length] != 0 on return from c_ptr() because another thread that owned that memory updated it

Also, c_ptr() is not a good name for this function as it does not always return a pointer to a C string as a C string always has a nul-terminator.

 inline char *c_ptr()
  {
    if (!Ptr || Ptr[str_length])                /* Should be safe */
      (void) realloc(str_length);
    return Ptr;
  }

  inline char *c_ptr_safe()
  {
    if (Ptr && str_length < Alloced_length)
      Ptr[str_length]=0;
    else
      (void) realloc(str_length);
    return Ptr;
  }
[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