maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #08876
Re: [Commits] 62fe87c: MDEV-8663: IF Statement returns multiple values erroneously (or Assertion `!null_value' failed in Item::send(Protocol*, String*))
Hi Sanja,
From my understanding, the patch fixes consequences of the problem,
but not the problem itself. It very likely will still have crashes
with slightly modified SQL scripts.
The source of the problem is that
Item_func_hybrid_result_type::val_decimal() does not return NULL
when int_op() or real_op() set null_value to "true".
By the way, compare Item_func_hybrid_result_type::val_decimal()
with a very similar method Item_func_hybrid_result_type::val_str().
The latter already handle the INT_RESULT and REAL_RESULT cases well
and return NULL properly in case of null_value.
get_date(), val_int(), val_real() also look fine for me.
So the are problems only in val_decimal().
I propose a small patch that fixes the source of the problem.
This is the small patch:
@@ -1038,17 +1026,21 @@ my_decimal *Item_func_hybrid_result_type::val_decimal(my_decimal *decimal_value)
case INT_RESULT:
{
longlong result= int_op();
+ if (null_value)
+ return NULL;
int2my_decimal(E_DEC_FATAL_ERROR, result, unsigned_flag, decimal_value);
break;
}
case REAL_RESULT:
{
double result= (double)real_op();
+ if (null_value)
+ return NULL;
double2my_decimal(E_DEC_FATAL_ERROR, result, decimal_value);
break;
> }
I also propose an extended patch version, to cover the code with more
DBUG_ASSERTs, so in case of the null_value related bugs it crashes much
earlier than in Item::send(). It will be easier to debug problems this
way. To avoid much code duplication, I added three private helper methods.
The extended version of the patch is attached.
The scripts from the bug report start to return expected results,
with no crashes in DEBUG build, and "mtr --suite=main" works fine.
I have not run the entire mtr though, to save time.
Greetings.
On 09/03/2015 08:00 PM, sanja@xxxxxxxxxxx wrote:
revision-id: 62fe87c5a239c7304b245ce5954c87aecb6382ad (mariadb-5.5.45-1-g62fe87c)
parent(s): fa51f70dc68fe2f3afe943e2c81fcbdb34f16cad
committer: Oleksandr Byelkin
timestamp: 2015-09-03 18:00:43 +0200
message:
MDEV-8663: IF Statement returns multiple values erroneously (or Assertion `!null_value' failed in Item::send(Protocol*, String*))
keeping contract: NULL value mean NULL pointer in val_str and val_deciman.
---
mysql-test/r/func_if.result | 17 +++++++++++++++++
mysql-test/t/func_if.test | 14 ++++++++++++++
sql/item_cmpfunc.cc | 6 ++++--
sql/item_func.cc | 5 +++++
4 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/mysql-test/r/func_if.result b/mysql-test/r/func_if.result
index c7f548a..61f63cc 100644
--- a/mysql-test/r/func_if.result
+++ b/mysql-test/r/func_if.result
@@ -234,3 +234,20 @@ SELECT if(1, NULL, (SELECT min('hello')));
if(1, NULL, (SELECT min('hello')))
NULL
End of 5.2 tests
+#
+# MDEV-8663: IF Statement returns multiple values erroneously
+# (or Assertion `!null_value' failed in Item::send(Protocol*, String*)
+#
+CREATE TABLE `t1` (
+`datas` VARCHAR(25) NOT NULL
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+Warnings:
+Warning 1286 Unknown storage engine 'InnoDB'
+Warning 1266 Using storage engine MyISAM for table 't1'
+INSERT INTO `t1` VALUES ('1,2'), ('2,3'), ('3,4');
+SELECT IF(FIND_IN_SET('1', `datas`), 1.5, IF(FIND_IN_SET('2', `datas`), 2, NULL)) AS `First`, '1' AS `Second`, '2' AS `Third` FROM `t1`;
+First Second Third
+1.5 1 2
+2.0 1 2
+NULL 1 2
+drop table t1;
diff --git a/mysql-test/t/func_if.test b/mysql-test/t/func_if.test
index 2b89a61..8fdba77 100644
--- a/mysql-test/t/func_if.test
+++ b/mysql-test/t/func_if.test
@@ -206,6 +206,20 @@ SELECT if(1, NULL, (SELECT min('hello')));
--echo End of 5.2 tests
+--echo #
+--echo # MDEV-8663: IF Statement returns multiple values erroneously
+--echo # (or Assertion `!null_value' failed in Item::send(Protocol*, String*)
+--echo #
+CREATE TABLE `t1` (
+`datas` VARCHAR(25) NOT NULL
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+
+INSERT INTO `t1` VALUES ('1,2'), ('2,3'), ('3,4');
+
+SELECT IF(FIND_IN_SET('1', `datas`), 1.5, IF(FIND_IN_SET('2', `datas`), 2, NULL)) AS `First`, '1' AS `Second`, '2' AS `Third` FROM `t1`;
+
+drop table t1;
+
--disable_query_log
# Restore timezone to default
set time_zone= @@global.time_zone;
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 998cb1c..4f07318 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -2692,7 +2692,8 @@ Item_func_if::str_op(String *str)
String *res=arg->val_str(str);
if (res)
res->set_charset(collation.collation);
- null_value=arg->null_value;
+ if (null_value=arg->null_value)
+ res= NULL;
return res;
}
@@ -2703,7 +2704,8 @@ Item_func_if::decimal_op(my_decimal *decimal_value)
DBUG_ASSERT(fixed == 1);
Item *arg= args[0]->val_bool() ? args[1] : args[2];
my_decimal *value= arg->val_decimal(decimal_value);
- null_value= arg->null_value;
+ if (null_value= arg->null_value)
+ value= NULL;
return value;
}
diff --git a/sql/item_func.cc b/sql/item_func.cc
index 2f51308..cbb8aa0 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -924,6 +924,7 @@ String *Item_func_hybrid_result_type::val_str(String *str)
ltime.time_type= mysql_type_to_time_type(field_type());
str->length(my_TIME_to_str(<ime, const_cast<char*>(str->ptr()), decimals));
str->set_charset(&my_charset_bin);
+ DBUG_ASSERT(!null_value);
return str;
}
return str_op(&str_value);
@@ -932,6 +933,7 @@ String *Item_func_hybrid_result_type::val_str(String *str)
case IMPOSSIBLE_RESULT:
DBUG_ASSERT(0);
}
+ DBUG_ASSERT(!null_value || (str == NULL));
return str;
}
@@ -1071,7 +1073,10 @@ my_decimal *Item_func_hybrid_result_type::val_decimal(my_decimal *decimal_value)
}
String *res;
if (!(res= str_op(&str_value)))
+ {
+ null_value= 1;
return NULL;
+ }
str2my_decimal(E_DEC_FATAL_ERROR, (char*) res->ptr(),
res->length(), res->charset(), decimal_value);
_______________________________________________
commits mailing list
commits@xxxxxxxxxxx
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
diff --git a/sql/item_func.cc b/sql/item_func.cc
index da68868..011f87e 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -885,7 +885,7 @@ String *Item_func_hybrid_result_type::val_str(String *str)
case DECIMAL_RESULT:
{
my_decimal decimal_value, *val;
- if (!(val= decimal_op(&decimal_value)))
+ if (!(val= decimal_op_with_null_check(&decimal_value)))
return 0; // null is set
my_decimal_round(E_DEC_FATAL_ERROR, val, decimals, FALSE, val);
str->set_charset(collation.collation);
@@ -912,19 +912,15 @@ String *Item_func_hybrid_result_type::val_str(String *str)
if (is_temporal_type(field_type()))
{
MYSQL_TIME ltime;
- if (date_op(<ime,
- field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0) ||
- str->alloc(MAX_DATE_STRING_REP_LENGTH))
- {
- null_value= 1;
+ if (date_op_with_null_check(<ime) ||
+ (null_value= str->alloc(MAX_DATE_STRING_REP_LENGTH)))
return (String *) 0;
- }
ltime.time_type= mysql_type_to_time_type(field_type());
str->length(my_TIME_to_str(<ime, const_cast<char*>(str->ptr()), decimals));
str->set_charset(&my_charset_bin);
return str;
}
- return str_op(&str_value);
+ return str_op_with_null_check(&str_value);
case TIME_RESULT:
case ROW_RESULT:
case IMPOSSIBLE_RESULT:
@@ -942,7 +938,7 @@ double Item_func_hybrid_result_type::val_real()
{
my_decimal decimal_value, *val;
double result;
- if (!(val= decimal_op(&decimal_value)))
+ if (!(val= decimal_op_with_null_check(&decimal_value)))
return 0.0; // null is set
my_decimal2double(E_DEC_FATAL_ERROR, val, &result);
return result;
@@ -959,18 +955,14 @@ double Item_func_hybrid_result_type::val_real()
if (is_temporal_type(field_type()))
{
MYSQL_TIME ltime;
- if (date_op(<ime,
- field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0 ))
- {
- null_value= 1;
+ if (date_op_with_null_check(<ime))
return 0;
- }
ltime.time_type= mysql_type_to_time_type(field_type());
return TIME_to_double(<ime);
}
char *end_not_used;
int err_not_used;
- String *res= str_op(&str_value);
+ String *res= str_op_with_null_check(&str_value);
return (res ? my_strntod(res->charset(), (char*) res->ptr(), res->length(),
&end_not_used, &err_not_used) : 0.0);
}
@@ -990,7 +982,7 @@ longlong Item_func_hybrid_result_type::val_int()
case DECIMAL_RESULT:
{
my_decimal decimal_value, *val;
- if (!(val= decimal_op(&decimal_value)))
+ if (!(val= decimal_op_with_null_check(&decimal_value)))
return 0; // null is set
longlong result;
my_decimal2int(E_DEC_FATAL_ERROR, val, unsigned_flag, &result);
@@ -1005,18 +997,14 @@ longlong Item_func_hybrid_result_type::val_int()
if (is_temporal_type(field_type()))
{
MYSQL_TIME ltime;
- if (date_op(<ime,
- field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0))
- {
- null_value= 1;
+ if (date_op_with_null_check(<ime))
return 0;
- }
ltime.time_type= mysql_type_to_time_type(field_type());
return TIME_to_ulonglong(<ime);
}
int err_not_used;
String *res;
- if (!(res= str_op(&str_value)))
+ if (!(res= str_op_with_null_check(&str_value)))
return 0;
char *end= (char*) res->ptr() + res->length();
@@ -1038,17 +1026,21 @@ my_decimal *Item_func_hybrid_result_type::val_decimal(my_decimal *decimal_value)
DBUG_ASSERT(fixed == 1);
switch (cached_result_type) {
case DECIMAL_RESULT:
- val= decimal_op(decimal_value);
+ val= decimal_op_with_null_check(decimal_value);
break;
case INT_RESULT:
{
longlong result= int_op();
+ if (null_value)
+ return NULL;
int2my_decimal(E_DEC_FATAL_ERROR, result, unsigned_flag, decimal_value);
break;
}
case REAL_RESULT:
{
double result= (double)real_op();
+ if (null_value)
+ return NULL;
double2my_decimal(E_DEC_FATAL_ERROR, result, decimal_value);
break;
}
@@ -1057,18 +1049,16 @@ my_decimal *Item_func_hybrid_result_type::val_decimal(my_decimal *decimal_value)
if (is_temporal_type(field_type()))
{
MYSQL_TIME ltime;
- if (date_op(<ime,
- field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0))
+ if (date_op_with_null_check(<ime))
{
my_decimal_set_zero(decimal_value);
- null_value= 1;
return 0;
}
ltime.time_type= mysql_type_to_time_type(field_type());
return date2my_decimal(<ime, decimal_value);
}
String *res;
- if (!(res= str_op(&str_value)))
+ if (!(res= str_op_with_null_check(&str_value)))
return NULL;
str2my_decimal(E_DEC_FATAL_ERROR, (char*) res->ptr(),
@@ -1092,7 +1082,7 @@ bool Item_func_hybrid_result_type::get_date(MYSQL_TIME *ltime,
case DECIMAL_RESULT:
{
my_decimal value, *res;
- if (!(res= decimal_op(&value)) ||
+ if (!(res= decimal_op_with_null_check(&value)) ||
decimal_to_datetime_with_warn(res, ltime, fuzzydate,
field_name_or_null()))
goto err;
@@ -1122,7 +1112,7 @@ bool Item_func_hybrid_result_type::get_date(MYSQL_TIME *ltime,
return date_op(ltime, fuzzydate);
char buff[40];
String tmp(buff,sizeof(buff), &my_charset_bin),*res;
- if (!(res= str_op(&tmp)) ||
+ if (!(res= str_op_with_null_check(&tmp)) ||
str_to_datetime_with_warn(res->charset(), res->ptr(), res->length(),
ltime, fuzzydate))
goto err;
diff --git a/sql/item_func.h b/sql/item_func.h
index ce1f2fd..eab182c 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -419,6 +419,29 @@ class Item_real_func :public Item_func
class Item_func_hybrid_result_type: public Item_func
{
+ /*
+ Helper methods to make sure that the result of
+ decimal_op(), str_op() and date_op() is properly synched with null_value.
+ */
+ bool date_op_with_null_check(MYSQL_TIME *ltime)
+ {
+ bool rc= date_op(ltime,
+ field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0);
+ DBUG_ASSERT(!rc ^ null_value);
+ return rc;
+ }
+ String *str_op_with_null_check(String *str)
+ {
+ String *res= str_op(str);
+ DBUG_ASSERT((res != NULL) ^ null_value);
+ return res;
+ }
+ my_decimal *decimal_op_with_null_check(my_decimal *decimal_buffer)
+ {
+ my_decimal *res= decimal_op(decimal_buffer);
+ DBUG_ASSERT((res != NULL) ^ null_value);
+ return res;
+ }
protected:
Item_result cached_result_type;
