List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:April 12 2012 2:29pm
Subject:bzr push into mysql-trunk branch (tor.didriksen:3870 to 3871) Bug#13871079
View as plain text  
 3871 Tor Didriksen	2012-04-12
      Bug#13871079 RQG_MYISAM_DML_ALTER_VALGRIND FAILS ON VALGRIND PN PB2
      
      The class Copy_field contains a String tmp, 
      which may allocate memory on the heap.
      That means that all instances of Copy_field
      must be properly destroyed. Alas they are not.
      
      Solution: don't use Copy_field::tmp for copying
      from_field => tmp => to_field
      in do_field_string()
     @ sql/field.cc
        In Field_set::val_str
        return empty string (of appropriate character set) for an empty set.
     @ sql/field.h
        New private member in Field_enum: empty_set_string.
     @ sql/field_conv.cc
        In do_field_string, use an auto variable for copying
        from_field => tmp => to_field
        rather than copy->tmp.
     @ sql/sql_class.h
        Verifies that these objects are never destroyed....
     @ unittest/gunit/alignment-t.cc
        Add copyright notice.
     @ unittest/gunit/fake_table.h
        More initializations, to avoid valgrind warnings.
     @ unittest/gunit/field-t.cc
        New unit test.
     @ unittest/gunit/field_timestamp-t.cc
        Remove obsolete note about min/max macros.

    modified:
      sql/field.cc
      sql/field.h
      sql/field_conv.cc
      sql/sql_class.h
      unittest/gunit/alignment-t.cc
      unittest/gunit/fake_table.h
      unittest/gunit/field-t.cc
      unittest/gunit/field_timestamp-t.cc
 3870 Sujatha Sivakumar	2012-04-12 [merge]
      upmerge from mysql-5.5 -> mysql-trunk.

    modified:
      mysql-test/extra/rpl_tests/rpl_row_basic.test
      mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result
      mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result
      mysql-test/suite/rpl/r/rpl_row_basic_allow_batching.result
      sql/log_event.cc
=== modified file 'sql/field.cc'
--- a/sql/field.cc	2012-03-30 15:38:01 +0000
+++ b/sql/field.cc	2012-04-12 14:29:14 +0000
@@ -8506,7 +8506,19 @@ String *Field_set::val_str(String *val_b
   ulonglong tmp=(ulonglong) Field_enum::val_int();
   uint bitnr=0;
 
-  val_buffer->set("", 0, field_charset);
+  if (tmp == 0)
+  {
+    /*
+      Some callers expect *val_buffer to contain the result,
+      so we assign to it, rather than doing 'return &empty_set_string.
+     */
+    *val_buffer= empty_set_string;
+    return val_buffer;
+  }
+
+  val_buffer->set_charset(field_charset);
+  val_buffer->length(0);
+
   while (tmp && bitnr < (uint) typelib->count)
   {
     if (tmp & 1)

=== modified file 'sql/field.h'
--- a/sql/field.h	2012-03-06 14:29:42 +0000
+++ b/sql/field.h	2012-04-12 14:29:14 +0000
@@ -3250,7 +3250,8 @@ public:
     :Field_enum(ptr_arg, len_arg, null_ptr_arg, null_bit_arg,
 		    unireg_check_arg, field_name_arg,
                 packlength_arg,
-                typelib_arg,charset_arg)
+                typelib_arg,charset_arg),
+      empty_set_string("", 0, charset_arg)
     {
       flags= (flags & ~ENUM_FLAG) | SET_FLAG;
     }
@@ -3270,6 +3271,8 @@ public:
     DBUG_ASSERT(real_type() == MYSQL_TYPE_SET);
     return new Field_set(*this);
   }
+private:
+  const String empty_set_string;
 };
 
 

