Hi Sergei, On 10/07/2017 04:52 PM, Sergei Golubchik wrote:

Hi, Alexander!

See comments below.

TL;DR - dunno. Changes in behavior. Lots of code for overflow checks that is unused or duplicated. And only to avoid side effects with invalid arguments for SEC_TO_TIME() ?

Perhaps, just check for val_str() returing NULL here and have the side effects fixed in 10.4, when Items will return a Value object?

Thanks for the review! I agree, the true fix removing the side effect appeared to be too big for 5.5. Let's do it in 10.4 (or maybe 10.3). I'm attaching a smaller version, which only handles the NULL result from val_str(). Also, I found a new small problem with tricky charsets such as ucs2 and fixed it by adding ErrConvString. Please see attached. Still, I'd like to response your comments, please see below:

On Oct 05, Alexander Barkov wrote:

...

Hello Sergei,

Please review a patch for MDEV-13972.

The patch is for 5.5, as we agreed in slack.

Thanks!

...

commit 003c19c7677e1ce5b18b4081e21b7864700f8250 Author: Alexander Barkov Date: Thu Oct 5 21:07:19 2017 +0400

MDEV-13972 crash in Item_func_sec_to_time::get_date

Item_func_sec_to_time::get_date() called a value method two times: - once val_int() or val_decimal() inside Item::get_seconds() - then val_str() to generate a warning, in case an out-of-range value

This approach was wrong for two reasons: 1. val_str() in some cases can return NULL even if val_int()/val_decimal() previously returned a not-NULL value. This caused the crash reported in the bug: the warning generation code in Item_func_sec_to_time::get_date() did not expect val_str() to return NULL. 2. Calling val_str() to generate warnings fires the function side effect again. For example, consider SEC_TO_TIME(f1()), where f1() is a stored function doing some INSERTs as a side effect. If f1() returns an out-of-range value, the call for val_str() to generate the warning forces the INSERTs to happen again, which is wrong. The side effect must happen only once per call.

Fixing the code not to do a dedicated call for val_str() for warnings. To preserve the old warnings recorded in *.result files, four methods where added: - get_seconds_from_string() - get_seconds_from_int() - get_seconds_from_decimal() - get_seconds_from_real()

A new structure Secondf_st was added: - to avoid having too many arguments in get_seconds_from_xxx() - to share common code, e.g. between double_to_datetime_with_warn() and get_seconds_from_real() - to share the code easier in the future - to have the code in item.cc and item_timefunc.cc smaller

As a good side effect, a redundant warning in func_time.result disappeared

Good comment!

...

diff --git a/mysql-test/r/func_time.result b/mysql-test/r/func_time.result index 68b1e0f..b46e7f3 100644 --- a/mysql-test/r/func_time.result +++ b/mysql-test/r/func_time.result @@ -2626,3 +2625,126 @@ DROP TABLE t1; SELECT 1 MOD ADDTIME( '13:58:57', '00:00:01' ) + 2; 1 MOD ADDTIME( '13:58:57', '00:00:01' ) + 2 3 +# +# MDEV-13972 crash in Item_func_sec_to_time::get_date +# +DO TO_DAYS(SEC_TO_TIME(TIME(CEILING(UUID())))) /*sporadic warnings*/;

sporadic warnings in the test file?

No, the warnings are suppressed in *.test file. By this comment I tried to say *why* they are suppressed. I agree that the *.result file looked confusing. I removed the /*sporadic warnings*/ part from the query and added a comment before the query instead, like this: +# The below query can return warning sporadically +--disable_warnings +DO TO_DAYS(SEC_TO_TIME(TIME(CEILING(UUID())))); +--enable_warnings

...

+DO TO_DAYS(SEC_TO_TIME(MAKEDATE('',RAND(~(''))))); ... diff --git a/sql/item.cc b/sql/item.cc index fdfbba3..73a1acd 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -1407,22 +1407,82 @@ bool Item::get_date(MYSQL_TIME *ltime,ulonglong fuzzydate) return null_value|= !(fuzzydate & TIME_FUZZY_DATES); }

