Skip to content

Commit

Permalink
MDEV-33010 Crash when pushing condition with CHARSET()/COERCIBILITY()…
Browse files Browse the repository at this point in the history
… into derived table

Based on the current logic, objects of classes Item_func_charset and
Item_func_coercibility (responsible for CHARSET() and COERCIBILITY()
functions) are always considered constant.
However, SQL syntax allows their use in a non-constant manner, such as
CHARSET(t1.a), COERCIBILITY(t1.a).

In these cases, the `used_tables()` parameter corresponds to table names
in the function parameters, creating an inconsistency: the item is marked
as constant but accesses tables. This leads to crashes when
conditions with CHARSET()/COERCIBILITY() are pushed into derived tables.

This commit addresses the issue by setting `used_tables()` to 0 for
`Item_func_charset` and `Item_func_coercibility`. Additionally, the items
now store the return values during the preparation phase and return
them during the execution phase. This ensures that the items do not call
its arguments methods during the execution and are truly constant.

Reviewer: Alexander Barkov <bar@mariadb.com>
  • Loading branch information
Olernov committed Jul 16, 2024
1 parent 384ec03 commit 972879f
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 18 deletions.
39 changes: 39 additions & 0 deletions mysql-test/main/derived_cond_pushdown_innodb.result
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#
# MDEV-33010: Crash when pushing condition with CHARSET()/COERCIBILITY()
# into derived table
#
CREATE TABLE t1 (c1 BIGINT, KEY (c1)) ENGINE=InnoDB;
INSERT INTO t1 VALUES (1);
CREATE TABLE t2 (c2 DOUBLE UNSIGNED);
INSERT INTO t2 VALUES (1);
SET optimizer_switch='derived_merge=off';
EXPLAIN EXTENDED
SELECT dt1_c1 FROM
(SELECT c1 AS dt1_c1 FROM t1) AS dt1
JOIN
(SELECT 1 AS dt2_c2 FROM t2) AS dt2
ON CHARSET(dt2_c2) BETWEEN dt1_c1 AND dt1_c1;
id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY <derived3> system NULL NULL NULL NULL 1 100.00
1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where
3 DERIVED t2 system NULL NULL NULL NULL 1 100.00
2 DERIVED t1 ref c1 c1 9 const 1 100.00 Using where; Using index
Warnings:
Warning 1292 Truncated incorrect DECIMAL value: 'binary'
Note 1003 /* select#1 */ select `dt1`.`dt1_c1` AS `dt1_c1` from (/* select#2 */ select `test`.`t1`.`c1` AS `dt1_c1` from `test`.`t1` where <cache>(charset(1)) between `test`.`t1`.`c1` and `test`.`t1`.`c1`) `dt1` where <cache>(charset(1)) between `dt1`.`dt1_c1` and `dt1`.`dt1_c1`
EXPLAIN EXTENDED
SELECT dt1_c1 FROM
(SELECT c1 AS dt1_c1 FROM t1) AS dt1
JOIN
(SELECT 1 AS dt2_c2 FROM t2) AS dt2
ON COERCIBILITY(dt2_c2) BETWEEN dt1_c1 AND dt1_c1;
id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY <derived3> system NULL NULL NULL NULL 1 100.00
1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where
3 DERIVED t2 system NULL NULL NULL NULL 1 100.00
2 DERIVED t1 ref c1 c1 9 const 1 100.00 Using where; Using index
Warnings:
Note 1003 /* select#1 */ select `dt1`.`dt1_c1` AS `dt1_c1` from (/* select#2 */ select `test`.`t1`.`c1` AS `dt1_c1` from `test`.`t1` where <cache>(coercibility(1)) between `test`.`t1`.`c1` and `test`.`t1`.`c1`) `dt1` where <cache>(coercibility(1)) between `dt1`.`dt1_c1` and `dt1`.`dt1_c1`
SET optimizer_switch=DEFAULT;
DROP TABLE t1, t2;
# End of 10.4 tests
31 changes: 31 additions & 0 deletions mysql-test/main/derived_cond_pushdown_innodb.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
--source include/have_innodb.inc

--echo #
--echo # MDEV-33010: Crash when pushing condition with CHARSET()/COERCIBILITY()
--echo # into derived table
--echo #
CREATE TABLE t1 (c1 BIGINT, KEY (c1)) ENGINE=InnoDB;
INSERT INTO t1 VALUES (1);
CREATE TABLE t2 (c2 DOUBLE UNSIGNED);
INSERT INTO t2 VALUES (1);

SET optimizer_switch='derived_merge=off';

EXPLAIN EXTENDED
SELECT dt1_c1 FROM
(SELECT c1 AS dt1_c1 FROM t1) AS dt1
JOIN
(SELECT 1 AS dt2_c2 FROM t2) AS dt2
ON CHARSET(dt2_c2) BETWEEN dt1_c1 AND dt1_c1;

EXPLAIN EXTENDED
SELECT dt1_c1 FROM
(SELECT c1 AS dt1_c1 FROM t1) AS dt1
JOIN
(SELECT 1 AS dt2_c2 FROM t2) AS dt2
ON COERCIBILITY(dt2_c2) BETWEEN dt1_c1 AND dt1_c1;

SET optimizer_switch=DEFAULT;
DROP TABLE t1, t2;

