MySQL Bugs: #64105: valgrind error, simple loop fix

Bug #64105	valgrind error, simple loop fix
Submitted:	23 Jan 2012 2:19	Modified:	18 Sep 2013 23:02
Reporter:	Paul Harris	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / ODBC	Severity:	S3 (Non-critical)
Version:	5.1.9	OS:	Any
Assigned to:	Bogdan Degtyariov	CPU Architecture:	Any

Description:
stringutil.c, line 95

the for loop looks like this:

  for (pos= str, i= 0; *pos && pos < str_end; )

it should look like this:

  for (pos= str, i= 0; pos < str_end && *pos; )

ie, test for the end of the buffer, before testing the contents of the pointer.

How to repeat:
Valgrind detected this

Suggested fix:
Change for loop to look like this:
  for (pos= str, i= 0; pos < str_end && *pos; )

Paul, thank you for your interest in MySQL Software.

In this particular case the suggested change does not improve the code stability for NULL pointers because pos is initialized from str, which means that str_end will be NULL pointer when str is NULL.

However, for not-NULL values it might prevent the out-of-bounds condition if for some reason utf8toutf32() returns the number of consumed bytes longer than the end of the buffer.

The problem is not when the string passed is NULL,

The problem is that it dereferences the pos pointer when it is passed the end of the string...

if pos == str_end (ie passed the end of the string-buffer), then you should not dereference pos to check its contents.  You will be accessing memory that is either not allocated, or belongs to another object on the stack.

This is most definitely a real problem - if you pass a string length, or the routine does a transcode internally before getting to this loop, then the byte at str_end is at the very least not defined and if you're unlucky could be in a different page and trigger a seg fault when you try and read it.

Simply reversing the order of the tests as described is enough to fix it.

If nothing else please fix it so that those of us trying to run valgrind on programs using the MySQL ODBC connector don't get lots of noise in our logs.

There is not much space for imagination here

=== modified file 'util/stringutil.c'
--- util/stringutil.c   2013-06-28 16:53:41 +0000
+++ util/stringutil.c   2013-09-02 15:57:03 +0000
@@ -94,7 +94,7 @@
     return NULL;
   }

-  for (pos= str, i= 0; *pos && pos < str_end; )
+  for (pos= str, i= 0; pos < str_end && *pos != 0; )
   {
     if (sizeof(SQLWCHAR) == 4)
     {

We agreed this patch is needed. Given its complexity I believe one review is enough.

The patch has been pushed to 5.1 as rev#1109, to 5.2 as rev#1169

Added the following entry to the Connector/ODBC 5.2.6 and 5.1.13 changelogs:

"The exit condition for a for loop in stringutil.c is changed to avoid a possible out-of-bounds error and the associated reports by Valgrind."