List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:January 13 2011 8:21am
Subject:bzr push into mysql-trunk branch (martin.hansson:3493 to 3494) Bug#58165
View as plain text  
 3494 Martin Hansson	2011-01-13 [merge]
      Merge of fix for Bug#58165.

    modified:
      mysql-test/r/func_str.result
      mysql-test/t/func_str.test
      sql/item_strfunc.cc
      sql/item_strfunc.h
      sql/sql_string.cc
      sql/sql_string.h
 3493 Olav Sandstaa	2011-01-13
      Fix for Bug#58816 Extra temporary duplicate rows in result set when 
                        switching ICP off.
      
      The wrong result was caused by that the handler object was in an
      inconsistent state when running the query. The same handler object
      was previously used by an explain for the same query. 
      
      When doing the explain the optimizer pushed down an index condition to
      the handler. This set information about the pushed index condition
      as well as setting the member variable in_range_check_pushed_down to
      true. The inconsistency that this resulted in was that when the
      explain statement completed and the handler object was ready for
      re-use the information about the pushed index condition was reset but
      the in_range_check_pushed_down was still true.
      
      When executing the same query (after disabling index condition
      pushdown) using the same handler object with the
      in_range_check_pushed_down still being true caused that neither the
      server nor the storage engine were performing the range check. This
      resulted in that we would read to the end of the table for the first
      range instead of stopping at the end of range criterion.
      
      The fix for this problem is to add code to handler::ha_reset() to
      to reset both information about the pushed index condition and the
      in_range_check_pushed_down. This method is called when a
      statement closes its tables and the handler object is made ready
      for reuse.
     @ mysql-test/include/icp_tests.inc
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/innodb_icp.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/innodb_icp_all.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/innodb_icp_none.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/myisam_icp.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/myisam_icp_all.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ mysql-test/r/myisam_icp_none.result
        Test case for Bug#58816 Extra temporary duplicate rows in result
        set when switching ICP off.
     @ sql/handler.cc
        When an explain is run on a statement using index condition pushdown
        it will leave the handler object in an inconsistent state where
        the information about the pushed index condition is reset while
        the value of in_range_check_pushed_down is still true. If this
        handler object is later re-used for a range query it might lead
        to more records being found due to the range condition is not
        evaluated neither by the server nor the storage engine.
        
        The fix for this is to extend handler::ha_reset() to reset
        both the information about pushed index condition and the
        in_range_check_pushed_down.
     @ storage/innobase/handler/ha_innodb.cc
        Move resetting of the handler's ICP member variables to handler::ha_reset().
     @ storage/myisam/ha_myisam.cc
        Move resetting of the handler's ICP member variables to handler::ha_reset().

    modified:
      mysql-test/include/icp_tests.inc
      mysql-test/r/innodb_icp.result
      mysql-test/r/innodb_icp_all.result
      mysql-test/r/innodb_icp_none.result
      mysql-test/r/myisam_icp.result
      mysql-test/r/myisam_icp_all.result
      mysql-test/r/myisam_icp_none.result
      sql/handler.cc
      storage/innobase/handler/ha_innodb.cc
      storage/myisam/ha_myisam.cc
=== modified file 'mysql-test/r/func_str.result'
--- a/mysql-test/r/func_str.result	2010-12-14 16:29:15 +0000
+++ b/mysql-test/r/func_str.result	2011-01-13 08:19:52 +0000
@@ -2615,6 +2615,22 @@ 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
 Start of 5.4 tests
 SELECT format(12345678901234567890.123, 3);

=== modified file 'mysql-test/t/func_str.test'
--- a/mysql-test/t/func_str.test	2010-12-14 16:29:15 +0000
+++ b/mysql-test/t/func_str.test	2011-01-13 08:19:52 +0000
@@ -1370,6 +1370,17 @@ 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
 
 --echo Start of 5.4 tests

=== modified file 'sql/item_strfunc.cc'
--- a/sql/item_strfunc.cc	2010-12-20 10:28:06 +0000
+++ b/sql/item_strfunc.cc	2011-01-13 08:19:52 +0000
@@ -57,6 +57,9 @@ C_MODE_START
 #include "../mysys/my_static.h"			// For soundex_map
 C_MODE_END
 
+/**
+   @todo Remove this. It is not safe to use a shared String object.
+ */
 String my_empty_string("",default_charset_info);
 
 /*
@@ -739,7 +742,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 make_empty_result();
   if (arg_count == 1)
   {
     /* Protect against someone doing FLUSH DES_KEY_FILE */
@@ -929,7 +932,7 @@ String *Item_func_concat_ws::val_str(Str
     }
 
   if (i ==  arg_count)
