MySQL Bugs: #59307: Valgrind: uninitialized value in rw_lock_set_writer_id_and_recursion

Bug #59307	Valgrind: uninitialized value in rw_lock_set_writer_id_and_recursion_flag()
Submitted:	5 Jan 2011 10:26	Modified:	27 Jun 2011 16:43
Reporter:	Jørgen Løland	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	5.1, 5.5, 5.6	OS:	Any
Assigned to:	Marko Mäkelä	CPU Architecture:	Any

Description:
Valgrind reports:

==32310== Conditional jump or move depends on uninitialised value(s)
==32310==    at 0xA4B91E: rw_lock_set_writer_id_and_recursion_flag (sync0rw.ic:283)
==32310==    by 0xA4C5F7: rw_lock_x_lock_low (sync0rw.c:569)
==32310==    by 0xA4C718: rw_lock_x_lock_func (sync0rw.c:628)
==32310==    by 0xB178D7: pfs_rw_lock_x_lock_func (sync0rw.ic:692)
==32310==    by 0xB185F3: hash_lock_x_all (hash0hash.c:232)
==32310==    by 0xAB72CF: buf_pool_validate_instance (buf0buf.c:4632)
==32310==    by 0xAB7B97: buf_validate (buf0buf.c:4850)
==32310==    by 0xAB2F12: buf_page_get_gen (buf0buf.c:2999)
==32310==    by 0xA670BA: trx_rsegf_get_new (trx0rseg.ic:69)
==32310==    by 0xA679A2: trx_rseg_mem_create (trx0rseg.c:195)
==32310==    by 0xA67D04: trx_rseg_create_instance (trx0rseg.c:275)
==32310==    by 0xA67F45: trx_rseg_list_and_array_init (trx0rseg.c:340)
==32310==    by 0xA6B06E: trx_sys_init_at_db_start (trx0sys.c:962)
==32310==    by 0xA490C5: innobase_start_or_create_for_mysql (srv0start.c:1714)
==32310==    by 0x9E9E53: innobase_init(void*) (ha_innodb.cc:2674)
==32310==    by 0x76D7AC: ha_initialize_handlerton(st_plugin_int*) (handler.cc:475)

How to repeat:
Run the following mtr test with valgrind enabled:

CREATE TABLE t1 (
  t1_int INT,
  t1_time TIME
) ENGINE=innodb;

CREATE TABLE t2 ( 
  t2_int int PRIMARY KEY,
  t2_int2 INT
) ENGINE=INNODB;

INSERT INTO t2 VALUES ();
INSERT INTO t1 VALUES ();

SELECT *
FROM t1 AS t1a 
WHERE NOT EXISTS
  (SELECT *
   FROM t1 AS t1b
   WHERE t1b.t1_int NOT IN
     (SELECT t2.t2_int 
      FROM t2
      WHERE t1b.t1_time LIKE t1b.t1_int
      OR t1b.t1_time <> t2.t2_int2
      AND 6=7 
 )
)
;

$ bzr parent
bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-trunk-bugfixing/
$ bzr revision-info
3474 dao-gang.qu@sun.com-20101229053531-6jamezb1vubx5t3r

I get this issue on my laptop running 32-bit Ubuntu in all valgrind runs as far as I can tell.  I see it even when I run:

 ./mtr --valgrind-mysqld 1st

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/131053

3706 Vasil Dimov	2011-02-10
      Fix Bug#59307 Valgrind: uninitialized value in rw_lock_set_writer_id_and_recursion_flag()
      
      by silencing a bogus Valgrind warning:
      
      ==4392== Conditional jump or move depends on uninitialised value(s)
      ==4392==    at 0x5A18416: rw_lock_set_writer_id_and_recursion_flag (sync0rw.ic:283)
      ==4392==    by 0x5A1865C: rw_lock_x_lock_low (sync0rw.c:558)
      ==4392==    by 0x5A18481: rw_lock_x_lock_func (sync0rw.c:617)
      ==4392==    by 0x597EEE6: mtr_x_lock_func (mtr0mtr.ic:271)
      ==4392==    by 0x597EBBD: fsp_header_init (fsp0fsp.c:970)
      ==4392==    by 0x5A15E78: innobase_start_or_create_for_mysql (srv0start.c:1508)
      ==4392==    by 0x598B789: innobase_init(void*) (ha_innodb.cc:2282)
      
      os_compare_and_swap_thread_id() is defined as
      __sync_bool_compare_and_swap(). From the GCC doc:
      
      `bool __sync_bool_compare_and_swap (TYPE *ptr, TYPE oldval TYPE newval, ...)'
        ...
        The "bool" version returns true if the comparison is successful and
        NEWVAL was written.
      
      So it is not possible that the return value is uninitialized, no matter what
      the arguments to os_compare_and_swap_thread_id() are. Probably Valgrind gets
      confused by the implementation of the GCC internal function
      __sync_bool_compare_and_swap().

Note that InnoDB implements some Valgrind instrumentation that has to be enabled at compile time. Most of this instrumentation is to allow catching more errors, but this is an exception.

The function rw_lock_set_writer_id_and_recursion_flag() is using atomic compare-and-swap for assigning lock->writer_thread to curr_thread:

	local_thread = lock->writer_thread;
	success = os_compare_and_swap_thread_id(
		&lock->writer_thread, local_thread, curr_thread);

The call is roughly equivalent to the following:

	success = (local_thread == lock->writer_thread);
	if (success) lock->writer_thread = curr_thread;

Valgrind fails to detect that (a==a) is always TRUE, even when a is uninitialized.

However, the patch that was committed is incorrect. If Valgrind emits a warning for "success" being uninitialized, after we have declared local_thread and lock->writer_thread to be initialized in Valgrind, that would be a sign of real trouble.

Because this warning keeps popping up every now and then, I think that we need a fix that makes it disappear even when Valgrind instrumentation is disabled. We could simply initialize writer_thread in rw_lock_create().

Marko, thanks!

Some duplicates: Bug #44186, Bug #47873, Bug #48095, Bug #51792, Bug #55720, a comment in Bug #58837.