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:
Category:Connector / ODBC Severity:S3 (Non-critical)
Version:5.1.9 OS:Any
Assigned to: Bogdan Degtyariov CPU Architecture:Any

[23 Jan 2012 2:19] Paul Harris
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; )
[23 Jan 2012 6:00] Bogdan Degtyariov
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.
[23 Jan 2012 6:08] Paul Harris
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.
[20 Mar 2012 9:21] Tom Hughes
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.
[3 Sep 2013 21:50] Lawrenty Novitsky
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.
[4 Sep 2013 16:32] Lawrenty Novitsky
The patch has been pushed to 5.1 as rev#1109, to 5.2 as rev#1169
[18 Sep 2013 23:02] Daniel So
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."