List:Commits« Previous MessageNext Message »
From:Gleb Shchepa Date:May 26 2011 10:31pm
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3102) Bug#11765562
View as plain text  
Hello Tor,

Your fix looks good.
The only issue: do you really need to combine refactoring and bugfixing?
All these code beautifications do nothing with the reported problem.
IMO the minimal patch is better, but it's up to you.

Minor issue: please adjust operators in the added/modified code (see below).

Thank you,
Gleb.

On 05/19/2011 03:48 PM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/trunk-bug11765562/ based on
> revid:mayank.prasad@stripped
>
>   3102 Tor Didriksen	2011-05-19
>        Bug#11765562 58545 EXPORT_SET() CAN BE USED TO MAKE ENTIRE SERVER COMPLETELY
> UNRESPONSIVE
>
>        The function EXPORT_SET(bits,on,off[,separator[,number_of_bits]])
>        will concatenate (up to) 64*2 -1 strings: on/off and separator.
>        The total length should not be greater than
> thd->variables.max_allowed_packet.
>       @ mysql-test/r/func_str.result
>          New test case.
>       @ mysql-test/t/func_str.test
>          New test case.
>       @ sql/item_strfunc.cc
>          In Item_func_export_set::val_str, verify that the size of the end result is
> within reasonable bounds.
>       @ unittest/gunit/item-t.cc
>          New test cases.
>       @ unittest/gunit/test_utils.h
>          Add accessor for handle_called.
>
>      modified:
>        mysql-test/r/func_str.result
>        mysql-test/t/func_str.test
>        sql/item_strfunc.cc
>        unittest/gunit/item-t.cc
>        unittest/gunit/test_utils.h
> === modified file 'mysql-test/r/func_str.result'
> --- a/mysql-test/r/func_str.result	2011-01-13 08:19:52 +0000
> +++ b/mysql-test/r/func_str.result	2011-05-19 11:48:46 +0000
> @@ -4306,5 +4306,14 @@ FROM_BASE64(TO_BASE64(dt1))
>   2011-01-01 02:03:04
>   DROP TABLE t1;
>   #
> +# Bug#11765562 58545:
> +# EXPORT_SET() CAN BE USED TO MAKE ENTIRE SERVER COMPLETELY UNRESPONSIVE
> +#
> +SELECT CHAR_LENGTH(EXPORT_SET(1,1,1,REPEAT(1,100000000)));
> +CHAR_LENGTH(EXPORT_SET(1,1,1,REPEAT(1,100000000)))
> +NULL
> +Warnings:
> +Warning	1301	Result of repeat() was larger than max_allowed_packet (1048576) -
> truncated
> +#
>   # End of 5.6 tests
>   #
>
> === modified file 'mysql-test/t/func_str.test'
> --- a/mysql-test/t/func_str.test	2011-01-13 08:19:52 +0000
> +++ b/mysql-test/t/func_str.test	2011-05-19 11:48:46 +0000
> @@ -1557,5 +1557,12 @@ SELECT FROM_BASE64(TO_BASE64(dt1)) FROM
>   DROP TABLE t1;
>
>   --echo #
> +--echo # Bug#11765562 58545:
> +--echo # EXPORT_SET() CAN BE USED TO MAKE ENTIRE SERVER COMPLETELY UNRESPONSIVE
> +--echo #
> +
> +SELECT CHAR_LENGTH(EXPORT_SET(1,1,1,REPEAT(1,100000000)));
> +
> +--echo #
>   --echo # End of 5.6 tests
>   --echo #
>
> === modified file 'sql/item_strfunc.cc'
> --- a/sql/item_strfunc.cc	2011-05-06 13:32:53 +0000
> +++ b/sql/item_strfunc.cc	2011-05-19 11:48:46 +0000
> @@ -3437,23 +3437,21 @@ err:
>   String* Item_func_export_set::val_str(String* str)
>   {
>     DBUG_ASSERT(fixed == 1);
> -  ulonglong the_set = (ulonglong) args[0]->val_int();
> -  String yes_buf, *yes;
> -  yes = args[1]->val_str(&yes_buf);
> -  String no_buf, *no;
> -  no = args[2]->val_str(&no_buf);
> -  String *sep = NULL, sep_buf ;
> +  String yes_buf, no_buf, sep_buf;
> +  const ulonglong the_set = (ulonglong) args[0]->val_int();
> +  const String *yes= args[1]->val_str(&yes_buf);
> +  const String *no= args[2]->val_str(&no_buf);
> +  const String *sep= NULL;
>
>     uint num_set_values = 64;
> -  ulonglong mask = 0x1;
>     str->length(0);
>     str->set_charset(collation.collation);
>
>     /* Check if some argument is a NULL value */
>     if (args[0]->null_value || args[1]->null_value || args[2]->null_value)
>     {
> -    null_value=1;
> -    return 0;
> +    null_value= true;
> +    return NULL;
>     }
>     /*
>       Arg count can only be 3, 4 or 5 here. This is guaranteed from the
> @@ -3466,37 +3464,62 @@ String* Item_func_export_set::val_str(St
>         num_set_values=64;
>       if (args[4]->null_value)
>       {
> -      null_value=1;
> -      return 0;
> +      null_value= true;
> +      return NULL;
>       }
>       /* Fall through */
>     case 4:
>       if (!(sep = args[3]->val_str(&sep_buf)))	// Only true if NULL
>       {
> -      null_value=1;
> -      return 0;
> +      null_value= true;
> +      return NULL;
>       }
>       break;
>     case 3:
>       {
>         /* errors is not checked - assume "," can always be converted */
>         uint errors;
> -      sep_buf.copy(STRING_WITH_LEN(","),&my_charset_bin,
> collation.collation,&errors);
> +      sep_buf.copy(STRING_WITH_LEN(","),&my_charset_bin,
> +                   collation.collation,&errors);
>         sep =&sep_buf;
>       }
>       break;
>     default:
>       DBUG_ASSERT(0); // cannot happen
>     }
> -  null_value=0;
> +  null_value= false;
> +
> +  ulonglong total_length= 0;
> +  ulonglong mask;
> +  uint ix;
> +  for (ix= 0, mask= 0x1; ix<  num_set_values; ++ix, mask= (mask<<  1))
                                                                   ^ please insert space
