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? 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 <bar@mariadb.org> 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?
+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. 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. Some items, e.g. BIT fields, need to know in what context they're evaluated.
+ 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.
- 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