Hi Tor,
The patch looks good. Thanks for adding copy constructor and assignment
operators and a google test for this.
One minor comments that you should feel free to ignore:
* In Item_func_int_div::val_int(): We have now five my_decimal objects,
I think tree of these are just needed for having as input arguments to
the various calls and are never used after the calls. So the following
my_decimal objects:
my_decimal value0, value1, tmp;
could probably be replaced by just on my_decimal object?
OK to push.
Olav
On 13/01/2011 12:37, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/trunk-bug59241divmod/ based on
> revid:ole.john.aske@stripped
>
> 3498 Tor Didriksen 2011-01-13
> Bug #59241 invalid memory read in do_div_mod with doubly assigned variables
>
> Fix: copy my_decimal by value, to avoid dangling pointers.
> @ mysql-test/r/func_math.result
> New test case.
> @ mysql-test/t/func_math.test
> New test case.
> @ sql/item_cmpfunc.cc
> No need to call fix_buffer_pointer() anymore.
> @ sql/item_func.cc
> Copy my_decimal by value, to avoid dangling pointers.
> @ sql/my_decimal.h
> Implement proper copy constructor and assignment operator for my_decimal.
> @ sql/sql_analyse.cc
> No need to call fix_buffer_pointer() anymore.
> @ strings/decimal.c
> Remove #line directive: it messes up TAGS and it confuses gdb when
> debugging.
> @ unittest/gunit/CMakeLists.txt
> New unit test.
> @ unittest/gunit/my_decimal-t.cc
> Unit test for my_decimal copy constructor and assignment operator.
>
> added:
> unittest/gunit/my_decimal-t.cc
> modified:
> mysql-test/r/func_math.result
> mysql-test/t/func_math.test
> sql/item_cmpfunc.cc
> sql/item_func.cc
> sql/my_decimal.h
> sql/sql_analyse.cc
> strings/decimal.c
> unittest/gunit/CMakeLists.txt
> === modified file 'mysql-test/r/func_math.result'
> --- a/mysql-test/r/func_math.result 2010-12-24 11:30:28 +0000
> +++ b/mysql-test/r/func_math.result 2011-01-13 11:37:42 +0000
> @@ -641,3 +641,12 @@ INSERT INTO t1 (SELECT -pi());
> Warnings:
> Warning 1265 Data truncated for column 'a' at row 1
> DROP TABLE t1;
> +#
> +# Bug #59241 invalid memory read
> +# in do_div_mod with doubly assigned variables
> +#
> +SELECT ((@a:=@b:=1.0) div (@b:=@a:=get_format(datetime, 'usa')));
> +((@a:=@b:=1.0) div (@b:=@a:=get_format(datetime, 'usa')))
> +NULL
> +Warnings:
> +Warning 1366 Incorrect decimal value: '' for column '' at row -1
>
> === modified file 'mysql-test/t/func_math.test'
> --- a/mysql-test/t/func_math.test 2010-12-24 11:30:28 +0000
> +++ b/mysql-test/t/func_math.test 2011-01-13 11:37:42 +0000
> @@ -489,3 +489,9 @@ as foo;
> CREATE TABLE t1(a char(0));
> INSERT INTO t1 (SELECT -pi());
> DROP TABLE t1;
> +
> +--echo #
> +--echo # Bug #59241 invalid memory read
> +--echo # in do_div_mod with doubly assigned variables
> +--echo #
> +SELECT ((@a:=@b:=1.0) div (@b:=@a:=get_format(datetime, 'usa')));
>
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc 2011-01-13 09:08:47 +0000
> +++ b/sql/item_cmpfunc.cc 2011-01-13 11:37:42 +0000
> @@ -2172,7 +2172,6 @@ void Item_func_interval::fix_length_and_
> if (dec !=&range->dec)
> {
> range->dec= *dec;
> - range->dec.fix_buffer_pointer();
> }
> }
> else
>
> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc 2010-12-29 00:38:59 +0000
> +++ b/sql/item_func.cc 2011-01-13 11:37:42 +0000
> @@ -1600,17 +1600,16 @@ longlong Item_func_int_div::val_int()
> args[1]->result_type() != INT_RESULT)
> {
> my_decimal value0, value1, tmp;
> - my_decimal *val0, *val1;
> longlong res;
> int err;
>
> - val0= args[0]->val_decimal(&value0);
> - val1= args[1]->val_decimal(&value1);
> + my_decimal val0= *args[0]->val_decimal(&value0);
> + my_decimal val1= *args[1]->val_decimal(&value1);
> if ((null_value= (args[0]->null_value || args[1]->null_value)))
> return 0;
>
> if ((err= my_decimal_div(E_DEC_FATAL_ERROR& ~E_DEC_DIV_ZERO,&tmp,
> - val0, val1, 0))> 3)
> +&val0,&val1, 0))> 3)
> {
> if (err == E_DEC_DIV_ZERO)
> signal_divide_by_null();
>
> === modified file 'sql/my_decimal.h'
> --- a/sql/my_decimal.h 2010-10-19 22:58:36 +0000
> +++ b/sql/my_decimal.h 2011-01-13 11:37:42 +0000
> @@ -102,6 +102,24 @@ class my_decimal :public decimal_t
>
> public:
>
> + my_decimal(const my_decimal&rhs) : decimal_t(rhs)
> + {
> + for (uint i= 0; i< DECIMAL_BUFF_LENGTH; i++)
> + buffer[i]= rhs.buffer[i];
> + fix_buffer_pointer();
> + }
> +
> + my_decimal& operator=(const my_decimal&rhs)
> + {
> + if (this ==&rhs)
> + return *this;
> + decimal_t::operator=(rhs);
> + for (uint i= 0; i< DECIMAL_BUFF_LENGTH; i++)
> + buffer[i]= rhs.buffer[i];
> + fix_buffer_pointer();
> + return *this;
> + }
> +
> void init()
> {
> len= DECIMAL_BUFF_LENGTH;
> @@ -248,7 +266,6 @@ inline
> void my_decimal2decimal(const my_decimal *from, my_decimal *to)
> {
> *to= *from;
> - to->fix_buffer_pointer();
> }
>
>
>
> === modified file 'sql/sql_analyse.cc'
> --- a/sql/sql_analyse.cc 2010-07-16 21:00:50 +0000
> +++ b/sql/sql_analyse.cc 2011-01-13 11:37:42 +0000
> @@ -521,9 +521,6 @@ void field_decimal::add()
> {
> found = 1;
> min_arg = max_arg = sum[0] = *dec;
> - min_arg.fix_buffer_pointer();
> - max_arg.fix_buffer_pointer();
> - sum[0].fix_buffer_pointer();
> my_decimal_mul(E_DEC_FATAL_ERROR, sum_sqr, dec, dec);
> cur_sum= 0;
> min_length = max_length = length;
> @@ -545,12 +542,10 @@ void field_decimal::add()
> if (my_decimal_cmp(dec,&min_arg)< 0)
> {
> min_arg= *dec;
> - min_arg.fix_buffer_pointer();
> }
> if (my_decimal_cmp(dec,&max_arg)> 0)
> {
> max_arg= *dec;
> - max_arg.fix_buffer_pointer();
> }
> }
> }
>
> === modified file 'strings/decimal.c'
> --- a/strings/decimal.c 2010-07-20 19:30:10 +0000
> +++ b/strings/decimal.c 2011-01-13 11:37:42 +0000
> @@ -13,8 +13,6 @@
> along with this program; if not, write to the Free Software
> Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
>
> -#line 18 "decimal.c"
> -
> /*
> =======================================================================
> NOTE: this library implements SQL standard "exact numeric" type
>
> === modified file 'unittest/gunit/CMakeLists.txt'
> --- a/unittest/gunit/CMakeLists.txt 2010-12-17 09:41:21 +0000
> +++ b/unittest/gunit/CMakeLists.txt 2011-01-13 11:37:42 +0000
> @@ -212,6 +212,7 @@ SET(TESTS
> dbug
> mdl
> mdl_mytap
> + my_decimal
> my_regex
> sql_list
> thread_utils
>
> === added file 'unittest/gunit/my_decimal-t.cc'
> --- a/unittest/gunit/my_decimal-t.cc 1970-01-01 00:00:00 +0000
> +++ b/unittest/gunit/my_decimal-t.cc 2011-01-13 11:37:42 +0000
> @@ -0,0 +1,51 @@
> +/* Copyright (c) 2011, 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
> +
> +#include "my_config.h"
> +#include<gtest/gtest.h>
> +
> +#include<my_decimal.h>
> +
> +namespace {
> +
> +class DecimalTest : public ::testing::Test
> +{
> +protected:
> + my_decimal d1;
> + my_decimal d2;
> +};
> +
> +TEST_F(DecimalTest, CopyAndCompare)
> +{
> + const ulonglong val= 42;
> + EXPECT_EQ(0, ulonglong2decimal(val,&d1));
> +
> + d2= d1; // operator=()
> + my_decimal d3(d1); // Copy constructor.
> +
> + EXPECT_EQ(0, my_decimal_cmp(&d1,&d2));
> + EXPECT_EQ(0, my_decimal_cmp(&d2,&d3));
> + EXPECT_EQ(0, my_decimal_cmp(&d3,&d1));
> +
> + ulonglong val1, val2, val3;
> + EXPECT_EQ(0, decimal2ulonglong(&d1,&val1));
> + EXPECT_EQ(0, decimal2ulonglong(&d2,&val2));
> + EXPECT_EQ(0, decimal2ulonglong(&d3,&val3));
> + EXPECT_EQ(val, val1);
> + EXPECT_EQ(val, val2);
> + EXPECT_EQ(val, val3);
> +}
> +
> +}
>
>
>
>
>
>
