Skip to content

Commit 0d927a5

Browse files
OlernovOlernov
committed
MDEV-29624 MDEV-29655 Fix ASAN errors on pushdown of derived table
Deallocation of TABLE_LIST::dt_handler and TABLE_LIST::pushdown_derived was performed in multiple places if code. This not only made the code more difficult to maintain but also led to memory leaks and ASAN heap-use-after-free errors. This commit puts deallocation of TABLE_LIST::dt_handler and TABLE_LIST::pushdown_derived to the single point - JOIN::cleanup()
1 parent ce443c8 commit 0d927a5

File tree

6 files changed

+132
-19
lines changed

6 files changed

+132
-19
lines changed

‎mysql-test/suite/federated/federatedx_create_handlers.result

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,51 @@ a
469469
1
470470
2
471471
3
472+
#
473+
# MDEV-29655: ASAN heap-use-after-free in
474+
# Pushdown_derived::Pushdown_derived
475+
#
476+
connection slave;
477+
DROP TABLE IF EXISTS federated.t1;
478+
CREATE TABLE federated.t1 (
479+
id int(20) NOT NULL,
480+
name varchar(16) NOT NULL default ''
481+
)
482+
DEFAULT CHARSET=latin1;
483+
INSERT INTO federated.t1 VALUES
484+
(3,'xxx'), (7,'yyy'), (4,'xxx'), (1,'zzz'), (5,'yyy');
485+
connection master;
486+
DROP TABLE IF EXISTS federated.t1;
487+
CREATE TABLE federated.t1 (
488+
id int(20) NOT NULL,
489+
name varchar(16) NOT NULL default ''
490+
)
491+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
492+
CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t1';
493+
use federated;
494+
SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM t1 where id=3) dt3
495+
WHERE id=2) dt2) dt;
496+
id name
497+
connection slave;
498+
CREATE TABLE federated.t10 (a INT,b INT);
499+
CREATE TABLE federated.t11 (a INT, b INT);
500+
INSERT INTO federated.t10 VALUES (1,1),(2,2);
501+
INSERT INTO federated.t11 VALUES (1,1),(2,2);
502+
connection master;
503+
CREATE TABLE federated.t10
504+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
505+
CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t10';
506+
CREATE TABLE federated.t11
507+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
508+
CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t11';
509+
use federated;
510+
SELECT * FROM t10 LEFT JOIN
511+
(t11, (SELECT * FROM (SELECT * FROM (SELECT * FROM t1 where id=3) dt3
512+
WHERE id=2) dt2) dt
513+
) ON t10.a=t11.a;
514+
a b a b id name
515+
1 1 NULL NULL NULL NULL
516+
2 2 NULL NULL NULL NULL
472517
set global federated_pushdown=0;
473518
connection master;
474519
DROP TABLE IF EXISTS federated.t1;

‎mysql-test/suite/federated/federatedx_create_handlers.test

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,6 @@ INSERT INTO federated.t2
267267
SELECT * FROM (SELECT * FROM federated.t1 LIMIT 70000) dt;
268268
SELECT COUNT(DISTINCT a) FROM federated.t2;
269269

270-
271270
--echo #
272271
--echo # MDEV-29640 FederatedX does not properly handle pushdown
273272
--echo # in case of difference in local and remote table names
@@ -314,6 +313,64 @@ CREATE TABLE federated.t3 (a INT)
314313
EXPLAIN SELECT * FROM federated.t3;
315314
SELECT * FROM federated.t3;
316315

316+
--echo #
317+
--echo # MDEV-29655: ASAN heap-use-after-free in
318+
--echo # Pushdown_derived::Pushdown_derived
319+
--echo #
320+
321+
connection slave;
322+
DROP TABLE IF EXISTS federated.t1;
323+
324+
CREATE TABLE federated.t1 (
325+
id int(20) NOT NULL,
326+
name varchar(16) NOT NULL default ''
327+
)
328+
DEFAULT CHARSET=latin1;
329+
330+
INSERT INTO federated.t1 VALUES
331+
(3,'xxx'), (7,'yyy'), (4,'xxx'), (1,'zzz'), (5,'yyy');
332+
333+
connection master;
334+
DROP TABLE IF EXISTS federated.t1;
335+
336+
--replace_result $SLAVE_MYPORT SLAVE_PORT
337+
eval
338+
CREATE TABLE federated.t1 (
339+
id int(20) NOT NULL,
340+
name varchar(16) NOT NULL default ''
341+
)
342+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
343+
CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1';
344+
345+
use federated;
346+
SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM t1 where id=3) dt3
347+
WHERE id=2) dt2) dt;
348+
349+
connection slave;
350+
CREATE TABLE federated.t10 (a INT,b INT);
351+
CREATE TABLE federated.t11 (a INT, b INT);
352+
INSERT INTO federated.t10 VALUES (1,1),(2,2);
353+
INSERT INTO federated.t11 VALUES (1,1),(2,2);
354+
355+
connection master;
356+
--replace_result $SLAVE_MYPORT SLAVE_PORT
357+
eval
358+
CREATE TABLE federated.t10
359+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
360+
CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t10';
361+
362+
--replace_result $SLAVE_MYPORT SLAVE_PORT
363+
eval
364+
CREATE TABLE federated.t11
365+
ENGINE="FEDERATED" DEFAULT CHARSET=latin1
366+
CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t11';
367+
368+
use federated;
369+
SELECT * FROM t10 LEFT JOIN
370+
(t11, (SELECT * FROM (SELECT * FROM (SELECT * FROM t1 where id=3) dt3
371+
WHERE id=2) dt2) dt
372+
) ON t10.a=t11.a;
373+
317374
set global federated_pushdown=0;
318375