=== modified file 'sql/field_conv.cc'
--- a/sql/field_conv.cc	2012-03-06 14:29:42 +0000
+++ b/sql/field_conv.cc	2012-04-12 14:29:14 +0000
@@ -291,10 +291,11 @@ static void do_save_blob(Copy_field *cop
 static void do_field_string(Copy_field *copy)
 {
   char buff[MAX_FIELD_WIDTH];
-  copy->tmp.set_quick(buff,sizeof(buff),copy->tmp.charset());
-  copy->from_field->val_str(&copy->tmp);
-  copy->to_field->store(copy->tmp.c_ptr_quick(),copy->tmp.length(),
-                        copy->tmp.charset());
+  String res(buff, sizeof(buff), copy->from_field->charset());
+  res.length(0U);
+
+  copy->from_field->val_str(&res);
+  copy->to_field->store(res.c_ptr_quick(), res.length(), res.charset());
 }
 
 
@@ -568,7 +569,7 @@ void Copy_field::set(uchar *to,Field *fr
 /*
   To do: 
 
-  If 'save\ is set to true and the 'from' is a blob field, do_copy is set to
+  If 'save' is set to true and the 'from' is a blob field, do_copy is set to
   do_save_blob rather than do_conv_blob.  The only differences between them
   appears to be:
 

=== modified file 'sql/sql_class.h'
--- a/sql/sql_class.h	2012-03-21 21:12:45 +0000
+++ b/sql/sql_class.h	2012-04-12 14:29:14 +0000
@@ -4530,8 +4530,13 @@ public:
     table(NULL), tab_ref(NULL), in_equality(NULL),
     join_cond(NULL), copy_field(NULL)
   {}
-  ~Semijoin_mat_exec() {}
-
+private:
+  // Nobody deletes me apparently ...
+  ~Semijoin_mat_exec()
+  {
+    delete [] copy_field;
+  }
+public:
   const uint table_count;       // Number of tables in the sj-nest
   const bool is_scan;           // TRUE if executing as a scan, FALSE if lookup
   bool materialized;            // TRUE <=> materialization has been performed

=== modified file 'unittest/gunit/alignment-t.cc'
--- a/unittest/gunit/alignment-t.cc	2012-01-11 09:33:52 +0000
+++ b/unittest/gunit/alignment-t.cc	2012-04-12 14:29:14 +0000
@@ -1,3 +1,18 @@
+/* Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; version 2 of the License.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02111-1307  USA */
+
 // First include (the generated) my_config.h, to get correct platform defines.
 #include "my_config.h"
 #include <gtest/gtest.h>

=== modified file 'unittest/gunit/fake_table.h'
--- a/unittest/gunit/fake_table.h	2012-02-27 09:09:31 +0000
+++ b/unittest/gunit/fake_table.h	2012-04-12 14:29:14 +0000
@@ -30,6 +30,7 @@ public:
     // fix if you plan to test with >32 columns.
     column_bitmap_size= sizeof(int);
     tmp_table= NO_TMP_TABLE;
+    db_low_byte_first= true;
   }
 };
 
@@ -61,6 +62,7 @@ class Fake_TABLE: public TABLE
       field[i]->field_index= i;
     }
     const_table= true;
+    maybe_null= 0;
   }
 
 public:

