{{ message }}
Fix UBSan in MergeTreeDataPartWriterCompact::cancel#101292
Merged
Merged
Conversation
`streams_by_codec` can contain null entries when `addStreams` partially fails: `std::map::operator[]` value-initializes (inserts null) before `std::make_shared<CompressedStream>` runs, and if the allocation throws, the null entry persists. When `cancel` is later called (e.g. from the background executor's catch block), iterating and dereferencing these null entries is undefined behavior caught by UBSan. Add a null guard, consistent with the base class `MergeTreeDataPartWriterOnDisk::cancel` which already guards its optional stream pointers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
clickhouse-gh
Bot
added
pr-critical-bugfix
pr-must-backport
Member
Author
|
Let's create a test using failpoints. |
Member
Author
|
FYI, this is the top reason for crashes of 26.3. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ll check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
After rewriting the code in an exception-safe way, we can no longer add a specific test. |
alexey-milovidov
added
pr-improvement
tuanpach
approved these changes
Mar 31, 2026
Member
|
The |
nikitamikhaylov
approved these changes
Mar 31, 2026
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Member
Author
|
The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix. |
antonio2368
added a commit
that referenced
this pull request
Apr 7, 2026
…cellation The Wide writer's `addStreams` used `operator[]=make_unique<>()` which modifies the map before evaluating `make_unique`. If the constructor throws (e.g. `MEMORY_LIMIT_EXCEEDED`), a null entry remains in `column_streams`. When `cancel()` iterates the map and dereferences the null entry, it crashes with SIGSEGV. Replace with `emplace` so `make_unique` is evaluated before the map is modified — if it throws, no null entry is left. Also add a defensive null check in `cancel()`. Same class of bug as #101292 (Compact writer fix). ClickHouse/clickhouse-private#1518 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 task
alexey-milovidov
added a commit
that referenced
this pull request
Apr 10, 2026
When the join optimizer reorders a multi-table join, type changes (e.g., `toNullable` from a LEFT JOIN) were applied at the first step involving the affected relation, regardless of whether the join that causes the type change was actually being executed at that step. For example, with `t1 LEFT JOIN t2 INNER JOIN t3`, if the optimizer reorders to join t2-t3 first, the Nullable wrapping for t2 (from the LEFT JOIN with t1) was incorrectly applied at the t2-t3 step. This replaced original input nodes in `current_input_nodes`, so when the t1-t2 LEFT JOIN expression was later processed, it could not find the original `t2.id` INPUT node — causing the exception "Cannot fold actions for projection". The fix checks the `join_kinds` dependency before applying type changes: a type change for a relation is only applied when the other side of the type-changing join is also present in the current step's joined set. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101292&sha=2d129c6df44b0ed28a05397e55abe0cbbff9bb1e&name_0=PR&name_1=Stress%20test%20%28arm_msan%29 #101292 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 task
Merged
1 task
…riter-cancel # Conflicts: # src/Common/FailPoint.cpp
Contributor
LLVM Coverage ReportChanged lines: 100.00% (15/15) · Uncovered code |
robot-ch-test-poll
added
the
pr-synced-to-cloud
robot-ch-test-poll1
added
the
pr-must-backport-synced
This was referenced Apr 29, 2026
robot-ch-test-poll1
added a commit
that referenced
this pull request
May 6, 2026
Cherry pick #101292 to 26.4: Fix UBSan in MergeTreeDataPartWriterCompact::cancel
robot-clickhouse
added a commit
that referenced
this pull request
May 6, 2026
alexey-milovidov
added a commit
that referenced
this pull request
May 6, 2026
Backport #101292 to 26.4: Fix UBSan in MergeTreeDataPartWriterCompact::cancel
alexey-milovidov
added a commit
that referenced
this pull request
May 7, 2026
robot-clickhouse
added
the
pr-backports-created
Closed
robot-ch-test-poll1
removed
the
pr-backports-created
robot-ch-test-poll1
added a commit
that referenced
this pull request
May 25, 2026
Cherry pick #101292 to 26.3: Fix UBSan in MergeTreeDataPartWriterCompact::cancel
robot-clickhouse
added a commit
that referenced
this pull request
May 25, 2026
robot-ch-test-poll3
added
the
pr-backports-created
Algunenano
added a commit
that referenced
this pull request
May 25, 2026
Backport #101292 to 26.3: Fix UBSan in MergeTreeDataPartWriterCompact::cancel
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In
MergeTreeDataPartWriterCompact::addStreams,streams_by_codec[codec_id]inserts a nullshared_ptrviaoperator[]beforestd::make_sharedruns. If the allocation throws, the null entry remains in the map. Whencancelis later called (e.g. from the background executor's error path), it iteratesstreams_by_codecand dereferences the null entry, causing UBSan to report undefined behavior.The fix rewrites the insertion to be exception-safe: use
findto check for an existing entry, and onlyemplacethe fully-constructedshared_ptrreturned bystd::make_shared. If the allocation throws, the map is not modified, so no null entries can be observed bycancel.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix undefined behavior in
MergeTreeDataPartWriterCompact::cancelwhen a stream allocation fails.Documentation entry for user-facing changes
Version info
26.5.1.16426.4.2.6,26.3.13.10