--echo # End of 10.4 tests
8 changes: 1 addition & 7 deletions mysql-test/main/olap.result
Original file line number Diff line number Diff line change
Expand Up @@ -787,18 +787,12 @@ DROP TABLE t1;
#
# MDEV-17830 Server crashes in Item_null_result::field_type upon SELECT with CHARSET(date) and ROLLUP
#
# Note, different MariaDB versions can return different results
# in the two rows (such as "latin1" vs "binary"). This is wrong.
# Both lines should return equal values.
# The point in this test is to make sure it does not crash.
# As this is a minor issue, bad result will be fixed
# in a later version, presumably in 10.4.
CREATE TABLE t (d DATE) ENGINE=MyISAM;
INSERT INTO t VALUES ('2018-12-12');
SELECT CHARSET(d) AS f FROM t GROUP BY d WITH ROLLUP;
f
binary
latin1
binary
DROP TABLE t;
#
# MDEV-14041 Server crashes in String::length on queries with functions and ROLLUP
Expand Down
7 changes: 0 additions & 7 deletions mysql-test/main/olap.test
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,6 @@ DROP TABLE t1;
--echo # MDEV-17830 Server crashes in Item_null_result::field_type upon SELECT with CHARSET(date) and ROLLUP
--echo #

--echo # Note, different MariaDB versions can return different results
--echo # in the two rows (such as "latin1" vs "binary"). This is wrong.
--echo # Both lines should return equal values.
--echo # The point in this test is to make sure it does not crash.
--echo # As this is a minor issue, bad result will be fixed
--echo # in a later version, presumably in 10.4.

CREATE TABLE t (d DATE) ENGINE=MyISAM;
INSERT INTO t VALUES ('2018-12-12');
SELECT CHARSET(d) AS f FROM t GROUP BY d WITH ROLLUP;
Expand Down
18 changes: 17 additions & 1 deletion sql/item_func.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3143,11 +3143,27 @@ longlong Item_func_char_length::val_int()
}


bool Item_func_coercibility::fix_length_and_dec()
{
max_length=10;
maybe_null= 0;
/*
Since this is a const item which doesn't use tables (see used_tables()),
we don't want to access the function arguments during execution.
That's why we store the derivation here during the preparation phase
and only return it later at the execution phase
*/
DBUG_ASSERT(args[0]->is_fixed());
m_cached_collation_derivation= (longlong) args[0]->collation.derivation;
return false;
}


longlong Item_func_coercibility::val_int()
{
DBUG_ASSERT(fixed == 1);
null_value= 0;
return (longlong) args[0]->collation.derivation;
return m_cached_collation_derivation;
}


Expand Down
5 changes: 4 additions & 1 deletion sql/item_func.h
Original file line number Diff line number Diff line change
Expand Up @@ -2285,13 +2285,15 @@ class Item_func_char_length :public Item_long_func_length

class Item_func_coercibility :public Item_long_func
{
longlong m_cached_collation_derivation;

bool check_arguments() const override
{ return args[0]->check_type_can_return_str(func_name()); }
public:
Item_func_coercibility(THD *thd, Item *a): Item_long_func(thd, a) {}
longlong val_int() override;
const char *func_name() const override { return "coercibility"; }
bool fix_length_and_dec() override { max_length=10; maybe_null= 0; return FALSE; }
bool fix_length_and_dec() override;
bool eval_not_null_tables(void *) override
{
not_null_tables_cache= 0;
Expand All @@ -2306,6 +2308,7 @@ class Item_func_coercibility :public Item_long_func
bool const_item() const override { return true; }
Item *do_get_copy(THD *thd) const override
{ return get_item_copy<Item_func_coercibility>(thd, this); }
table_map used_tables() const override { return 0; }
};


Expand Down
3 changes: 1 addition & 2 deletions sql/item_strfunc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3625,9 +3625,8 @@ String *Item_func_charset::val_str(String *str)
DBUG_ASSERT(fixed == 1);
uint dummy_errors;

CHARSET_INFO *cs= args[0]->charset_for_protocol();
null_value= 0;
str->copy(cs->csname, (uint) strlen(cs->csname),
str->copy(m_cached_charset_info.str, m_cached_charset_info.length,
&my_charset_latin1, collation.collation, &dummy_errors);
return str;
}
Expand Down
19 changes: 19 additions & 0 deletions sql/item_strfunc.h
Original file line number Diff line number Diff line change
Expand Up @@ -1692,13 +1692,32 @@ class Item_func_expr_str_metadata :public Item_str_func

class Item_func_charset :public Item_func_expr_str_metadata
{
LEX_CSTRING m_cached_charset_info;

public:
Item_func_charset(THD *thd, Item *a)
:Item_func_expr_str_metadata(thd, a) { }
String *val_str(String *) override;
const char *func_name() const override { return "charset"; }
Item *do_get_copy(THD *thd) const override
{ return get_item_copy<Item_func_charset>(thd, this); }
table_map used_tables() const override { return 0; }
bool fix_length_and_dec() override
{
if (Item_func_expr_str_metadata::fix_length_and_dec())
return true;
/*
Since this is a const item which doesn't use tables (see used_tables()),
we don't want to access the function arguments during execution.
That's why we store the charset here during the preparation phase
and only return it later at the execution phase
*/
DBUG_ASSERT(args[0]->is_fixed());
m_cached_charset_info.str= args[0]->charset_for_protocol()->csname;
m_cached_charset_info.length=
strlen(args[0]->charset_for_protocol()->csname);
return false;
}
};


Expand Down

0 comments on commit 972879f

Please sign in to comment.