ClickHouse / ClickHouse/ClickHouse

Use-after-free of a dangling output peer in `IProcessor::getProcessorsProfileLogInfo` (release-reachable)

Open
#112,320 2 comments 0 reactions 0 assignees View on GitHub
comp-query-execution
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

`IProcessor::getProcessorsProfileLogInfo` (`src/Processors/IProcessor.cpp:158-172`) dereferences the peer processor of every connected output port:

```cpp
for (const auto & port : outputs)
{
if (!port.isConnected())
continue;
const IProcessor & next = port.getInputPort().getProcessor();
info.parent_ids.push_back(get_proc_id(next));
}
```

`ExecutingGraph::removeNode` erases finished processors from the shared `Processors` list, and `delayed_destruction` frees them before the post-execution logging passes run. After that, a surviving output port can still report `isConnected()` while its peer input port lives inside a freed processor, so `getInputPort().getProcessor()` (`src/Processors/Port.h`) reads freed memory. This is the same lifetime problem that https://github.com/ClickHouse/ClickHouse/pull/110955 removed from `printPipeline`, but unlike `dumpPipeline` this path is reachable in release builds: `finalizeQueryPipelineBeforeLogging` / `logQueryFinish` (`src/Interpreters/executeQuery.cpp`), `PreparedSets`, and scalar-subquery logging all call `getProcessorsProfileLogInfo` on successful query completion, and `ProcessorsProfileLogElement` then stores the collected `parent_ids`.

It was found by the automated review of https://github.com/ClickHouse/ClickHouse/pull/110955 and independently confirmed there.

### How to fix

A first attempt, https://github.com/ClickHouse/ClickHouse/pull/111019, collected `parent_ids` for the whole supplied processor set and matched ports through the shared port-connection identity (the approach used by `printPipeline` after #110955), so that no peer processor is dereferenced. It was closed with design objections from @yariks5s, which are worth recording because any fix has to answer them:

- a per-processor `getProcessorsProfileLogInfo` that no longer fills `parent_ids` still promises to, by its name and return type;
- peers that are absent from the supplied processor set silently disappear from `system.processors_profile_log`, which changes the table's semantics;
- it adds a hash map allocation on every successful-query finish path, including when profile logging is disabled;
- matching by connection identity works around the dangling pointers instead of fixing the lifetime that produces them.

So the open question is a design one: either make the `Port` / processor lifetime guarantee strong enough that a connected output can never outlive its peer processor (which fixes the class of bugs rather than one site), or define what `parent_ids` means for peers outside the logged set and collect them without dereferencing the peer.

Related: https://github.com/ClickHouse/ClickHouse/pull/110955
Related: https://github.com/ClickHouse/ClickHouse/pull/111019
Related: https://github.com/ClickHouse/ClickHouse/issues/110834

Contributor guide

Open the contributing guide

Research direction

Start with IProcessor::getProcessorsProfileLogInfo in src/Processors/IProcessor.cpp and the port lifetime behavior in src/Processors/Port.h, then trace ExecutingGraph::removeNode and delayed_destruction. Read the related changes in PRs #110955 and #111019 before examining finalizeQueryPipelineBeforeLogging and logQueryFinish in src/Interpreters/executeQuery.cpp. Done means a resolved lifetime or semantics design prevents release-build use-after-free while preserving the intended parent_ids behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, observability
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.