Re: bk commit into 5.1 tree (tnurnberg:1.2654) BUG#32757

[Sergei Golubchik]

Hi!

On Jan 07, Tatjana A Nuernberg wrote:
> ChangeSet@stripped, 2008-01-07 02:47:56+01:00, tnurnberg@stripped +5 -0
>   Bug#32757: hang with sql_mode set when setting some global variables
>   
>   If setting a system-variable provided by a plug-in failed, no OK or
>   error was sent in some cases, hanging the client. We now send an error
>   in the case from the ticket (integer-argument out of range in STRICT
>   mode). We also provide a semi-generic fallback message for possible
>   future cases like this where an error is signalled, but no message is
>   sent to the client.
> 
>   mysql-test/r/plugin.result@stripped, 2008-01-07 02:47:54+01:00, tnurnberg@stripped +27
> -0
>     show that on out-of-range values, plugin interface throws errors
>     in STRICT mode and warnings otherwise.
> 
>   mysql-test/t/plugin.test@stripped, 2008-01-07 02:47:54+01:00, tnurnberg@stripped +35
> -0
>     show that on out-of-range values, plugin interface throws errors
>     in STRICT mode and warnings otherwise.
> 
>   sql/sql_parse.cc@stripped, 2008-01-07 02:47:54+01:00, tnurnberg@stripped +12 -0
>     If sql_set_variables() returns with an error but no message
>     was sent to the client, send a semi-generic one so the session
>     won't hang and we won't fail silently.
> 
>   sql/sql_plugin.cc@stripped, 2008-01-07 02:47:54+01:00, tnurnberg@stripped +57 -21
>     For integers provided by plugins:
>     - handle signedness correctly in error-/warning-messages
>     - in STRICT mode: throw an error if value is out of range
>       otherwise: throw a warning if more than just block-size
>                  was corrected (like we used to)
> 
>   storage/example/ha_example.cc@stripped, 2008-01-07 02:47:54+01:00, tnurnberg@stripped
> +14 -0
>     Add a ULONG system variable to example plugin so
>     we can test integers in the plugin-interface without
>     having to depend on the presence of innobase.
> 
> --- a/mysql-test/t/plugin.test	2007-11-14 10:48:17 +01:00
> +++ b/mysql-test/t/plugin.test	2008-01-07 02:47:54 +01:00
> @@ -39,3 +39,38 @@ SET GLOBAL example_enum_var= e2;
>  SET GLOBAL example_enum_var= impossible;
>  
>  UNINSTALL PLUGIN example;
> +
> +
> +
> +#
> +# Bug #32757 hang with sql_mode set when setting some global variables
> +#
> +INSTALL PLUGIN example SONAME 'ha_example.so';
> +
> +select @@session.sql_mode into @old_sql_mode;
> +
> +# first, try normal sql_mode (no error, send OK)
> +set session sql_mode='';
> +set global example_ulong_var=500;
> +select @@global.example_ulong_var;
> +# overflow -- throw warning, do NOT change value

wrong comment, should be "throw warning, auto-correct the value"

> +set global example_ulong_var=1111;
> +select @@global.example_ulong_var;
>
> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc	2007-12-13 15:26:24 +01:00
> +++ b/sql/sql_parse.cc	2008-01-07 02:47:54 +01:00
> @@ -3127,6 +3127,18 @@ end_with_restore_list:
>        thd->one_shot_set|= lex->one_shot_set;
>        send_ok(thd);
>      }
> +    else
> +    {
> +      /*
> +        We encountered some sort of error, but no message was sent.
> +        Send something semi-generic here since we don't know which
> +        assignment in the list caused the error.
> +      */
> +      if (!thd->is_error())
> +        my_error(ER_WRONG_ARGUMENTS,MYF(0),"SET");

Can you create a test case that shows this error ?

> +      goto error;
> +    }
> +
>      break;
>    }
>  
> diff -Nrup a/sql/sql_plugin.cc b/sql/sql_plugin.cc
> --- a/sql/sql_plugin.cc	2007-12-13 12:49:55 +01:00
> +++ b/sql/sql_plugin.cc	2008-01-07 02:47:54 +01:00
> @@ -1916,16 +1928,28 @@ static int check_func_long(THD *thd, str
>    else
>      *(long *)save= (long) getopt_ll_limit_value(tmp, &options, &fixed);
>  
> -  if (fixed)
> +  if (*(long *)save != (long) tmp)

why ?
in set_var.cc you use didn't change 'fixed' to 'save != tmp'.

>    {
>      char buf[22];
> +    if (var->flags & PLUGIN_VAR_UNSIGNED)
> +      ullstr(tmp, buf);
> +    else
> +      llstr(tmp, buf);
> +
> +    if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES)
> +    {
> +      my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0),
> +               var->name, buf);
> +      return TRUE;
> +    }
> +    else if (fixed)
>        push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
>                            ER_TRUNCATED_WRONG_VALUE,
>                            ER(ER_TRUNCATED_WRONG_VALUE), var->name,
>                            ullstr(tmp, buf));
>    }
> -  return (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES) &&
> -         (*(long *)save != (long) tmp);

You didn't fix set_var.cc and server built-in variables still
auto-correct invalid values even in strict mode.

Wouldn't you avoid code duplication if you add thsi check to
throw_bounds_warning() ? (and use it in sql_plugin.cc, of course)

> +  return FALSE;
>  }

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140