=== modified file 'unittest/gunit/field-t.cc'
--- a/unittest/gunit/field-t.cc	2011-12-20 12:04:31 +0000
+++ b/unittest/gunit/field-t.cc	2012-04-12 14:29:14 +0000
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved. 
+/* Copyright (c) 2011, 2012, Oracle and/or its affiliates. All rights reserved. 
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -18,6 +18,7 @@
 #include <gtest/gtest.h>
 
 #include "test_utils.h"
+#include "fake_table.h"
 
 #include "field.h"
 #include "sql_time.h"
@@ -30,29 +31,17 @@ using my_testing::Mock_error_handler;
 class FieldTest : public ::testing::Test
 {
 protected:
-  static void SetUpTestCase()
-  {
-    Server_initializer::SetUpTestCase();
-  }
-
-  static void TearDownTestCase()
-  {
-    Server_initializer::TearDownTestCase();
-  }
-
-  virtual void SetUp()
-  {
-    initializer.SetUp();
-  }
+  static void SetUpTestCase() { Server_initializer::SetUpTestCase(); }
+  static void TearDownTestCase() { Server_initializer::TearDownTestCase(); }
 
-  virtual void TearDown()
-  {
-    initializer.TearDown();
-  }
+  virtual void SetUp() { initializer.SetUp(); }
+  virtual void TearDown() { initializer.TearDown(); }
 
   THD *thd() { return initializer.thd(); }
 
   Server_initializer initializer;
+
+  Field_set *create_field_set(TYPELIB *tl);
 };
 
 
@@ -361,4 +350,61 @@ TEST_F(FieldTest, FieldTime)
 }
 
 
+const char *type_names3[]= { "one", "two", "three", NULL };
+unsigned int type_lengths3[]= { 3U, 3U, 5U, 0U };
+TYPELIB tl3= { 3, "tl3", type_names3, type_lengths3 };
+
+const char *type_names4[]= { "one", "two", "three", "four", NULL };
+unsigned int type_lengths4[]= { 3U, 3U, 5U, 4U, 0U };
+TYPELIB tl4= { 4, "tl4", type_names4, type_lengths4 };
+
+
+Field_set *FieldTest::create_field_set(TYPELIB *tl)
+{
+  Field_set *f= new (thd()->mem_root)
+    Field_set(NULL,                             // ptr_arg
+              42,                               // len_arg
+              NULL,                             // null_ptr_arg
+              '\0',                             // null_bit_arg
+              Field::NONE,                      // unireg_check_arg
+              "f1",                             // field_name_arg
+              1,                                // packlength_arg
+              tl,                               // typelib_arg
+              &my_charset_latin1);              // charset_arg
+  f->table= new Fake_TABLE(f);
+  return f;
+}
+
+
+// Bug#13871079 RQG_MYISAM_DML_ALTER_VALGRIND FAILS ON VALGRIND PN PB2
+TEST_F(FieldTest, CopyFieldSet)
+{
+  int err;
+  char fields[]= "one,two";
+  my_ulonglong typeset= find_typeset(fields, &tl3, &err);
+  EXPECT_EQ(0, err);
+
+  // Using two different TYPELIBs will set cf->do_copy to do_field_string().
+  Field_set *f_to= create_field_set(&tl3);
+  bitmap_set_all(f_to->table->write_set);
+  uchar to_fieldval= 0;
+  f_to->ptr= &to_fieldval;
+
+  Field_set *f_from= create_field_set(&tl4);
+  bitmap_set_all(f_from->table->write_set);
+  uchar from_fieldval= static_cast<uchar>(typeset);
+  f_from->ptr= &from_fieldval;
+
+  Copy_field *cf= new (thd()->mem_root) Copy_field;
+  cf->set(f_to, f_from, false);
+  cf->do_copy(cf);
+
+  // Copy_field DTOR is not invoked in all contexts, so we may leak memory.
+  EXPECT_FALSE(cf->tmp.is_alloced());
+
+  delete f_to->table;
+  delete f_from->table;
+}
+
+
 }

=== modified file 'unittest/gunit/field_timestamp-t.cc'
--- a/unittest/gunit/field_timestamp-t.cc	2012-01-31 15:16:16 +0000
+++ b/unittest/gunit/field_timestamp-t.cc	2012-04-12 14:29:14 +0000
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2011, 2012, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -13,8 +13,7 @@
    along with this program; if not, write to the Free Software Foundation,
    51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA */
 
-// First include (the generated) my_config.h, to get correct platform defines,
-// then gtest.h (before any other MySQL headers), to avoid min() macros etc ...
+// First include (the generated) my_config.h, to get correct platform defines.
 #include "my_config.h"
 #include <gtest/gtest.h>
 

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk branch (tor.didriksen:3870 to 3871) Bug#13871079Tor Didriksen12 Apr