-bool Item::get_seconds(ulonglong *sec, ulong *sec_part) + +bool Item::get_seconds(Secondf_st *sec, const char *type, ulonglong maxsec) { - if (decimals == 0) - { // optimize for an important special case - longlong val= val_int(); - bool neg= val < 0 && !unsigned_flag; - *sec= neg ? -val : val; - *sec_part= 0; - return neg; + switch (cmp_type()) { + case STRING_RESULT: + return get_seconds_from_string(sec, type, maxsec); + case TIME_RESULT: + case DECIMAL_RESULT: + if (decimals == 0) // optimize for an important special case + return get_seconds_from_int(sec, type, maxsec); + return get_seconds_from_decimal(sec, type, maxsec); + case INT_RESULT: + return get_seconds_from_int(sec, type, maxsec); + case REAL_RESULT: + return get_seconds_from_real(sec, type, maxsec); + case ROW_RESULT: + case IMPOSSIBLE_RESULT: + break; }

I'm not sure that's the same as before. First, you completely ignore decimals when converting from double.

I think I am not. get_seconds_from_real() uses this method: void Secondf_st::set(double value) { if ((m_neg= value < 0)) value= -value; if (value > LONGLONG_MAX) value= static_cast<double>(LONGLONG_MAX); m_second= static_cast<ulonglong>(floor(value)); m_usecond= static_cast<ulong>((value -floor(value))*TIME_SECOND_PART_FACTOR); }

Second, old code was doing item->val_decimal(), while you're doing, for example, item->val_str() + str2my_decimal(). Which is not always the same.

Item::val_decimal_from_string() uses exactly the same approach. All Items with STRING_RESUL should go through this chain: val_str + str2my_decimal.

Some items, e.g. BIT fields, need to know in what context they're evaluated.

As for the BIT data type, it reports result_type() and cmp_type() as INT_RESULT, and decimals for an BIT type Item should be 0. So both before the patch and after the patch, BIT went through the val_int() branch. There should not be behavior change for BIT. I could not invent a bad example, but some unexpected anomalies are probably still possible, e.g. for inconsistently coded Items. Anyway, to be on the safe side, let's keep 5.5 with as small change as possible.

...

+ DBUG_ASSERT(0); + sec->init(); + return 0; +} + + +bool Item::get_seconds_from_int(Secondf_st *sec, + const char *type, ulonglong maxsec) +{ + longlong val= val_int(); + sec->set(val, unsigned_flag); + sec->check_range_with_warn(type, maxsec, ErrConvInteger(val, unsigned_flag)); + return null_value; +} + + +bool Item::get_seconds_from_real(Secondf_st *sec, + const char *type, ulonglong maxsec) +{ + double val= val_real(); + sec->set(val); + sec->check_range_with_warn(type, maxsec, ErrConvDouble(val)); + return null_value; +} + + +bool Item::get_seconds_from_decimal(Secondf_st *sec, + const char *type, ulonglong maxsec) +{ my_decimal tmp, *dec= val_decimal(&tmp); if (!dec) - return 0; - return my_decimal2seconds(dec, sec, sec_part); + { + sec->init(); + return true; + } + sec->set(dec); + sec->check_range_with_warn(type, maxsec, ErrConvDecimal(dec)); + return false; }

+ +bool Item::get_seconds_from_string(Secondf_st *sec, + const char *type, ulonglong maxsec) +{ + char buff[64]; + String tmp(buff, sizeof(buff), &my_charset_bin), *res= val_str(&tmp); + if (!res) + { + sec->init(); + return true; + } + sec->set(res); + sec->check_range_with_warn(type, maxsec, ErrConvString(res)); + return false; +} + + CHARSET_INFO *Item::default_charset() { return current_thd->variables.collation_connection; diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc index 0ed1506..72c3e95 100644 --- a/sql/item_timefunc.cc +++ b/sql/item_timefunc.cc @@ -1700,42 +1700,33 @@ bool Item_func_sysdate_local::get_date(MYSQL_TIME *res, bool Item_func_sec_to_time::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) { DBUG_ASSERT(fixed == 1); - bool sign; - ulonglong sec; - ulong sec_part; + Secondf_st sec;

bzero((char *)ltime, sizeof(*ltime)); ltime->time_type= MYSQL_TIMESTAMP_TIME;

- sign= args[0]->get_seconds(&sec, &sec_part); - - if ((null_value= args[0]->null_value)) - return 1; + if ((null_value= (args[0]->get_seconds(&sec, + "time", TIME_MAX_VALUE_SECONDS))))

strange parentheses around get_seconds() call.

...

+ return true;

- ltime->neg= sign; - if (sec > TIME_MAX_VALUE_SECONDS) + ltime->neg= sec.m_neg; + if (sec.m_second > TIME_MAX_VALUE_SECONDS) goto overflow;

kinda silly. get_seconds() checks for the overflow and you need to check it here again.

Notice, the maxsec value passed to gets_seconds() is not always the same with what the caller later checks again. Only Item_func_sec_to_time::get_date() passes maxsec=TIME_MAX_VALUE_SECONDS, while the other two calls pass LONGLONG_MAX. This way I wanted to preserve the behavior in 5.5 a much as possible. In 10.3 we can fix it in a better way: - Item_func_from_unixtime::get_date() should issue a warning when "seconds > TIMESTAMP_MAX_VALUE". Currently it issues warnings only when "seconds >= LONGLONG_MAX". For the seconds range TIMESTAMP_MAX_VALUE..LONGLONG_MAX it returns NULL silently, which is bad. - Item_func_maketime::get_date() should issue a warning when "seconds > 59". Currently it issues warnings only when "seconds >= LONGLONG_MAX", For the seconds range 59..LONGLONG_MAX it returns NULL silently, which is bad again.

...

- DBUG_ASSERT(sec_part <= TIME_MAX_SECOND_PART); + DBUG_ASSERT(sec.m_usecond <= TIME_MAX_SECOND_PART);

- ltime->hour= (uint) (sec/3600); - ltime->minute= (uint) (sec % 3600) /60; - ltime->second= (uint) sec % 60; - ltime->second_part= sec_part; + ltime->hour= (uint) (sec.m_second / 3600); + ltime->minute= (uint) (sec.m_second % 3600) /60; + ltime->second= (uint) sec.m_second % 60; + ltime->second_part= sec.m_usecond;

return 0;

overflow: /* use check_time_range() to set ltime to the max value depending on dec */ int unused; - char buf[100]; - String tmp(buf, sizeof(buf), &my_charset_bin), *err= args[0]->val_str(&tmp); - ltime->hour= TIME_MAX_HOUR+1; check_time_range(ltime, decimals, &unused); - make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, - err->ptr(), err->length(), - MYSQL_TIMESTAMP_TIME, NullS); return 0; }

@@ -1929,21 +1920,18 @@ void Item_func_from_unixtime::fix_length_and_dec() bool Item_func_from_unixtime::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date __attribute__((unused))) { - bool sign; - ulonglong sec; - ulong sec_part; + Secondf_st sec;

bzero((char *)ltime, sizeof(*ltime)); ltime->time_type= MYSQL_TIMESTAMP_TIME;

- sign= args[0]->get_seconds(&sec, &sec_part); - - if (args[0]->null_value || sign || sec > TIMESTAMP_MAX_VALUE) + if (args[0]->get_seconds(&sec, "", ULONGLONG_MAX) || + sec.m_neg || sec.m_second > TIMESTAMP_MAX_VALUE) return (null_value= 1);

and again you need to check for the overflow outside of get_seconds(). because this one doesn't need a warning.

...

- tz->gmt_sec_to_TIME(ltime, (my_time_t)sec); + tz->gmt_sec_to_TIME(ltime, (my_time_t) sec.m_second);

- ltime->second_part= sec_part; + ltime->second_part= sec.m_usecond;

return (null_value= 0); } @@ -2735,12 +2723,11 @@ bool Item_func_maketime::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) bool overflow= 0; longlong hour= args[0]->val_int(); longlong minute= args[1]->val_int(); - ulonglong second; - ulong microsecond; - bool neg= args[2]->get_seconds(&second, µsecond); + Secondf_st sec;

- if (args[0]->null_value || args[1]->null_value || args[2]->null_value || - minute < 0 || minute > 59 || neg || second > 59) + if (args[0]->null_value || args[1]->null_value || + args[2]->get_seconds(&sec, "", ULONGLONG_MAX) || + minute < 0 || minute > 59 || sec.m_neg || sec.m_second > 59)

and again.

...

return (null_value= 1);

bzero(ltime, sizeof(*ltime));

Regards, Sergei Chief Architect MariaDB and security@mariadb.org