List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:February 2 2011 11:54am
Subject:Re: bzr commit into mysql-5.0 branch (Georgi.Kodinov:2895)
Bug#52315 Bug#55755
View as plain text  
Hello Georgi!

Here are my comments about your patch:

* Georgi Kodinov <Georgi.Kodinov@stripped> [11/01/17 19:51]:
> #At file:///Users/kgeorge/mysql/work/B55755-5.0/ based on
> revid:alexander.nozdrin@stripped
> 
>  2895 Georgi Kodinov	2011-01-17
>       Fixes for Bug #55755 and Bug #52315 part 2
>       
>       Bug #55755 : Date STD variable signness breaks server on FreeBSD and OpenBSD
>       
>       * Added a check to configure on the size of time_t
>       * Created a macro to check for a valid time_t that can fit into a my_time_t
>       * Used the macro consistently instead of the ad-hoc checks introduced by 52315
>       * Fixed compliation warnings on platforms where the size of time_t is smaller
> than
>         the size of a long (e.g. OpenBSD 4.8 64 amd64).
>       
>       Bug #52315: utc_date() crashes when system time > year 2037
>       
>       * Added a correct check for the timestamp range instead of just variable size
> check to
>       SET TIMESTAMP.
>       * Added overflow checking before converting to time_t. 
>       * Using a correct localized error message in this case instead of the generic
> error.
>       * Added a test suite.
> 

...

I think your patch mixes-up two different things:

* Range of theoretically possible values for my_time_t type which is
  [MY_TIME_T_MIN .. MY_TIME_T_MAX].
  This type is our own portable time_t replacement which allows to hold
  datetime values within  1902 ... 2038 range on all platforms.
  We use this type in the signatures/implementation of our own datetime
  functions in tztime.cc and in signatures of wrappers to system functions
  like my_system_gmt_sec().

  There is no guarantee that all values from this range can be safely
  passed to our own or wrappers to system functions (especially to the
  latter!), and/or store in TIMESTAMP columns.

* [TIMESTAMP_MIN_VALUE .. TIMESTAMP_MAX_VALUE] range. Values within
  this range can be safely passed to any datetime function in server
  and can always be stored in TIMESTAMP columns.

In most of checks in server we should be using the latter range and
not the former.

Actually, I think the original patch for bug #52315 made the same mistake
and as result bug #55755 was reported.

Below I suggest some changes to your patch which, in my opinion, allow
to get rid of this mix-up.

> === modified file 'configure.in'
> --- a/configure.in	2010-12-28 18:57:23 +0000
> +++ b/configure.in	2011-01-17 16:43:43 +0000
> @@ -1956,6 +1956,15 @@ dnl
>  
>  MYSQL_CHECK_TIME_T
>  
> +dnl
> +dnl check size of time_t
> +dnl
> +
> +AC_CHECK_SIZEOF(time_t, 8)
> +if test "$ac_cv_sizeof_time_t" -eq 0
> +then
> +  AC_MSG_ERROR("MySQL needs a time_t type.")
> +fi

OK. Although the comment for this check in my opinion is a bit redundant.

>  
>  # do we need #pragma interface/#pragma implementation ?
>  # yes if it's gcc 2.x, and not icc pretending to be gcc, and not cygwin
> 
> === modified file 'include/config-win.h'
> --- a/include/config-win.h	2009-07-31 19:22:02 +0000
> +++ b/include/config-win.h	2011-01-17 16:43:43 +0000
> @@ -167,6 +167,7 @@ typedef uint rf_SetTimer;
>  #define SIZEOF_LONG		4
>  #define SIZEOF_LONG_LONG	8
>  #define SIZEOF_OFF_T		8
> +#define SIZEOF_TIME_T		8
>  #ifdef _WIN64
>  #define SIZEOF_CHARP		8
>  #else

This change is potentially confusing/can raise questions since
sizeof(time_t) on Windows actually depends on compiler version.
AFAIK it was 4 bytes on versions prior to VC++ 8.0/VS2005 (actually
I think it was platform-specific) and became 8 bytes starting from
this version.

I suggest at least to add comment clarifying that sizeof(time_t) is 8
for all versions of VC which we support nowadays.

> 
> === modified file 'include/my_time.h'
> --- a/include/my_time.h	2008-04-03 15:32:00 +0000
> +++ b/include/my_time.h	2011-01-17 16:43:43 +0000
> @@ -44,6 +44,17 @@ typedef long my_time_t;
>  #define MY_TIME_T_MIN LONG_MIN
>  
>  
> +/*
> +  check for valid times only if the range of time_t is greater than
> +  the range of my_time_t
> +*/
> +#if SIZEOF_TIME_T > SIZEOF_LONG
> +# define IS_VALID_TIME_T(x) ((x) <= (time_t) MY_TIME_T_MAX && \
> +                             (x) >= (time_t) MY_TIME_T_MIN)
> +#else
> +# define IS_VALID_TIME_T(x) (1 > 0)
> +#endif
> +

This is one of places where I think we should be using TIMESTAMP_MIN_VALUE
and TIMESTAMP_MAX_VALUE instead. I also think that this macro needs a better
name. So the above code can look similar to:

/**
  Check if time_t value fits in the range which is known to be safe for
  handling in native and our own datetime functions and storing in TIMESTAMP
  columns.

  To avoid warnings from compiler we use simplified check on platforms where
  TIMESTAMP_MAX_VALUE is upper bound for all time_t values (e.g. OpenBSD).
*/

