Skip to content
/ server Public

MDEV-38474: ASAN heap-use-after-free in st_join_table::cleanup #4726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhishek593 wants to merge 1 commit into
base: 11.4
Choose a base branch
from
+68 −3
Open

MDEV-38474: ASAN heap-use-after-free in st_join_table::cleanup #4726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions mysql-test/suite/merge/merge.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4031,3 +4031,31 @@ UPDATE v1 SET a=0;
DROP VIEW v1;
DROP TABLE t1;
# End of 11.1 tests
#
# MDEV-38474 Double free or corruption, ASAN heap-use-after-free in st_join_table::cleanup
#
# Test case 1, fails on 10.11+
CREATE TABLE t1 (a INT);
CREATE TABLE t2 (b INT);
CREATE TABLE t3 (c INT);
# Inserts are optional, fails with and without data
INSERT INTO t1 VALUES (1),(2);
INSERT INTO t2 VALUES (3),(4);
INSERT INTO t3 VALUES (5),(6);
EXPLAIN SELECT * FROM t1 WHERE a IN (SELECT b FROM t2 WHERE a IN ((SELECT c FROM t3 WHERE FALSE HAVING c < 0)));
id select_type table type possible_keys key key_len ref rows Extra
1 PRIMARY NULL NULL NULL NULL NULL NULL NULL Impossible WHERE noticed after reading const tables
3 MATERIALIZED NULL NULL NULL NULL NULL NULL NULL Impossible WHERE
DROP TABLE t1, t2, t3;
# Test case 2, fails on 11.4 but not on 10.11
CREATE TABLE t1 (a INT);
CREATE TABLE t2 (b INT);
CREATE TABLE t3 (c INT);
CREATE TABLE t4 (d INT PRIMARY KEY);
SET SQL_SAFE_UPDATES=1;
UPDATE t1 STRAIGHT_JOIN t2 SET a = 89 WHERE 9 IN (SELECT c FROM t3 WHERE c IN (SELECT MAX(d) FROM t4));
ERROR HY000: You are using safe update mode and you tried to update a table without a WHERE that uses a KEY column
DROP TABLE t1, t2, t3, t4;
#
# End of 11.4 tests
#
34 changes: 34 additions & 0 deletions mysql-test/suite/merge/merge.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2965,3 +2965,37 @@ DROP VIEW v1;
DROP TABLE t1;

--echo # End of 11.1 tests

--echo #
--echo # MDEV-38474 Double free or corruption, ASAN heap-use-after-free in st_join_table::cleanup
--echo #

--echo # Test case 1, fails on 10.11+
CREATE TABLE t1 (a INT);
CREATE TABLE t2 (b INT);
CREATE TABLE t3 (c INT);

--echo # Inserts are optional, fails with and without data
INSERT INTO t1 VALUES (1),(2);
INSERT INTO t2 VALUES (3),(4);
INSERT INTO t3 VALUES (5),(6);

EXPLAIN SELECT * FROM t1 WHERE a IN (SELECT b FROM t2 WHERE a IN ((SELECT c FROM t3 WHERE FALSE HAVING c < 0)));

DROP TABLE t1, t2, t3;

--echo # Test case 2, fails on 11.4 but not on 10.11
CREATE TABLE t1 (a INT);
CREATE TABLE t2 (b INT);
CREATE TABLE t3 (c INT);
CREATE TABLE t4 (d INT PRIMARY KEY);

SET SQL_SAFE_UPDATES=1;
--error ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE
UPDATE t1 STRAIGHT_JOIN t2 SET a = 89 WHERE 9 IN (SELECT c FROM t3 WHERE c IN (SELECT MAX(d) FROM t4));

DROP TABLE t1, t2, t3, t4;

--echo #
--echo # End of 11.4 tests
--echo #
2 changes: 1 addition & 1 deletion sql/sql_lex.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3004,7 +3004,7 @@ void st_select_lex_node::init_query_common()
into the front of the stranded_clean_list:
before: root -> B -> A
after: root -> this -> B -> A
During cleanup, the stranded units are cleaned in FIFO order.
During cleanup, the stranded units are cleaned in LIFO order (parent-first).
*/
void st_select_lex_unit::remember_my_cleanup()
{
Expand Down
7 changes: 5 additions & 2 deletions sql/sql_union.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2688,8 +2688,6 @@ bool st_select_lex_unit::exec_recursive()

bool st_select_lex_unit::cleanup()
{
cleanup_stranded_units();

bool error= 0;
DBUG_ENTER("st_select_lex_unit::cleanup");

Expand Down Expand Up @@ -2778,6 +2776,11 @@ bool st_select_lex_unit::cleanup()
delete pushdown_unit;
pushdown_unit= nullptr;

/*
Cleanup stranded units only after this unit has completed its own
cleanup, ensuring a parent-first (LIFO) cleanup order for merged tables.
*/
cleanup_stranded_units();
Copy link
Member

@DaveGosselin-MariaDB DaveGosselin-MariaDB Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the cleanup order of units, so the comment for void st_select_lex_unit::remember_my_cleanup() introduced by commit 34a8209 needs to be updated.

Copy link
Member

@DaveGosselin-MariaDB DaveGosselin-MariaDB Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a new comment here explaining why this call should appear at the end of st_select_lex_unit::cleanup.

Copy link
Author

@abhishek593 abhishek593 Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@DaveGosselin-MariaDB DaveGosselin-MariaDB Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was hoping for a bit more detail in the analysis to make its way into this comment. Perhaps that would better appear in the git commit message. In any case, yes, you've included a test case and have added the comment, but it's not clear why we need to delay the stranded unit cleanup. Can you explain plainly why this fixes the problem and with some more details? You say that the parent-first "is required for the safe destruction of merged tables" (and I'm not doubting) but can you explain why? For some inspiration, see the commit message of the commit that you referenced in the description, 34a8209 as well as the various code-level comments in that commit.

Copy link
Author

@abhishek593 abhishek593 Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaveGosselin-MariaDB I will try with an example. Let's say, we have a query:

SELECT * FROM t1 
WHERE a IN (
    SELECT b FROM t2 
    WHERE a IN (
        SELECT c FROM t3 WHERE FALSE HAVING c < 0
    )
);

Where, Unit1 is the main query, Unit2 is the middle query, and Unit3 is the innermost query.

The list will be Unit1->Unit2->Unit3

Unit1->cleanup() begins, calls cleanup_stranded_units(), which again calls Unit2->cleanup(), calls cleanup_stranded_units(), which again calls, Unit3->cleanup(). Here, any heap allocated objects related to join are cleared. When the control reaches back to Unit2->cleanup(), it now tries its own local cleanup. Since, Unit3 was merged into Unit2, and the resources have already been freed, we have dangling pointers in Unit2 that still refers to Unit3's structures. This leads to accessing freed memory.

Moving cleanup_stranded_units() to the end of cleanup call fixes this, because Unit2 (the parent) completes its own local cleanup first (and thereby, clearing its references to any structures). Only then, Unit3's cleanup is called. Each unit cleanups up its own first, then its child.

DBUG_RETURN(error);
}

Expand Down
Loading