From 716b64cdb050ff9a22457990f336ef20a7b3663a Mon Sep 17 00:00:00 2001 From: Martin Hansson Date: Thu, 13 Jan 2011 08:57:15 +0100 Subject: Bug#58165: "my_empty_string" gets modified and causes LOAD DATA to fail and other crashes Some string manipulating SQL functions use a shared string object intended to contain an immutable empty string. This object was used by the SQL function SUBSTRING_INDEX() to return an empty string when one argument was of the wrong datatype. If the string object was then modified by the sql function INSERT(), undefined behavior ensued. Fixed by instead modifying the string object representing the function's result value whenever string manipulating SQL functions return an empty string. Relevant code has also been documented. --- sql/sql_string.h | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'sql/sql_string.h') diff --git a/sql/sql_string.h b/sql/sql_string.h index b15179bcbe5..092e194646f 100644 --- a/sql/sql_string.h +++ b/sql/sql_string.h @@ -136,6 +136,16 @@ public: Alloced_length=0; str_charset=str.str_charset; } + + + /** + Points the internal buffer to the supplied one. The old buffer is freed. + @param str Pointer to the new buffer. + @param arg_length Length of the new buffer in characters, excluding any + null character. + @param cs Character set to use for interpreting string data. + @note The new buffer will not be null terminated. + */ inline void set(char *str,uint32 arg_length, CHARSET_INFO *cs) { free(); -- cgit v1.2.1 From 6503226743a2fa24c7330d4541560a5b8fa821d6 Mon Sep 17 00:00:00 2001 From: Magne Mahre Date: Thu, 17 Feb 2011 12:43:53 +0100 Subject: Bug#48053 String::c_ptr has a race and/or does an invalid memory reference There are two issues present here. 1) There is a possibility that we test a byte beyond the allocated buffer 2) We compare a byte that might never have been initalized to see if it's 0. The first issue is not triggered by existing code, but an ASSERT has been added to safe-guard against introducing new code that triggers it. The second issue is what triggers the Valgrind warnings reported in the bug report. A buffer is allocated in class String to hold the value. This buffer is populated by the character data constituting the string, but is not zero-terminated in most cases. Testing if it is indeed zero-terminated means that we check a byte that has never been explicitly set, thus causing Valgrind to trigger. Note that issue 2 is not a serious problem. The variable is read, and if it's not zero, we will set it to zero. There are no further consequences. Note that this patch does not fix the underlying problems with issue 1, as it is deemed too risky to fix at this point (as noted in the bug report). As discussed in the report, the c_ptr() method should probably be replaced, but this requires a thorough analysis of the ~200 calls to the method. sql/set_var.cc: These two cases have been reported to fail with Valgrind. --- sql/sql_string.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'sql/sql_string.h') diff --git a/sql/sql_string.h b/sql/sql_string.h index 092e194646f..c56c69493d4 100644 --- a/sql/sql_string.h +++ b/sql/sql_string.h @@ -106,6 +106,9 @@ public: inline const char *ptr() const { return Ptr; } inline char *c_ptr() { + DBUG_ASSERT(!alloced || !Ptr || !Alloced_length || + (Alloced_length >= (str_length + 1))); + if (!Ptr || Ptr[str_length]) /* Should be safe */ (void) realloc(str_length); return Ptr; -- cgit v1.2.1