List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:January 13 2011 2:45pm
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241
View as plain text  
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);
> +}
> +
> +}
>
>    
>
>
>
>    


Thread
bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241Tor Didriksen13 Jan
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241Jorgen Loland13 Jan
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241Tor Didriksen13 Jan
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241Olav Sandstaa13 Jan
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3498) Bug#59241Tor Didriksen13 Jan