#if (SIZEOF_TIME_T > 4) || defined(TIME_T_UNSIGNED)
#define IS_TIME_T_VALID_FOR_TIMESTAMP(x) ((x) <= TIMESTAMP_MAX_VALUE && \
                                        (x) >= TIMESTAMP_MIN_VALUE)
#else
#define IS_TIME_T_VALID_FOR_TIMESTAMP(x) ((x) >= TIMESTAMP_MIN_VALUE)
#endif

...

> === modified file 'mysql-test/t/func_time.test'
> --- a/mysql-test/t/func_time.test	2008-12-23 14:08:04 +0000
> +++ b/mysql-test/t/func_time.test	2011-01-17 16:43:43 +0000
> @@ -838,4 +838,19 @@ SELECT '2008-02-18' + INTERVAL 1 FRAC_SE
>  --error ER_PARSE_ERROR
>  SELECT '2008-02-18' - INTERVAL 1 FRAC_SECOND;
>  
> +
> +--echo #
> +--echo # Bug #52315 part 2 : utc_date() crashes when system time > year 2037
> +--echo #
> +
> +--disable_result_log
> +--error ER_WRONG_VALUE_FOR_VAR
> +SET TIMESTAMP=-147490000; SELECT UTC_TIMESTAMP();
> +--error ER_WRONG_VALUE_FOR_VAR
> +SET TIMESTAMP=2147483648; SELECT UTC_TIMESTAMP();
> +SET TIMESTAMP=2147483646; SELECT UTC_TIMESTAMP();
> +SET TIMESTAMP=2147483647; SELECT UTC_TIMESTAMP();
> +--enable_result_log

I think that it is also worth to add coverage for values close to 0,
like -1, 0 and 1.

> +
> +
>  --echo End of 5.0 tests
> 
> === modified file 'sql-common/my_time.c'
> --- a/sql-common/my_time.c	2009-09-17 15:10:30 +0000
> +++ b/sql-common/my_time.c	2011-01-17 16:43:43 +0000
> @@ -984,8 +984,16 @@ my_system_gmt_sec(const MYSQL_TIME *t_sr
>      So, tmp < TIMESTAMP_MIN_VALUE will be triggered. On platfroms
>      with unsigned time_t tmp+= shift*86400L might result in a number,
>      larger then TIMESTAMP_MAX_VALUE, so another check will work.
> +
> +    tmp being larger than TIMESTAMP_MAX_VALUE can only happen on
> +    platforms where the size of time_t is larger than the size of
> +    TIMESTAMP_MAX_VALUE (currently INT32 : 4 bytes).
> +    #ifdef to avoid the compilation warnings on these platforms (OpenBSD).
>    */
> -  if ((tmp < TIMESTAMP_MIN_VALUE) || (tmp > TIMESTAMP_MAX_VALUE))
> +  if ((tmp < TIMESTAMP_MIN_VALUE)
> +#if SIZEOF_TIME_T > 4
> +      || (tmp > TIMESTAMP_MAX_VALUE))
> +#endif
>      tmp= 0;

Here we can use:

    if (! IS_TIME_T_VALID_FOR_TIMESTAMP(tmp))
      tmp= 0;

instead.

This also allows to remove the above addition to the comment which is
IMO a bit confusing.

...

> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc	2010-12-28 18:57:23 +0000
> +++ b/sql/set_var.cc	2011-01-17 16:43:43 +0000
> @@ -2714,14 +2714,22 @@ int set_var_collation_client::update(THD
>  
>  bool sys_var_timestamp::check(THD *thd, set_var *var)
>  {
> -  time_t val;
> +  longlong val;
> +  time_t time_val;
>    var->save_result.ulonglong_value= var->value->val_int();
> -  val= (time_t) var->save_result.ulonglong_value;
> -  if (val < (time_t) MY_TIME_T_MIN || val > (time_t) MY_TIME_T_MAX)
> +  val= (longlong) var->save_result.ulonglong_value;
> +  time_val= (time_t) val;
> +  if (val != 0                         // this is how you set the default value
> +      && (val < TIMESTAMP_MIN_VALUE
> +          || val != (longlong) time_val
> +#if SIZEOF_TIME_T > 4
> +          || val > TIMESTAMP_MAX_VALUE
> +#endif
> +         )
> +     )

Taking into account that "val" is of longlong type and the fact that values
within [TIMESTAMP_MIN_VALUE .. TIMESTAMP_MAX_VALUE] range can be safely stored
in time_t variable, the above code can be simplified to something like:

longlong val;
var->save_result.ulonglong_value= var->value->val_int();
val= (longlong) var->save_result.ulonglong_value;
if (val != 0 &&                       // this is how you set the default value
    (val < TIMESTAMP_MIN_VALUE || val > TIMESTAMP_MAX_VALUE ))
{
  ...

>    {
> -    my_message(ER_UNKNOWN_ERROR, 
> -               "This version of MySQL doesn't support dates later than 2038",
> -               MYF(0));
> +    char buf[64];
> +    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "timestamp", llstr(val, buf));
>      return TRUE;
>    }
>    return FALSE;
> 

Otherwise your patch looks OK to me.

Best regards,
-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.0 branch (Georgi.Kodinov:2895) Bug#52315 Bug#55755Georgi Kodinov17 Jan
  • Re: bzr commit into mysql-5.0 branch (Georgi.Kodinov:2895) Bug#52315Bug#55755Joerg Bruehe19 Jan
  • Re: bzr commit into mysql-5.0 branch (Georgi.Kodinov:2895)Bug#52315 Bug#55755Dmitry Lenev2 Feb