Fix a spurious AST fuzzer abort on a recycled node address - #113114
Conversation
`QueryFuzzer::fuzz` detects loops in the AST by remembering the address of every node it visits during one `fuzzMain` call, and calls `std::abort` when it sees an address twice. But fuzzing legitimately destroys parts of the query it has already visited: wrapping a table into `Remote` / `Distributed` / `Buffer` drops the whole column list, a random constraint or projection is erased, a key clause is removed from a storage definition, a child is replaced with a new node. Once such a node is freed, the allocator is free to hand its address to a node created later - for instance to a data type parsed by `fuzzColumnDeclaration` for a column of another table - and the check then aborts the server although the AST contains no loop at all. This is what happened in the report below, where the fuzzer wrapped `CREATE TABLE table1(x Int32)` into `Remote(...)` (dropping the already visited column list of `table1`) and then parsed `Array(UInt16)` for the column of `table2`: the dumped AST contains the reported node exactly once. Keep every visited node alive for the duration of the `fuzzMain` call, so that its address cannot be recycled and pointer identity remains a valid answer to "have I visited this node before". https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=dd1e53dfe2715c22d4b7e63c2459bc9eec994581&name_0=MasterCI&name_1=Stress%20test%20%28arm_release%29
|
Workflow [PR], commit [794880a] Summary: ✅
AI ReviewSummaryThis PR changes the AST fuzzer's loop detection from a raw-pointer set to a pointer-to- Findings❌ Blockers
Final VerdictStatus: Minimum required actions:
|
|
@alexey-milovidov let me look at it. It should be from the PR I merged this morning |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| // node is destroyed the allocator is free to hand its address to a node created later, which | ||
| // would look exactly like a loop. Holding the node alive makes the address unique for the whole | ||
| // fuzzMain call, so pointer identity is a valid answer to "have I visited this node before". | ||
| std::unordered_map<const IAST *, ASTPtr> debug_visited_nodes; |
There was a problem hiding this comment.
This doesn't actually keep every visited node alive for the full fuzzMain call, because some rewrite branches still erase(ast.get()) before overwriting the ASTPtr. The concrete ASTDataType path at src/Common/QueryFuzzer.cpp:8127 is enough to recreate the original ABA: once the old type node is reparsed, the previous node can be destroyed immediately, and a later sibling parse in the same fuzzMain can recycle that address and trip the loop check again. I think this needs a monotonic keepalive container for the whole fuzzMain (or a separate keepalive container plus the visited-address set), rather than erasing entries on replacement.
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
3 participants
The
Stress testjob kills the server withReceived signal 6 (internal)from inside the AST fuzzer itself:QueryFuzzer::fuzzdetects loops in the AST by remembering the address of every node it visits during onefuzzMaincall, and callsstd::abortwhen it sees an address twice. But fuzzing legitimately destroys parts of the query it has already visited: wrapping a table intoRemote/Distributed/Bufferdrops the whole column list, a random constraint or projection is erased, a key clause is removed from a storage definition, a child is replaced with a new node. Once such a node is freed, the allocator is free to hand its address to a node created later, and the check then aborts the server although the AST contains no loop at all.That is what happened here. The AST the fuzzer dumped before aborting (from
stderr.log) contains the reported node0xff5442bef380exactly once, so there is no loop:The corpus query was
CREATE TABLE table1(x Int32) ... PARALLEL WITH CREATE TABLE table2(y Int32) ...from03305_parallel_with.sql. The fuzzer first fuzzed the column list oftable1(visiting its column declaration and data type), then wrappedtable1intoRemote(...)and dropped that column list, freeing the nodes it had just visited. ParsingArray(UInt16)for the column oftable2afterwards got one of the freed addresses back.The fix keeps every visited node alive for the duration of the
fuzzMaincall, so that its address cannot be recycled and pointer identity remains a valid answer to "have I visited this node before". Real loops are still detected.No test: the failure depends on the allocator handing back a specific address, so it cannot be pinned down deterministically.
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=dd1e53dfe2715c22d4b7e63c2459bc9eec994581&name_0=MasterCI&name_1=Stress%20test%20%28arm_release%29
Changelog category (leave one):
Version info
26.8.1.777(included in26.8and later)