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

[Martin Hansson]

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

 3525 Martin Hansson	2010-12-16
      Bug#58165: "my_empty_string" gets modified and causes LOAD DATA to fail and
      other crashes
      
      The string manipulating SQL function insert() used a shared string buffer
      intended to contain an immutable empty string causing errors. With our code
      base this design faced two obstacles. The implementations of string valued SQL
      functions return pointers to String objects rather than the objects themselves
      and even if this were rectified we still have no protection against
      manipulation of shared string buffers.
      
      Fixed by always allocating a new String object whenever a function returns an
      empty string.
      
      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
      sql/sql_string.h
=== 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-16 12:36:35 +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-16 12:36:35 +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-16 12:36:35 +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-16 12:36:35 +0000
@@ -461,7 +461,7 @@ String *Item_func_des_encrypt::val_str(S
   if ((null_value= args[0]->null_value))
     return 0;                                   // ENCRYPT(NULL) == NULL
   if ((res_length=res->length()) == 0)
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   if (arg_count == 1)
   {
@@ -652,7 +652,7 @@ String *Item_func_concat_ws::val_str(Str
     }
 
   if (i ==  arg_count)
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   for (i++; i < arg_count ; i++)
   {
@@ -803,7 +803,7 @@ String *Item_func_reverse::val_str(Strin
     return 0;
   /* An empty string is a special case as the string pointer may be null */
   if (!res->length())
-    return &my_empty_string;
+    return new String("", default_charset_info);
   if (tmp_value.alloced_length() < res->length() &&
       tmp_value.realloc(res->length()))
   {
@@ -1137,7 +1137,7 @@ String *Item_func_left::val_str(String *
 
   /* if "unsigned_flag" is set, we have a *huge* positive number. */
   if ((length <= 0) && (!args[1]->unsigned_flag))
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   if ((res->length() <= (ulonglong) length) ||
       (res->length() <= (char_pos= res->charpos((int) length))))
@@ -1181,7 +1181,7 @@ String *Item_func_right::val_str(String 
 
   /* if "unsigned_flag" is set, we have a *huge* positive number. */
   if ((length <= 0) && (!args[1]->unsigned_flag))
-    return &my_empty_string; /* purecov: inspected */
+    return new String("", default_charset_info); /* purecov: inspected */
 
   if (res->length() <= (ulonglong) length)
     return res; /* purecov: inspected */
@@ -1220,7 +1220,7 @@ String *Item_func_substr::val_str(String
   /* Negative or zero length, will return empty string. */
   if ((arg_count == 3) && (length <= 0) && 
       (length == 0 || !args[2]->unsigned_flag))
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   /* Assumes that the maximum length of a String is < INT_MAX32. */
   /* Set here so that rest of code sees out-of-bound value as such. */
@@ -1231,12 +1231,12 @@ String *Item_func_substr::val_str(String
   /* Assumes that the maximum length of a String is < INT_MAX32. */
   if ((!args[1]->unsigned_flag && (start < INT_MIN32 || start > INT_MAX32)) ||
       (args[1]->unsigned_flag && ((ulonglong) start > INT_MAX32)))
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   start= ((start < 0) ? res->numchars() + start : start - 1);
   start= res->charpos((int) start);
   if ((start < 0) || ((uint) start + 1 > res->length()))
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   length= res->charpos((int) length, (uint32) start);
   tmp_length= res->length() - start;
@@ -1299,7 +1299,7 @@ String *Item_func_substr_index::val_str(
   null_value=0;
   uint delimiter_length= delimiter->length();
   if (!res->length() || !delimiter_length || !count)
-    return &my_empty_string;		// Wrong parameters
+    return new String("", default_charset_info);		// Wrong parameters
 
   res->set_charset(collation.collation);
 
@@ -1648,7 +1648,7 @@ String *Item_func_password::val_str(Stri
   if ((null_value=args[0]->null_value))
     return 0;
   if (res->length() == 0)
-    return &my_empty_string;
+    return new String("", default_charset_info);
   my_make_scrambled_password(tmp_value, res->ptr(), res->length());
   str->set(tmp_value, SCRAMBLED_PASSWORD_CHAR_LENGTH, res->charset());
   return str;
@@ -1672,7 +1672,7 @@ String *Item_func_old_password::val_str(
   if ((null_value=args[0]->null_value))
     return 0;
   if (res->length() == 0)
-    return &my_empty_string;
+    return new String("", default_charset_info);
   my_make_scrambled_password_323(tmp_value, res->ptr(), res->length());
   str->set(tmp_value, SCRAMBLED_PASSWORD_CHAR_LENGTH_323, res->charset());
   return str;
@@ -1700,7 +1700,7 @@ String *Item_func_encrypt::val_str(Strin
   if ((null_value=args[0]->null_value))
     return 0;
   if (res->length() == 0)
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   if (arg_count == 1)
   {					// generate random salt
@@ -1961,7 +1961,7 @@ String *Item_func_soundex::val_str(Strin
   for ( ; ; ) /* Skip pre-space */
   {
     if ((rc= cs->cset->mb_wc(cs, &wc, (uchar*) from, (uchar*) end)) <= 0)
-      return &my_empty_string; /* EOL or invalid byte sequence */
+      return new String("", default_charset_info); /* EOL or invalid byte sequence */
     
     if (rc == 1 && cs->ctype)
     {
@@ -1986,7 +1986,7 @@ String *Item_func_soundex::val_str(Strin
         {
           /* Extra safety - should not really happen */
           DBUG_ASSERT(false);
-          return &my_empty_string;
+          return new String("", default_charset_info);
         }
         to+= rc;
         break;
@@ -2259,7 +2259,11 @@ String *Item_func_make_set::val_str(Stri
   ulonglong bits;
   bool first_found=0;
   Item **ptr=args;
-  String *result=&my_empty_string;
+  String *empty_string= new String("", default_charset_info);
+  if (empty_string == NULL)
+    /* Out of memory */
+    return NULL;
+  String *result= empty_string;
 
   bits=item->val_int();
   if ((null_value=item->null_value))
@@ -2283,7 +2287,7 @@ String *Item_func_make_set::val_str(Stri
 	  else
 	  {
 	    if (tmp_str.copy(*res))		// Don't use 'str'
-	      return &my_empty_string;
+	      return empty_string;
 	    result= &tmp_str;
 	  }
 	}
@@ -2293,11 +2297,11 @@ String *Item_func_make_set::val_str(Stri
 	  {					// Copy data to tmp_str
 	    if (tmp_str.alloc(result->length()+res->length()+1) ||
 		tmp_str.copy(*result))
-	      return &my_empty_string;
+	      return empty_string;
 	    result= &tmp_str;
 	  }
 	  if (tmp_str.append(STRING_WITH_LEN(","), &my_charset_bin) || tmp_str.append(*res))
-	    return &my_empty_string;
+	    return empty_string;
 	}
       }
     }
@@ -2436,7 +2440,7 @@ String *Item_func_repeat::val_str(String
   null_value= 0;
 
   if (count <= 0 && (count == 0 || !args[1]->unsigned_flag))
-    return &my_empty_string;
+    return new String("", default_charset_info);
 
   /* Assumes that the maximum length of a String is < INT_MAX32. */
   /* Bounds check on count:  If this is triggered, we will error. */
@@ -2744,7 +2748,7 @@ String *Item_func_conv::val_str(String *
 
   ptr= longlong2str(dec, ans, to_base);
   if (str->copy(ans, (uint32) (ptr-ans), default_charset()))
-    return &my_empty_string;
+    return new String("", default_charset_info);
   return str;
 }
 
@@ -2911,7 +2915,7 @@ String *Item_func_hex::val_str(String *s
       return 0;
     ptr= longlong2str(dec,ans,16);
     if (str->copy(ans,(uint32) (ptr-ans),default_charset()))
-      return &my_empty_string;			// End of memory
+      return new String("", default_charset_info);			// End of memory
     return str;
   }
 

=== modified file 'sql/sql_string.h'
--- a/sql/sql_string.h	2010-10-19 22:36:59 +0000
+++ b/sql/sql_string.h	2010-12-16 12:36:35 +0000
@@ -82,6 +82,10 @@ public:
     Alloced_length=str.Alloced_length; alloced=0; 
     str_charset=str.str_charset;
   }
+  static void *operator new(size_t size) throw ()
+  {
+    return sql_alloc(size);
+  }
   static void *operator new(size_t size, MEM_ROOT *mem_root) throw ()
   { return (void*) alloc_root(mem_root, (uint) size); }
   static void operator delete(void *ptr_arg, size_t size)


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