-    return &my_empty_string;
+    return make_empty_result();
 
   for (i++; i < arg_count ; i++)
   {
@@ -1075,7 +1078,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 make_empty_result();
   if (tmp_value.alloced_length() < res->length() &&
       tmp_value.realloc(res->length()))
   {
@@ -1408,8 +1411,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 make_empty_result();
   if ((res->length() <= (ulonglong) length) ||
       (res->length() <= (char_pos= res->charpos((int) length))))
     return res;
@@ -1454,7 +1456,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 make_empty_result(); /* purecov: inspected */
 
   if (res->length() <= (ulonglong) length)
     return res; /* purecov: inspected */
@@ -1494,7 +1496,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 make_empty_result();
 
   /* 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. */
@@ -1505,12 +1507,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 make_empty_result();
 
   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 make_empty_result();
 
   length= res->charpos((int) length, (uint32) start);
   tmp_length= res->length() - start;
@@ -1573,7 +1575,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 make_empty_result();		// Wrong parameters
 
   res->set_charset(collation.collation);
 
@@ -1923,7 +1925,7 @@ String *Item_func_password::val_str_asci
   if ((null_value=args[0]->null_value))
     return 0;
   if (res->length() == 0)
-    return &my_empty_string;
+    return make_empty_result();
   my_make_scrambled_password(tmp_value, res->ptr(), res->length());
   str->set(tmp_value, SCRAMBLED_PASSWORD_CHAR_LENGTH, &my_charset_latin1);
   return str;
@@ -1947,7 +1949,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 make_empty_result();
   my_make_scrambled_password_323(tmp_value, res->ptr(), res->length());
   str->set(tmp_value, SCRAMBLED_PASSWORD_CHAR_LENGTH_323, &my_charset_latin1);
   return str;
@@ -1975,8 +1977,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 make_empty_result();
   if (arg_count == 1)
   {					// generate random salt
     time_t timestamp=current_thd->query_start();
@@ -2238,7 +2239,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 make_empty_result(); /* EOL or invalid byte sequence */
     
     if (rc == 1 && cs->ctype)
     {
@@ -2263,7 +2264,7 @@ String *Item_func_soundex::val_str(Strin
         {
           /* Extra safety - should not really happen */
           DBUG_ASSERT(false);
-          return &my_empty_string;
+          return make_empty_result();
         }
         to+= rc;
         break;
@@ -2604,7 +2605,7 @@ String *Item_func_make_set::val_str(Stri
 	  else
 	  {
 	    if (tmp_str.copy(*res))		// Don't use 'str'
-	      return &my_empty_string;
+              return make_empty_result();
 	    result= &tmp_str;
 	  }
 	}
@@ -2614,11 +2615,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 make_empty_result();
 	    result= &tmp_str;
 	  }
 	  if (tmp_str.append(STRING_WITH_LEN(","), &my_charset_bin) || tmp_str.append(*res))
-	    return &my_empty_string;
+            return make_empty_result();
 	}
       }
     }
@@ -2763,7 +2764,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 make_empty_result();
 
   /* Assumes that the maximum length of a String is < INT_MAX32. */
   /* Bounds check on count:  If this is triggered, we will error. */
@@ -3045,7 +3046,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 make_empty_result();
   return str;
 }
 
@@ -3264,7 +3265,7 @@ String *Item_func_hex::val_str_ascii(Str
       return 0;
     ptr= longlong2str(dec,ans,16);
     if (str->copy(ans,(uint32) (ptr-ans), &my_charset_numeric))
-      return &my_empty_string;			// End of memory
+      return make_empty_result();		// End of memory
     return str;
   }
 

=== modified file 'sql/item_strfunc.h'
--- a/sql/item_strfunc.h	2010-12-02 13:44:21 +0000
+++ b/sql/item_strfunc.h	2011-01-13 08:19:52 +0000
@@ -27,6 +27,16 @@ class MY_LOCALE;
 
 class Item_str_func :public Item_func
 {
+protected:
+  /**
+     Sets the result value of the function an empty string, using the current
+     character set. No memory is allocated.
+     @retval A pointer to the str_value member.
+   */
+  String *make_empty_result() {
+    str_value.set("", 0, collation.collation);
+    return &str_value; 
+  }
 public:
   Item_str_func() :Item_func() { decimals=NOT_FIXED_DEC; }
   Item_str_func(Item *a) :Item_func(a) {decimals=NOT_FIXED_DEC; }

=== modified file 'sql/sql_string.cc'
--- a/sql/sql_string.cc	2010-12-14 13:26:35 +0000
+++ b/sql/sql_string.cc	2011-01-13 08:19:52 +0000
@@ -51,11 +51,33 @@ bool String::real_alloc(uint32 length)
 }
 
 
-/*
-** 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,
+     one of the following is performed.
+
+     - If the requested length is greater than what fits in the buffer, a new
+       buffer is allocated, data moved and the old buffer freed.
+
+     - If the requested length is less or equal to what fits in the buffer, a
+       null character is inserted at the appropriate position.
 
+   - 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 requested string size in characters, excluding any
+   null terminator.
+
+   @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);
@@ -128,6 +150,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 is to be copied.
+   
+   @retval false Success.
+   @retval true Memory allocation failed.
+*/
 bool String::copy(const String &str)
 {
   if (alloc(str.str_length))

=== modified file 'sql/sql_string.h'
--- a/sql/sql_string.h	2010-10-27 14:46:44 +0000
+++ b/sql/sql_string.h	2011-01-13 08:19:52 +0000
@@ -148,6 +148,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();


Attachment: [text/bzr-bundle] bzr/martin.hansson@oracle.com-20110113081952-n7mg2swpu1xb5fjp.bundle
Thread
bzr push into mysql-trunk branch (martin.hansson:3493 to 3494) Bug#58165Martin Hansson13 Jan