List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:February 23 2011 1:39pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3537)
Bug#56976
View as plain text  
Hi Dmitry,

On 2/14/11 10:33 AM, Dmitry Shulga wrote:
> #At file:///Users/shulga/projects/mysql/5.1-bugteam-bug56976/ based on
> revid:dao-gang.qu@stripped
>
>   3537 Dmitry Shulga	2011-02-14
>        Fixed the bug#56976 - Severe Denial Of Service in prepared statements.
>
>        The problem was that server didn't check resulting size of prepared
>        statement argument which was set using mysql_send_long_data() API.
>        By calling mysql_send_long_data() several times it was possible
>        to create overly big string and thus force server to allocate
>        memory for it. There was no way to limit this allocation.
>
>        The solution is to add check for size of result string against
>        value of max_long_data_size start-up parameter. When intermediate string
>        exceeds max_long_data_size value an appropriate error message is emitted.
>
>        For this purpose can be used max_allowed_packet parameter but since its
>        value limited by 1GB we use new parameter max_long_data_size.
>        max_long_data_size parameter is marked as deprecated and has default
>        value that equials to value of max_allowed_packet start-up parameter.

equials -> equals

>        In the future start-up parameter max_long_data_size will be
>        replaced by max_allowed_packet.
>       @ mysql-test/t/variables.test
>          Added checking for new start-up parameter max_long_data_size.
>       @ sql/item.cc
>          Added call to my_message() when accumulated string exceeds
>          max_long_data_size value. my_message() calls error handler
>          that was installed in mysql_stmt_get_longdata before call
>          to Item_param::set_longdata.
>
>          The error handler sets to corresponding value the next
>          current statement fileds: state, last_error, last_errno.
>       @ sql/mysql_priv.h
>          Added max_long_data_size variable declaration.
>       @ sql/mysqld.cc
>          Added support for start-up parameter 'max_long_data_size'.
>          This parameter limits size of data which can be sent from
>          client to server using mysql_send_long_data() API.
>       @ sql/set_var.cc
>          Added variable 'max_long_data_size' into list of variables
>          displayed by command 'show variables'.
>       @ sql/sql_prepare.cc
>          Added error handler class Set_longdata_error_handler.
>          This handler is used to catch any errors that can be
>          generated during execution of Item_param::set_longdata().
>
>          Sourcecode snippet that makes checking for statement's state

Sourcecode -> source code

>          during statement execution moved from Prepared_statement::execute()
>          to Prepared_statement::execute_loop() in order not to call
>          set_parameters() when statement has failed during
>          set_long_data() execution. If it didn't do then call to
>          set_parameters would failed.
>       @ tests/mysql_client_test.c
>          A testcase for the bug #56976 was added.
>
>      modified:
>        mysql-test/r/variables.result
>        mysql-test/t/variables.test
>        sql/item.cc
>        sql/mysql_priv.h
>        sql/mysqld.cc
>        sql/set_var.cc
>        sql/sql_prepare.cc
>        tests/mysql_client_test.c
> === modified file 'mysql-test/r/variables.result'
> --- a/mysql-test/r/variables.result	2010-11-25 03:11:05 +0000
> +++ b/mysql-test/r/variables.result	2011-02-14 12:33:37 +0000
> @@ -1540,6 +1540,9 @@ ERROR HY000: Cannot drop default keycach
>   SET @@global.key_cache_block_size=0;
>   Warnings:
>   Warning	1292	Truncated incorrect key_cache_block_size value: '0'
> +select @@max_long_data_size;
> +@@max_long_data_size
> +1048576
>   SET @@global.max_binlog_cache_size=DEFAULT;
>   SET @@global.max_join_size=DEFAULT;
>   SET @@global.key_buffer_size=@kbs;
>
> === modified file 'mysql-test/t/variables.test'
> --- a/mysql-test/t/variables.test	2010-11-25 03:11:05 +0000
> +++ b/mysql-test/t/variables.test	2011-02-14 12:33:37 +0000
> @@ -1293,6 +1293,11 @@ SET @@global.max_join_size=0;
>   SET @@global.key_buffer_size=0;
>   SET @@global.key_cache_block_size=0;
>
> +#
> +# Bug#56976: added new start-up paramaeter

paramaeter -> parameter.

> +#
> +select @@max_long_data_size;
> +
>   # cleanup
>   SET @@global.max_binlog_cache_size=DEFAULT;
>   SET @@global.max_join_size=DEFAULT;
>

[...]

> @@ -8831,6 +8847,14 @@ static int get_options(int *argc,char **
>     else
>       pool_of_threads_scheduler(&thread_scheduler);  /* purecov: tested */
>   #endif
> +
> +  /*
> +    If max_long_data_size is not specified explicitly use
> +    value of max_allowed_packet.
> +  */
> +  if (!max_long_data_size_used)
> +    max_long_data_size= global_system_variables.max_allowed_packet;
> +
>     return 0;
>   }
>
>
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc	2010-12-28 23:47:05 +0000
> +++ b/sql/set_var.cc	2011-02-14 12:33:37 +0000
> @@ -394,6 +394,12 @@ static sys_var_thd_ulong	sys_max_seeks_f
>   					&SV::max_seeks_for_key);
>   static sys_var_thd_ulong   sys_max_length_for_sort_data(&vars,
> "max_length_for_sort_data",
>                                                   
> &SV::max_length_for_sort_data);
> +static sys_var_const    sys_max_long_data_size(&vars,
> +                                               "max_long_data_size",
> +                                               OPT_GLOBAL, SHOW_LONG,
> +                                               (uchar*)
> +&max_long_data_size);

