Commit bb3fbba1 authored by Alexey Kopytov's avatar Alexey Kopytov

Bug #54476: crash when group_concat and 'with rollup' in

                     prepared statements

Using GROUP_CONCAT() together with the WITH ROLLUP modifier
could crash the server.

The reason was a combination of several facts:

1. The Item_func_group_concat class stores pointers to ORDER
objects representing the columns in the ORDER BY clause of
GROUP_CONCAT().

2. find_order_in_list() called from
Item_func_group_concat::setup() modifies the ORDER objects so
that their 'item' member points to the arguments list
allocated in the Item_func_group_concat constructor.

3. In some cases (e.g. in JOIN::rollup_make_fields) a copy of
the original Item_func_group_concat object could be created by
using the Item_func_group_concat::Item_func_group_concat(THD
*thd, Item_func_group_concat *item) copy constructor. The
latter essentially creates a shallow copy of the source
object. Memory for the arguments array is allocated on
thd->mem_root, but the pointers for arguments and ORDER are
copied verbatim.

What happens in the test case is that when executing the query
for the first time, after a copy of the original
Item_func_group_concat object has been created by
JOIN::rollup_make_fields(), find_order_in_list() is called for
this new object. It then resolves ORDER BY by modifying the
ORDER objects so that they point to elements of the arguments
array which is local to the cloned object. When thd->mem_root
is freed upon completing the execution, pointers in the ORDER
objects become invalid. Those ORDER objects, however, are also
shared with the original Item_func_group_concat object which is
preserved between executions of a prepared statement. So the
first call to find_order_in_list() for the original object on
the second execution tries to dereference an invalid pointer.

The solution is to create copies of the ORDER objects when
copying Item_func_group_concat to not leave any stale pointers
in other instances with different lifecycles.
parent 0e7e4a02
...@@ -995,6 +995,7 @@ SELECT 1 FROM ...@@ -995,6 +995,7 @@ SELECT 1 FROM
1 1
1 1
DROP TABLE t1; DROP TABLE t1;
End of 5.0 tests
# #
# Bug #52397: another crash with explain extended and group_concat # Bug #52397: another crash with explain extended and group_concat
# #
...@@ -1010,4 +1011,22 @@ id select_type table type possible_keys key key_len ref rows filtered Extra ...@@ -1010,4 +1011,22 @@ id select_type table type possible_keys key key_len ref rows filtered Extra
Warnings: Warnings:
Note 1003 select 1 AS `1` from (select group_concat(`test`.`t1`.`a` order by `test`.`t1`.`a` ASC separator ',') AS `GROUP_CONCAT(t1.a ORDER BY t1.a ASC)` from `test`.`t1` `t2` join `test`.`t1` group by `test`.`t1`.`a`) `d` Note 1003 select 1 AS `1` from (select group_concat(`test`.`t1`.`a` order by `test`.`t1`.`a` ASC separator ',') AS `GROUP_CONCAT(t1.a ORDER BY t1.a ASC)` from `test`.`t1` `t2` join `test`.`t1` group by `test`.`t1`.`a`) `d`
DROP TABLE t1; DROP TABLE t1;
End of 5.0 tests #
# Bug #54476: crash when group_concat and 'with rollup' in prepared statements
#
CREATE TABLE t1 (a INT);
INSERT INTO t1 VALUES (1), (2);
PREPARE stmt FROM "SELECT GROUP_CONCAT(t1.a ORDER BY t1.a) FROM t1 JOIN t1 t2 GROUP BY t1.a WITH ROLLUP";
EXECUTE stmt;
GROUP_CONCAT(t1.a ORDER BY t1.a)
1,1
2,2
1,1,2,2
EXECUTE stmt;
GROUP_CONCAT(t1.a ORDER BY t1.a)
1,1
2,2
1,1,2,2
DEALLOCATE PREPARE stmt;
DROP TABLE t1;
End of 5.1 tests
...@@ -708,6 +708,7 @@ SELECT 1 FROM ...@@ -708,6 +708,7 @@ SELECT 1 FROM
DROP TABLE t1; DROP TABLE t1;
--echo End of 5.0 tests
--echo # --echo #
--echo # Bug #52397: another crash with explain extended and group_concat --echo # Bug #52397: another crash with explain extended and group_concat
...@@ -719,5 +720,18 @@ EXPLAIN EXTENDED SELECT 1 FROM ...@@ -719,5 +720,18 @@ EXPLAIN EXTENDED SELECT 1 FROM
t1 t2, t1 GROUP BY t1.a) AS d; t1 t2, t1 GROUP BY t1.a) AS d;
DROP TABLE t1; DROP TABLE t1;
--echo #
--echo # Bug #54476: crash when group_concat and 'with rollup' in prepared statements
--echo #
--echo End of 5.0 tests CREATE TABLE t1 (a INT);
INSERT INTO t1 VALUES (1), (2);
PREPARE stmt FROM "SELECT GROUP_CONCAT(t1.a ORDER BY t1.a) FROM t1 JOIN t1 t2 GROUP BY t1.a WITH ROLLUP";
EXECUTE stmt;
EXECUTE stmt;
DEALLOCATE PREPARE stmt;
DROP TABLE t1;
--echo End of 5.1 tests
...@@ -3034,7 +3034,6 @@ Item_func_group_concat::Item_func_group_concat(THD *thd, ...@@ -3034,7 +3034,6 @@ Item_func_group_concat::Item_func_group_concat(THD *thd,
tree(item->tree), tree(item->tree),
unique_filter(item->unique_filter), unique_filter(item->unique_filter),
table(item->table), table(item->table),
order(item->order),
context(item->context), context(item->context),
arg_count_order(item->arg_count_order), arg_count_order(item->arg_count_order),
arg_count_field(item->arg_count_field), arg_count_field(item->arg_count_field),
...@@ -3047,6 +3046,24 @@ Item_func_group_concat::Item_func_group_concat(THD *thd, ...@@ -3047,6 +3046,24 @@ Item_func_group_concat::Item_func_group_concat(THD *thd,
{ {
quick_group= item->quick_group; quick_group= item->quick_group;
result.set_charset(collation.collation); result.set_charset(collation.collation);
/*
Since the ORDER structures pointed to by the elements of the 'order' array
may be modified in find_order_in_list() called from
Item_func_group_concat::setup(), create a copy of those structures so that
such modifications done in this object would not have any effect on the
object being copied.
*/
ORDER *tmp;
if (!(order= (ORDER **) thd->alloc(sizeof(ORDER *) * arg_count_order +
sizeof(ORDER) * arg_count_order)))
return;
tmp= (ORDER *)(order + arg_count_order);
for (uint i= 0; i < arg_count_order; i++, tmp++)
{
memcpy(tmp, item->order[i], sizeof(ORDER));
order[i]= tmp;
}
} }
......
...@@ -55,7 +55,6 @@ typedef struct st_order { ...@@ -55,7 +55,6 @@ typedef struct st_order {
struct st_order *next; struct st_order *next;
Item **item; /* Point at item in select fields */ Item **item; /* Point at item in select fields */
Item *item_ptr; /* Storage for initial item */ Item *item_ptr; /* Storage for initial item */
Item **item_copy; /* For SPs; the original item ptr */
int counter; /* position in SELECT list, correct int counter; /* position in SELECT list, correct
only if counter_used is true*/ only if counter_used is true*/
bool asc; /* true if ascending */ bool asc; /* true if ascending */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment