From: Tor Didriksen Date: April 12 2012 2:29pm Subject: bzr push into mysql-trunk branch (tor.didriksen:3870 to 3871) Bug#13871079 List-Archive: http://lists.mysql.com/commits/143468 X-Bug: 13871079 Message-Id: <201204121429.q3CETh1F003928@acsmt358.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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(©->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 === 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 #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(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 No bundle (reason: useless for push emails).