Please highlight in the changeset comment that the variable can only be 
set at server start.

>   #ifndef TO_BE_DELETED	/* Alias for max_join_size */
>   static sys_var_thd_ha_rows	sys_sql_max_join_size(&vars, "sql_max_join_size",
>   					&SV::max_join_size,
>
> === modified file 'sql/sql_prepare.cc'
> --- a/sql/sql_prepare.cc	2010-12-28 23:47:05 +0000
> +++ b/sql/sql_prepare.cc	2011-02-14 12:33:37 +0000
> @@ -2730,6 +2730,32 @@ void mysql_sql_stmt_close(THD *thd)
>     }
>   }
>
> +
> +class Set_longdata_error_handler : public Internal_error_handler
> +{
> +public:
> +  Set_longdata_error_handler(Prepared_statement *statement)
> +    :stmt(statement)

Add space after the colon.

> +  { }
> +
> +public:
> +  bool handle_error(uint sql_errno,
> +                    const char *message,
> +                    MYSQL_ERROR::enum_warning_level level,
> +                    THD *thd)

Do not name the parameters that are not used. Example:

bool handle_error(uint sql_errno,
                   const char *message,
                   MYSQL_ERROR::enum_warning_level,
                   THD *)

> +  {
> +    stmt->state= Query_arena::ERROR;
> +    stmt->last_errno= sql_errno;
> +    strncpy(stmt->last_error, message, MYSQL_ERRMSG_SIZE);
> +
> +    return TRUE;
> +  }
> +
> +private:
> +  Prepared_statement *stmt;
> +};
> +
> +
>   /**
>     Handle long data in pieces from client.
>
> @@ -2786,16 +2812,19 @@ void mysql_stmt_get_longdata(THD *thd, c
>
>     param= stmt->param_array[param_number];
>
> +  Set_longdata_error_handler err_handler(stmt);
> +  /*
> +    Install handler that will catch any errors that can be generated
> +    during execution of Item_param::set_longdata() and propagate
> +    them to Statement::last_error.
> +  */
> +  thd->push_internal_handler(&err_handler);
>   #ifndef EMBEDDED_LIBRARY
> -  if (param->set_longdata(packet, (ulong) (packet_end - packet)))
> +  param->set_longdata(packet, (ulong) (packet_end - packet));
>   #else
> -  if (param->set_longdata(thd->extra_data, thd->extra_length))
> +  param->set_longdata(thd->extra_data, thd->extra_length);
>   #endif
> -  {
> -    stmt->state= Query_arena::ERROR;
> -    stmt->last_errno= ER_OUTOFMEMORY;
> -    sprintf(stmt->last_error, ER(ER_OUTOFMEMORY), 0);
> -  }
> +  thd->pop_internal_handler();
>
>     general_log_print(thd, thd->command, NullS);
>
> @@ -3257,6 +3286,13 @@ Prepared_statement::execute_loop(String
>     bool error;
>     int reprepare_attempt= 0;
>
> +  /* Check if we got an error when sending long data */
> +  if (state == Query_arena::ERROR)
> +  {
> +    my_message(last_errno, last_error, MYF(0));
> +    return TRUE;
> +  }
> +
>     if (set_parameters(expanded_query, packet, packet_end))
>       return TRUE;
>
> @@ -3497,12 +3533,6 @@ bool Prepared_statement::execute(String
>
>     status_var_increment(thd->status_var.com_stmt_execute);
>
> -  /* Check if we got an error when sending long data */
> -  if (state == Query_arena::ERROR)
> -  {
> -    my_message(last_errno, last_error, MYF(0));
> -    return TRUE;
> -  }
>     if (flags&  (uint) IS_IN_USE)
>     {
>       my_error(ER_PS_NO_RECURSION, MYF(0));
>
> === modified file 'tests/mysql_client_test.c'
> --- a/tests/mysql_client_test.c	2010-12-28 23:47:05 +0000
> +++ b/tests/mysql_client_test.c	2011-02-14 12:33:37 +0000
> @@ -18399,6 +18399,54 @@ static void test_bug47485()
>
>
>   /*
> +  Bug #56976:   Severe Denial Of Service in prepared statements
> +*/
> +
> +static void test_bug56976()
> +{
> +  MYSQL_STMT   *stmt;
> +  MYSQL_BIND    bind[1];
> +  int           rc;
> +  const char*   query = "SELECT LENGTH(?)";
> +  char *long_buffer;
> +  unsigned long i, packet_len = 256 * 1024L;
> +  unsigned long dos_len    = 2 * 1024 * 1024L;
> +
> +  DBUG_ENTER("test_bug56976");
> +  myheader("test_bug56976");
> +
> +  stmt= mysql_stmt_init(mysql);
> +  check_stmt(stmt);
> +
> +  rc= mysql_stmt_prepare(stmt, query, strlen(query));
> +  check_execute(stmt, rc);
> +
> +  memset(bind, 0, sizeof(bind));
> +  bind[0].buffer_type = MYSQL_TYPE_TINY_BLOB;
> +
> +  rc= mysql_stmt_bind_param(stmt, bind);
> +  check_execute(stmt, rc);
> +
> +  long_buffer= malloc(packet_len);

Use my_malloc. Check for failure to allocate. Free it.

> +  memset(long_buffer, 'a', packet_len);
> +

[...]

Regards,

Davi
Thread
bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3537) Bug#56976Dmitry Shulga14 Feb
  • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3537)Bug#56976Davi Arnaut23 Feb