maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "knielsen" == knielsen  <knielsen@xxxxxxxxxxxxxxx> writes:

knielsen> The sql_string.h String class has some strange code to null-terminate
knielsen> the string for String::c_ptr().

knielsen> It checks if the string is already null-terminated, and only writes
knielsen> the '\0' at the end if it is not already there.

knielsen> The reason for this I think is to avoid the re-allocation of the buffer to fit the null termination at append time if it is not needed due to never calling c_ptr().

Yes.

knielsen> However, the problem case here is a different one, where the null
knielsen> termination fits in the given buffer, but the value is uninitialised.

In what code did this happen ?
In most of MySQL code we will put a \0 at end when we generate a string.

knielsen> In this case, checking before zeroing works (if in a somewhat twisted
knielsen> way). But it causes a false Valgrind alarm.

knielsen> This is a minimal fix; not sure if it is the right one. Maybe it would
knielsen> be better to fix the c_str() method to not use this tricky way of
knielsen> working?

The problem with fixing c_ptr() is that we would have to do a lot of
unnecessary mallocs;  This is in particular true when you use a String
as a pointer to a null terminated C string.  In this case the end \0
is not part of the string and a c_ptr() would force us to do a
not-needed malloc.

knielsen> ---
knielsen>  sql/sql_string.h |    8 ++++++++
knielsen>  1 file changed, 8 insertions(+)

knielsen> Index: work-5.1-buildbot/sql/sql_string.h
knielsen> ===================================================================
knielsen> --- work-5.1-buildbot.orig/sql/sql_string.h	2009-04-08 00:34:49.000000000 +0200
knielsen> +++ work-5.1-buildbot/sql/sql_string.h	2009-04-08 00:35:38.000000000 +0200
knielsen> @@ -99,7 +99,15 @@ public:
knielsen>    inline const char *ptr() const { return Ptr; }
knielsen>    inline char *c_ptr()
knielsen>    {
knielsen> +    /*
knielsen> +      This code null-terminates the string if not already null-terminated.
knielsen> +      This works whether Ptr[str_length] is undefined or not, but if undefined
knielsen> +      it generates a Valgrind error. So in the Valgrind case, null-terminate
knielsen> +      unconditionally.
knielsen> +    */
knielsen> +#ifndef HAVE_purify
knielsen>      if (!Ptr || Ptr[str_length])		/* Should be safe */
knielsen> +#endif
knielsen>        (void) realloc(str_length);
knielsen>      return Ptr;
knielsen>    }

In this case I would like to keep the original code to ensure we don't
access unallocated or uninitialized memory.

Regards,
Monty