MySQL Bugs: #48939: First character in error text replaced with Z on DBUG

Bug #48939	First character in error text replaced with Z on DBUG_EXTRA builds
Submitted:	20 Nov 2009 11:30	Modified:	30 Nov 2009 9:35
Reporter:	Mattias Jonsson	Email Updates:
Status:	Duplicate	Impact on me:	None
Category:	MySQL Server: General	Severity:	S2 (Serious)
Version:	mysql-next	OS:	Any
Assigned to:	Assigned Account	CPU Architecture:	Any

Description:
The fix for removing a valgrind warning introduces an off by one error in strmake (introduced in bug#46042):
=== modified file 'strings/strmake.c'
--- strings/strmake.c	2007-11-26 08:00:41 +0000
+++ strings/strmake.c	2009-07-16 13:13:26 +0000
@@ -41,9 +41,9 @@
     write a character rather than '\0' as this makes spotting these
     problems in the results easier.
   */
-  uint n= strlen(src) + 1;
-  if (n <= length)
-    memset(dst + n, (int) 'Z', length - n + 1);
+  uint n= 0;
+  while (n < length && src[n++]);
+  memset(dst + n, (int) 'Z', length - n + 1);
 #endif
 
   while (length--)

Before was n = strlen + 1, after the patch only strlen, which leads to length - n + 1 is not longer correct, and it sets the byte after the dst to 'Z'.

I spotted this after building mysql-next-mr-bugfixing with BUILD/compile-pentium-debug-max (i.e. 32 bit) on my macbook (10.5.8) running ./mtr partition_innodb_semi_consistent and got this failure:
-ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ERROR HY000: Zock wait timeout exceeded; try restarting transaction

I.e. the error message string was overwritten with 'Z' on the first character.

when debugging this the call to strmake:
strmake (dst=0xb055836c "", src=0xb055856c "Lock wait timeout exceeded; try restarting transaction", length=512) at strmake.c:44

shows that it will write over the first byte in the src with the memset before copying the string.

Note: this probably only occurs when building with EXTRA_DEBUG i.e. no production release.

Maybe related to bug#48864.

How to repeat:
branch mysql-next-mr-bugfixing (maybe other trees as well)
compile on a mac os x 10.5.8 with BUILD/compile-pentium-debug-max
run ./mtr partition_innodb_semi_consistent and get:

-ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ERROR HY000: Zock wait timeout exceeded; try restarting transaction

Suggested fix:
=== modified file 'strings/strmake.c'
--- strings/strmake.c	2009-07-16 13:17:47 +0000
+++ strings/strmake.c	2009-11-20 11:22:42 +0000
@@ -43,7 +43,7 @@
   */
   uint n= 0;
   while (n < length && src[n++]);
-  memset(dst + n, (int) 'Z', length - n + 1);
+  memset(dst + n, (int) 'Z', length - n);
 #endif
 
   while (length--)

Mattias, can you repeat this with 5.1 or 5.0 ?

The DBUG_EXTRA code in strmake is actually correct since it also writes to dst[length] i.e. one byte after the given length (which will be replaced by a '\0' if the resulting string is >= length.

So to fix the found bug (error string changed first character to 'Z' on DBUG_EXTRA build) i will propose a patch for net_send_error_packet instead (where strmake was called with a too long length).

Changed the synopsis from 'strmake buffer overwrite after valgrind fix' to 'First character in error text replaced with Z on DBUG_EXTRA builds' since that was my first observation of the bug (on BUILD/compile-pentium-debug-max on Mac OS X 10.5).

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

2956 Mattias Jonsson	2009-11-23
      Bug#48939: First character in error text replaced with Z on DBUG_EXTRA builds
      
      Bad strmake call (using too long length)
     @ sql/protocol.cc
        Bug#48939: First character in error text replaced with Z on DBUG_EXTRA builds
        
        Bad strmake call (using too long length)
        strmake may add an '\0' to the byte AFTER length too.

Mattias,

I don't think it's correct. It'll work correctly if you run out of buffer space (length < strlen(src)) but it will not work correctly e.g. in cases where we have src as 'abc\0' and length is 6. In this case it will only set dst[4] and dst[5] to 'Z', but will skip dst[3] (because of the postfix increase of n in the check.
IMHO it should not increase the n++ in the while, but rather : 

while (n < length && src[n])
  n++;

The behavior of strmake is correct (sorry for the confusion). This bug is about bad call to strmake from net_send_error_packet.

Other bad strmake calls is in bug#48983 (or bug#31752).

Pushed into mysql-next-mr-bugfixing as a part of bug#35765 (test did not succeed without it on my macbook). Closing as duplicate.

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

2919 Mattias Jonsson	2009-12-15 [merge]
      merge (and including the fix for bug#48939 since that was not included in mysql-trunk-bugfixing).

Fix in now also in mysql-trunk-bugfixing, since it was missing there (as a part of pushing bug#49369

Pushed into 6.0.14-alpha (revid:alik@sun.com-20091221153807-80nxoli1tw1z9bxn) (version source revid:mattias.jonsson@sun.com-20091215225254-61dn8whhoy5susl1) (merge vers: 6.0.14-alpha) (pib:15)

Pushed into 5.5.1-m2 (revid:alik@sun.com-20091221153538-ifi3mxf3y10ozxin) (version source revid:mattias.jonsson@sun.com-20091215224704-vryjzqv1jrcxmclj) (merge vers: 5.5.0-beta) (pib:15)

Pushed into mysql-next-mr (revid:alik@sun.com-20091221153659-d7bt0fh6mhhclxuf) (version source revid:mattias.jonsson@sun.com-20091215225030-8pd8icoclp4ma89x) (pib:15)