Bug #50120 Valgrind errors in any test, inside mysqltest
Submitted: 6 Jan 2010 21:33 Modified: 12 Jan 2010 17:15
Reporter: Guilhem Bichot Email Updates:
Status: Closed Impact on me:
None 
Category:Tools: MTR / mysql-test-run Severity:S3 (Non-critical)
Version:5.5.99-m3, 6.0-codebase OS:Linux (64-bit)
Assigned to: Guilhem Bichot CPU Architecture:Any

[6 Jan 2010 21:33] Guilhem Bichot
Description:
When a test passes, mtr doesn't report any valgrind error caused by mysqltest, though there are some.
When a test fails, mtr does report those errors (see URL in private post):

backup.backup_xpfm_compat_restore_lctn0 [ fail ]
==2088== Conditional jump or move depends on uninitialised value(s)
==2088==    at 0x4A06509: index (mc_replace_strmem.c:164)
==2088==    by 0x358CE33109: setenv (in /lib64/libc-2.5.so)
==2088==    by 0x4558C5: var_set(char const*, char const*, char const*, char const*) (mysqltest.cc:2088)
==2088==    by 0x455B49: var_set_string(char const*, char const*) (mysqltest.cc:2098)
==2088==    by 0x457C2A: main (mysqltest.cc:7746)

Seen with 6.0-codebase-bugfixing:
guilhem@mysql.com-20100106153545-2cbhdxst7u3oxtlh
and next-mr-bugfixing:
guilhem@mysql.com-20100106153111-85776eajdrd4yxxa

How to repeat:
./mtr --mem alias --valgrind
and then
grep -r "at 0x" var/log
you will see this in mysqltest.log:
var/log/mysqltest.log:==4169==    at 0x416216: var_set(char const*, char const*, char const*, char const*) (mysqltest.cc:2086)
var/log/mysqltest.log:==4169==    at 0x4C27FEF: setenv (mc_replace_strmem.c:778)
var/log/mysqltest.log:==4169==    at 0x4C26A39: index (mc_replace_strmem.c:160)
var/log/mysqltest.log:==4169==    at 0x4C26A40: index (mc_replace_strmem.c:160)
var/log/mysqltest.log:==4169==    at 0x4C26D38: strlen (mc_replace_strmem.c:242)
var/log/mysqltest.log:==4169==    at 0x41624C: var_set(char const*, char const*, char const*, char const*) (mysqltest.cc:2089)
var/log/mysqltest.log:==4169==    at 0x4C265AE: malloc (vg_replace_malloc.c:207)
var/log/mysqld.1.err:==4133==    at 0x4C265AE: malloc (vg_replace_malloc.c:207)

In 6.0-codebase-bugfixing, there are leaks (not serious) and "conditional jump or move depends on uninitialized values" (serious). In next-mr-bugfixing, only leaks.
So to me the severity of this bug is bigger in 6.0 than in next-mr.
Though even the non-serious leaks are an annoyance when a test fail (they just clutter the screen) so should be fixed if easy (some look like libc-specific, cannot be fixed, maybe hidden with Valgrind suppressions, if it's worth it).

Suggested fix:
debug those errors and eliminate at least the most serious ones.
[6 Jan 2010 21:53] Guilhem Bichot
For 6.0-codebase-bugfixing
==========================
"conditional jump" error was introduced by
serg@mysql.com-20091229134448-phe834ukzmi0k2e3
and is in this code (mysqltest.cc:var_set()):
    char oldc= v->name[v->name_len];
    if (oldc)
      v->name[v->name_len]= 0;   // setenv() expects \0-terminated strings
    setenv(v->name, v->str_val, 1); // v->str_val is always \0-terminated
    if (oldc)
      v->name[v->name_len]= oldc;
Say that v->name_len is 3 (string has length 3). Then v->name[3] may not be initialized, so should not be used in an if(). Here it's used in an if() hence the error. The solution is to just remove the two "if (oldc)" lines, keeping
v->name[v->name_len]= 0
and
v->name[v->name_len]= oldc
always done unconditionally. It achieves the same as with "if(oldc)" but without the error.
After doing this change, the only error left in mysqltest is a leak in NPTL which I won't look at (not a problem in MySQL itself).

For next-mr-bugfixing
=====================
The leak in mysqltest.cc:var_set() is because we don't free my_strdup()-produced strings (v->env_s), but this is already fixed in 6.0-codebase-bugfixing (where we use setenv() instead of putenv(), thus let setenv() do the allocation, thus we don't have to do my_strdup()).
As it's a small annoyance already fixed in 6.0-codebase-bugfixing, I ignore this.
[7 Jan 2010 10:13] 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/96231

3816 Guilhem Bichot	2010-01-07
      fix for BUG#50120 "Valgrind errors in any test, inside mysqltest"
      Problem was that as v->name[v->name_len] may be uninitialized (which is ok per se),
      it shouldn't be used in an if(). Now we zero/restore this character unconditionally instead of with if(),
      which has the same effects but no Valgrind error.
[8 Jan 2010 9:30] 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/96352

3816 Guilhem Bichot	2010-01-08
      fix for BUG#50120 "Valgrind errors in any test, inside mysqltest"
      Problem was that as v->name[v->name_len] may be uninitialized (which is ok per se),
      it shouldn't be used in an if(). We remove this zero_the_char/restore_it logic by
      rather zero-terminating the v->name string when we create it in var_init().
[8 Jan 2010 13:50] 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/96391

3817 Guilhem Bichot	2010-01-08
      fix for BUG#50120 "Valgrind errors in any test, inside mysqltest"
      Problem was that as v->name[v->name_len] may be uninitialized (which is ok per se),
      it shouldn't be used in an if(). We remove this zero_the_char/restore_it logic by
      rather zero-terminating the v->name string when we create it in var_init().
[8 Jan 2010 14:05] Guilhem Bichot
queued to 6.0-codebase-bugfixing.
[11 Jan 2010 16:44] 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/96524

2963 Alexander Nozdrin	2010-01-11
      Backporting revision from mysql-6.0-codebase-bugfixing.
      Original revision:
      ------------------------------------------------------------
      revno: 3817
      revision-id: guilhem@mysql.com-20100108092756-k0zzf4kvx9b7bh38
      parent: guilhem@mysql.com-20100107101133-hrrgcdqg508runuf
      committer: Guilhem Bichot <guilhem@mysql.com>
      branch nick: mysql-6.0-codebase-bugfixing
      timestamp: Fri 2010-01-08 10:27:56 +0100
      message:
        fix for BUG#50120 "Valgrind errors in any test, inside mysqltest"
        Problem was that as v->name[v->name_len] may be uninitialized (which is ok per se),
        it shouldn't be used in an if(). We remove this zero_the_char/restore_it logic by
        rather zero-terminating the v->name string when we create it in var_init().
      ------------------------------------------------------------
[11 Jan 2010 16:48] Alexander Nozdrin
Backporting patch queued to next-mr-bugfixing.
[12 Jan 2010 16:24] Bugs System
Pushed into 6.0.14-alpha (revid:alik@sun.com-20100112162328-2sblcul1kl08bbib) (version source revid:guilhem@mysql.com-20100108092756-k0zzf4kvx9b7bh38) (merge vers: 6.0.14-alpha) (pib:15)
[12 Jan 2010 17:15] Paul DuBois
Test suite change. No changelog entry needed.