bzr commit into mysql-5.1-bugteam branch (martin.hansson:3525) Bug#58165

[Martin Hansson]

#At file:///data0/martin/bzrroot/bug58165/5.1bt/ based on revid:gleb.shchepa@stripped

 3525 Martin Hansson	2010-12-15
      Bug#58165: "my_empty_string" gets modified and causes LOAD DATA to fail and
      other crashes
      
      The string manipulating function insert() used a shared string buffer intended
      to always contain an empty string. Fixed by copying the buffer before
      modifying it.
      
      Relevant code has also been documented.

    modified:
      client/sql_string.cc
      mysql-test/r/func_str.result
      mysql-test/t/func_str.test
      sql/item_strfunc.cc
=== modified file 'client/sql_string.cc'
--- a/client/sql_string.cc	2010-07-09 12:00:17 +0000
+++ b/client/sql_string.cc	2010-12-15 11:00:25 +0000
@@ -58,11 +58,28 @@ bool String::real_alloc(uint32 arg_lengt
 }
 
 
-/*
-** Check that string is big enough. Set string[alloc_length] to 0
-** (for C functions)
-*/
+/**
+   Allocates a new buffer on the heap for this String.
+
+   - If the String's internal buffer is privately owned and heap allocated, a
+     new buffer will be allocated, data moved and the old buffer freed.
+
+   - If the String does not keep a private buffer on the heap, such a buffer
+     will be allocated and the string copied accoring to its length, as found
+     in String::length().
 
+   For C compatibility, the new string buffer is null terminated.
+
+   @param alloc_length The size of new buffer to be allocated, excluding null
+   terminator. If the current string buffer is larger than this value, no
+   allocation occurs.
+
+   @retval false Either the copy operation is complete or, if the size of the
+   new buffer is smaller than the currently allocated buffer (if one exists),
+   no allocation occured.
+
+   @retval true An error occured when attempting to allocate memory.
+*/
 bool String::realloc(uint32 alloc_length)
 {
   uint32 len=ALIGN_SIZE(alloc_length+1);
@@ -206,6 +223,17 @@ bool String::copy()
   return FALSE;
 }
 
+/**
+   Copies the internal buffer from str. If this String has a private heap
+   allocated buffer where new data does not fit, a new buffer is allocated
+   before copying and the old buffer freed. Character set information is also
+   copied.
+   
+   @param str The string whose internal buffer was duplicated.
+   
+   @retval false Success.
+   @retval true Memory allocation failed.
+ */
 bool String::copy(const String &str)
 {
   if (alloc(str.str_length))

=== modified file 'mysql-test/r/func_str.result'
--- a/mysql-test/r/func_str.result	2010-12-14 16:08:25 +0000
+++ b/mysql-test/r/func_str.result	2010-12-15 11:00:25 +0000
@@ -2612,4 +2612,20 @@ CONVERT(('' IN (REVERSE(CAST(('') AS DEC
 1
 Warnings:
 Warning	1292	Truncated incorrect DECIMAL value: ''
+# 
+# Bug#58165: "my_empty_string" gets modified and causes LOAD DATA to fail
+# and other crashes
+#
+CREATE TABLE t1 ( a TEXT );
+SELECT 'aaaaaaaaaaaaaa' INTO OUTFILE 'bug58165.txt';
+SELECT insert( substring_index( 'a', 'a', 'b' ), 1, 0, 'x' );
+insert( substring_index( 'a', 'a', 'b' ), 1, 0, 'x' )
+x
+Warnings:
+Warning	1292	Truncated incorrect INTEGER value: 'b'
+LOAD DATA INFILE 'bug58165.txt' INTO TABLE t1;
+SELECT * FROM t1;
+a
+aaaaaaaaaaaaaa
+DROP TABLE t1;
 End of 5.1 tests

=== modified file 'mysql-test/t/func_str.test'
--- a/mysql-test/t/func_str.test	2010-12-14 16:08:25 +0000
+++ b/mysql-test/t/func_str.test	2010-12-15 11:00:25 +0000
@@ -1369,4 +1369,15 @@ DROP TABLE t1;
 SELECT '1' IN ('1', SUBSTRING(-9223372036854775809, 1));
 SELECT CONVERT(('' IN (REVERSE(CAST(('') AS DECIMAL)), '')), CHAR(3));
 
+--echo # 
+--echo # Bug#58165: "my_empty_string" gets modified and causes LOAD DATA to fail
+--echo # and other crashes
+--echo #
+CREATE TABLE t1 ( a TEXT );
+SELECT 'aaaaaaaaaaaaaa' INTO OUTFILE 'bug58165.txt';
+SELECT insert( substring_index( 'a', 'a', 'b' ), 1, 0, 'x' );
+LOAD DATA INFILE 'bug58165.txt' INTO TABLE t1;
+SELECT * FROM t1;
+DROP TABLE t1;
+
 --echo End of 5.1 tests

=== modified file 'sql/item_strfunc.cc'
--- a/sql/item_strfunc.cc	2010-10-12 19:28:03 +0000
+++ b/sql/item_strfunc.cc	2010-12-15 11:00:25 +0000
@@ -39,6 +39,12 @@ C_MODE_START
 #include "../mysys/my_static.h"			// For soundex_map
 C_MODE_END
 
+/*
+  This is an inherently dangerous design and should be abandoned. Our String
+  class does not support safely mutable strings. Functions that return
+  non-const String objects should either allocate buffers or use pre-allocated
+  ones.
+ */
 String my_empty_string("",default_charset_info);
 
 
@@ -1047,6 +1053,12 @@ String *Item_func_insert::val_str(String
     goto null;
   }
   res=copy_if_not_alloced(str,res,res->length());
+  /* This shows the dangers of returning my_empty_string. */
+  if (res == &my_empty_string)
+  {
+    str->copy("", 0, default_charset_info);
+    res= str;
+  }
   res->replace((uint32) start,(uint32) length,*res2);
   return res;
 null:


Attachment: [text/bzr-bundle] bzr/[e-mail]