319376
source include/federated_cleanup.inc;

‎sql/derived_handler.cc

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,6 @@ Pushdown_derived::Pushdown_derived(TABLE_LIST *tbl, derived_handler *h)
4444
}
4545

4646

47-
Pushdown_derived::~Pushdown_derived()
48-
{
49-
delete handler;
50-
}
51-
5247

5348
int Pushdown_derived::execute()
5449
{

‎sql/sql_derived.cc

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,11 +1000,7 @@ bool mysql_derived_optimize(THD *thd, LEX *lex, TABLE_LIST *derived)
10001000
/* Create an object for execution of the query specifying the table */
10011001
if (!(derived->pushdown_derived=
10021002
new (thd->mem_root) Pushdown_derived(derived, derived->dt_handler)))
1003-
{
1004-
delete derived->dt_handler;
1005-
derived->dt_handler= NULL;
10061003
DBUG_RETURN(TRUE);
1007-
}
10081004
}
10091005

10101006
lex->current_select= first_select;
@@ -1229,7 +1225,6 @@ bool mysql_derived_fill(THD *thd, LEX *lex, TABLE_LIST *derived)
12291225
/* Execute the query that specifies the derived table by a foreign engine */
12301226
res= derived->pushdown_derived->execute();
12311227
unit->executed= true;
1232-
delete derived->pushdown_derived;
12331228
DBUG_RETURN(res);
12341229
}
12351230

‎sql/sql_select.cc

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
#include "select_handler.h"
6969
#include "my_json_writer.h"
7070
#include "opt_trace.h"
71+
#include "derived_handler.h"
7172

7273
/*
7374
A key part number that means we're using a fulltext scan.
@@ -14086,6 +14087,7 @@ void JOIN::cleanup(bool full)
1408614087
}
1408714088
}
1408814089
}
14090+
free_pushdown_handlers(*join_list);
1408914091
}
1409014092
/* Restore ref array to original state */
1409114093
if (current_ref_ptrs != items0)
@@ -14096,6 +14098,32 @@ void JOIN::cleanup(bool full)
1409614098
DBUG_VOID_RETURN;
1409714099
}
1409814100

14101+
/**
14102+
Clean up all derived pushdown handlers in this join.
14103+
14104+
@detail
14105+
Note that dt_handler is picked at the prepare stage (as opposed
14106+
to optimization stage where one could expect this).
14107+
Because of that, we have to do cleanups in this function that is called
14108+
from JOIN::cleanup() and not in JOIN_TAB::cleanup.
14109+
*/
14110+
void JOIN::free_pushdown_handlers(List<TABLE_LIST>& join_list)
14111+
{
14112+
List_iterator<TABLE_LIST> li(join_list);
14113+
TABLE_LIST *table_ref;
14114+
while ((table_ref= li++))
14115+
{
14116+
if (table_ref->nested_join)
14117+
free_pushdown_handlers(table_ref->nested_join->join_list);
14118+
if (table_ref->pushdown_derived)
14119+
{
14120+
delete table_ref->pushdown_derived;
14121+
table_ref->pushdown_derived= NULL;
14122+
}
14123+
delete table_ref->dt_handler;
14124+
table_ref->dt_handler= NULL;
14125+
}
14126+
}
1409914127

1410014128
/**
1410114129
Remove the following expressions from ORDER BY and GROUP BY:
@@ -27400,12 +27428,6 @@ bool mysql_explain_union(THD *thd, SELECT_LEX_UNIT *unit, select_result *result)
2740027428
result, unit, first);
2740127429
}
2740227430

27403-
if (unit->derived && unit->derived->pushdown_derived)
27404-
{
27405-
delete unit->derived->pushdown_derived;
27406-
unit->derived->pushdown_derived= NULL;
27407-
}
27408-
2740927431
DBUG_RETURN(res || thd->is_error());
2741027432
}
2741127433

‎sql/sql_select.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1840,6 +1840,7 @@ class JOIN :public Sql_alloc
18401840
bool add_having_as_table_cond(JOIN_TAB *tab);
18411841
bool make_aggr_tables_info();
18421842
bool add_fields_for_current_rowid(JOIN_TAB *cur, List<Item> *fields);
1843+
void free_pushdown_handlers(List<TABLE_LIST>& join_list);
18431844
};
18441845

18451846
enum enum_with_bush_roots { WITH_BUSH_ROOTS, WITHOUT_BUSH_ROOTS};
@@ -2507,8 +2508,6 @@ class Pushdown_derived: public Sql_alloc
25072508

25082509
Pushdown_derived(TABLE_LIST *tbl, derived_handler *h);
25092510

2510-
~Pushdown_derived();
2511-
25122511
int execute();
25132512
};
25142513

0 commit comments

Comments
 (0)