> +  {
> +    if (the_set&  mask)
                   ^ please insert space
> +      total_length+= yes->length();
> +    else
> +      total_length+= no->length();
> +    if (ix != num_set_values - 1)
> +      total_length+= sep->length();
> +  }
> +
> +  if (total_length>  current_thd->variables.max_allowed_packet)
                      ^ please insert space
> +  {
> +    push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                        ER_WARN_ALLOWED_PACKET_OVERFLOWED,
> +                        ER(ER_WARN_ALLOWED_PACKET_OVERFLOWED),
> +                        func_name(), current_thd->variables.max_allowed_packet);
> +    null_value= true;
> +    return NULL;
> +  }
>
> -  for (uint i = 0; i<  num_set_values; i++, mask = (mask<<  1))
> +  str->reserve(static_cast<uint32>(total_length));
> +  for (ix= 0, mask=0x1; ix<  num_set_values; ++ix, mask = (mask<<  1))
                       ^      ^  please insert space       ^ please remove space
>     {
>       if (the_set&  mask)
>         str->append(*yes);
>       else
>         str->append(*no);
> -    if (i != num_set_values - 1)
> +    if (ix != num_set_values - 1)
>         str->append(*sep);
>     }
>     return str;
>
> === modified file 'unittest/gunit/item-t.cc'
> --- a/unittest/gunit/item-t.cc	2011-05-13 09:36:13 +0000
> +++ b/unittest/gunit/item-t.cc	2011-05-19 11:48:46 +0000
> @@ -176,6 +176,88 @@ TEST_F(ItemTest, ItemFuncDesDecrypt)
>   }
>
>
> +TEST_F(ItemTest, ItemFuncExportSet)
> +{
> +  String str;
> +  Item *on_string= new Item_string(STRING_WITH_LEN("on"),&my_charset_bin);
> +  Item *off_string= new Item_string(STRING_WITH_LEN("off"),&my_charset_bin);
> +  Item *sep_string= new Item_string(STRING_WITH_LEN(","),&my_charset_bin);
> +  {
> +    // Testing basic functionality.
> +    Item_func_export_set *export_set=
> +      new Item_func_export_set(new Item_int(2),
> +                               on_string,
> +                               off_string,
> +                               sep_string,
> +                               new Item_int(4));
> +    EXPECT_FALSE(export_set->fix_fields(thd(), NULL));
> +    EXPECT_EQ(&str, export_set->val_str(&str));
> +    EXPECT_STREQ("off,on,off,off", str.c_ptr_safe());
> +  }
> +  {
> +    // Testing corner case: number_of_bits == zero.
> +    Item_func_export_set *export_set=
> +      new Item_func_export_set(new Item_int(2),
> +                               on_string,
> +                               off_string,
> +                               sep_string,
> +                               new Item_int(0));
> +    EXPECT_FALSE(export_set->fix_fields(thd(), NULL));
> +    EXPECT_EQ(&str, export_set->val_str(&str));
> +    EXPECT_STREQ("", str.c_ptr_safe());
> +  }
> +
> +  /*
> +    Bug#11765562 58545:
> +    EXPORT_SET() CAN BE USED TO MAKE ENTIRE SERVER COMPLETELY UNRESPONSIVE
> +   */
> +  const ulong max_size= 1024;
> +  const ulonglong repeat= max_size / 2;
> +  Item *item_int_repeat= new Item_int(repeat);
> +  Item *string_x= new Item_string(STRING_WITH_LEN("x"),&my_charset_bin);
> +  thd()->variables.max_allowed_packet= max_size;
> +  {
> +    // Testing overflow caused by 'on-string'.
> +    Mock_error_handler error_handler(thd(), ER_WARN_ALLOWED_PACKET_OVERFLOWED);
> +    Item_func_export_set *export_set=
> +      new Item_func_export_set(new Item_int(0xff),
> +                               new Item_func_repeat(string_x, item_int_repeat),
> +                               string_x,
> +                               sep_string);
> +    EXPECT_FALSE(export_set->fix_fields(thd(), NULL));
> +    EXPECT_EQ(NULL, export_set->val_str(&str));
> +    EXPECT_STREQ("", str.c_ptr_safe());
> +    EXPECT_EQ(1, error_handler.handle_called());
> +  }
> +  {
> +    // Testing overflow caused by 'off-string'.
> +    Mock_error_handler error_handler(thd(), ER_WARN_ALLOWED_PACKET_OVERFLOWED);
> +    Item_func_export_set *export_set=
> +      new Item_func_export_set(new Item_int(0xff),
> +                               string_x,
> +                               new Item_func_repeat(string_x, item_int_repeat),
> +                               sep_string);
> +    EXPECT_FALSE(export_set->fix_fields(thd(), NULL));
> +    EXPECT_EQ(NULL, export_set->val_str(&str));
> +    EXPECT_STREQ("", str.c_ptr_safe());
> +    EXPECT_EQ(1, error_handler.handle_called());
> +  }
> +  {
> +    // Testing overflow caused by 'separator-string'.
> +    Mock_error_handler error_handler(thd(), ER_WARN_ALLOWED_PACKET_OVERFLOWED);
> +    Item_func_export_set *export_set=
> +      new Item_func_export_set(new Item_int(0xff),
> +                               string_x,
> +                               string_x,
> +                               new Item_func_repeat(string_x, item_int_repeat));
> +    EXPECT_FALSE(export_set->fix_fields(thd(), NULL));
> +    EXPECT_EQ(NULL, export_set->val_str(&str));
> +    EXPECT_STREQ("", str.c_ptr_safe());
> +    EXPECT_EQ(1, error_handler.handle_called());
> +  }
> +}
> +
> +
>   TEST_F(ItemTest, ItemFuncIntDivOverflow)
>   {
>     const char dividend_str[]=
>
> === modified file 'unittest/gunit/test_utils.h'
> --- a/unittest/gunit/test_utils.h	2011-05-13 09:36:13 +0000
> +++ b/unittest/gunit/test_utils.h	2011-05-19 11:48:46 +0000
> @@ -63,6 +63,8 @@ public:
>                                   MYSQL_ERROR::enum_warning_level level,
>                                   const char* msg,
>                                   MYSQL_ERROR ** cond_hdl);
> +
> +  int handle_called() const { return m_handle_called; }
>   private:
>     THD *m_thd;
>     uint m_expected_error;
>
>
>
>
>

Thread
bzr commit into mysql-trunk branch (tor.didriksen:3102) Bug#11765562Tor Didriksen19 May
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3102) Bug#11765562Gleb Shchepa27 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3102) Bug#11765562Gleb Shchepa27 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3102) Bug#11765562Tor Didriksen27 May