Skip to content

Commit

Permalink
Fix valgrind issue in bypass
Browse files Browse the repository at this point in the history
Summary:
Valgrind is complaining access uninitialized memory. The problem is happening because  in string::set_int it converts integer
1 to string without appending null:

```
bool String::set_int(longlong num, bool unsigned_flag, const CHARSET_INFO *cs)
{
  uint l=20*cs->mbmaxlen+1;
  int base= unsigned_flag ? 10 : -10;

  if (alloc(l))
    return TRUE;
  str_length=(uint32) (cs->cset->longlong10_to_str)(cs,Ptr,l,base,num);
  str_charset=cs;
  return FALSE;
}
```

So the Ptr buffer end up being "1<....rest of garbage>...", but the length is 24 (20+1 with alignment padding).

Later the c_ptr call tries to determine if the string is properly null terminated by accessing the immediate character after "1" which is "uninitialized data" in valgrind's terms but is relatively safe because either that char is \0 or it'll overwrite it with 0 in realloc:

```
  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;
  }
```

Either way, c_ptr_safe version is preferred as it doesn't touch uninitialized data, always overwrite the end with 0, and will realloc if necessary (in case the buffer isn't big enough). Another option is to call string::append which would be faster but given this is debug code (select_parser::dump is only used in this iteration and will be removed when the real execution code is in) and I'm not going to worry about perf.

Test Plan:
rocksdb.bypass_select_basic_bloom 'write_committed' [ pass ]   9929
rocksdb.bypass_select_basic_bloom 'write_prepared' [ pass ]  12954
valgrind_report                          [ pass ]
--------------------------------------------------------------------------
The servers were restarted 1 times
Spent 22.883 of 67 seconds executing testcases

Completed: All 3 tests were successful.

Reviewers: herman, #mysql_eng

Reviewed By: herman

Subscribers: butterflybot, zhichengzhu, ritwikyadav, vinaybhat, [email protected]

Differential Revision: https://phabricator.intern.facebook.com/D16693966

Tasks: T48436407

Signature: 16693966:1565210909:517fc04758a1ff608a08922f9f92fa3c5e6d2cb9
  • Loading branch information
yizhang82 committed Aug 7, 2019
1 parent 2520fda commit 8072e3d
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions storage/rocksdb/nosql_access.cc
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ class select_parser {
if (ret == nullptr) {
str += "NULL";
} else {
str += ret->c_ptr();
str += ret->c_ptr_safe();
}
}
str += ")";
Expand All @@ -233,7 +233,7 @@ class select_parser {
if (ret == nullptr) {
str += "NULL";
} else {
str += ret->c_ptr();
str += ret->c_ptr_safe();
}
}
str += ")";
Expand Down

0 comments on commit 8072e3d

Please sign in to comment.