Skip to content

Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns#88605

Merged
azat merged 1 commit intoClickHouse:masterfrom
azat:prewhere-const-shrink-fix
Oct 17, 2025
Merged

Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns#88605
azat merged 1 commit intoClickHouse:masterfrom
azat:prewhere-const-shrink-fix

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Oct 15, 2025 •
edited
Loading

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns

The issue was that a constant column in PREWHERE could be mutated concurrently by multiple threads (in MergeTreeReadTask::read() via shrinkToFit()), because for single-row columns it returns the same object without copying (e.g., via materialize()).

Fixes: #85404

@azat azat added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Oct 15, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 15, 2025 •
edited
Loading

Workflow [PR], commit [25675b2]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, parallel) failure
03237_insert_sparse_columns_mem FAIL cidb
Stateless tests (amd_msan, parallel, 2/2) failure
03602_alter_update_nullable_json FAIL cidb
Bugfix validation (functional tests) failure
Integration tests (amd_tsan, 6/6) failure
test_database_hms/test.py::test_hive_catalog_url_parsing FAIL cidb

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Oct 15, 2025
@azat azat changed the title Fix mutating const columns (shrinkToFit()) from PREWHERE from multiple threads Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns Oct 15, 2025
@azat
Copy link
Copy Markdown
Member Author

azat commented Oct 15, 2025

[2025-10-15 20:35:10] failure [Bugfix validation (functional tests)]
[2025-10-15 20:35:10] | Failed: 0, Passed: 1, Skipped: 0

Test requires sanitizer to reproduce the issue

@azat azat force-pushed the prewhere-const-shrink-fix branch from 9623805 to 954d0f4 Compare October 15, 2025 22:11
@davenger
Copy link
Copy Markdown
Member

davenger commented Oct 16, 2025

It looks like the problem is caused by this dangerous call of assumeMutableRef() which is doing a const_cast
https://github.com/ClickHouse/ClickHouse/blob/master/src/Storages/MergeTree/MergeTreeReadTask.cpp#L354
Maybe it's cleaner to clone const columns when they are added to read_result.columns?

@davenger davenger self-assigned this Oct 16, 2025
@azat
Copy link
Copy Markdown
Member Author

azat commented Oct 16, 2025

Maybe it's cleaner to clone const columns when they are added to read_result.columns?

You mean here, right?

ClickHouse/src/Storages/MergeTree/MergeTreeRangeReader.cpp

Line 1491 in 9dea2ef

result.columns.emplace_back(std::move(col.column));
(or maybe even later)

But the problem is that here we do not have const column anymore, since i.e. materialize will simply return underlying column -

ClickHouse/src/Functions/materialize.h

Line 59 in 9dea2ef

auto res = arguments[0].column->convertToFullColumnIfConst();

@azat azat force-pushed the prewhere-const-shrink-fix branch from 954d0f4 to 9273b7f Compare October 16, 2025 12:42
@azat
Copy link
Copy Markdown
Member Author

azat commented Oct 16, 2025

Rewrote it to

if (column->use_count() == 1)
    column->assumeMutableRef().shrinkToFit();

@azat azat force-pushed the prewhere-const-shrink-fix branch from 9273b7f to 64833aa Compare October 16, 2025 16:25
@azat azat enabled auto-merge October 16, 2025 16:27
@azat
Fix potential crash caused by concurrent mutation of underlying const…
25675b2 
… PREWHERE columns

The issue was that in MergeTreeReadTask we may have column that has
other references, usually it is a constant column that created during
analysis (that is not a constant anymore where we call shrink, i.e.
after `materialize()`), and we do not need to mutate such column anyway.

v2: move code from MergeTreeSplitPrewhereIntoReadSteps.cpp::addClonedDAGToDAG() into MergeTreeReadTask
@azat azat force-pushed the prewhere-const-shrink-fix branch from 64833aa to 25675b2 Compare October 16, 2025 18:29
@azat
Copy link
Copy Markdown
Member Author

azat commented Oct 17, 2025 •
edited
Loading

CI is unrelated, something is temporary, something is known

@azat azat added this pull request to the merge queue Oct 17, 2025
Merged via the queue into ClickHouse:master with commit 8ad6289 Oct 17, 2025
118 of 123 checks passed
@azat azat deleted the prewhere-const-shrink-fix branch October 17, 2025 07:44
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 17, 2025
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88711 from ClickHouse/cherrypick/25.3/88605
5e4e359 
Cherry pick #88605 to 25.3: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.3: Fix potential crash caused by concurrent mut…
973c382 
…ation of underlying const PREWHERE columns
This was referenced Oct 17, 2025
Merged
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88713 from ClickHouse/cherrypick/25.7/88605
4bf7d89 
Cherry pick #88605 to 25.7: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.7: Fix potential crash caused by concurrent mut…
88a20e6 
…ation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88715 from ClickHouse/cherrypick/25.8/88605
f07a211 
Cherry pick #88605 to 25.8: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.8: Fix potential crash caused by concurrent mut…
5426bfc 
…ation of underlying const PREWHERE columns
This was referenced Oct 17, 2025
Merged
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88717 from ClickHouse/cherrypick/25.9/88605
055e9e8 
Cherry pick #88605 to 25.9: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.9: Fix potential crash caused by concurrent mut…
e47365f 
…ation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
clickhouse-gh bot added a commit that referenced this pull request Oct 17, 2025
@clickhouse-gh
Merge pull request #88714 from ClickHouse/backport/25.7/88605
8d3e880 
Backport #88605 to 25.7: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
clickhouse-gh bot added a commit that referenced this pull request Oct 17, 2025
@clickhouse-gh
Merge pull request #88716 from ClickHouse/backport/25.8/88605
261935c 
Backport #88605 to 25.8: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Oct 17, 2025
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 17, 2025
azat added a commit that referenced this pull request Oct 20, 2025
@azat
Merge pull request #88718 from ClickHouse/backport/25.9/88605
1929e30 
Backport #88605 to 25.9: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
azat added a commit that referenced this pull request Oct 20, 2025
@azat
Merge pull request #88712 from ClickHouse/backport/25.3/88605
9adfb69 
Backport #88605 to 25.3: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
@arsenmuk arsenmuk mentioned this pull request Dec 18, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davenger davenger davenger approved these changes

Assignees

@davenger davenger

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AddressSanitizer: attempting double-free in DB::ColumnVector<char8_t>::shrinkToFit

5 participants

@azat @davenger @robot-ch-test-poll @robot-ch-test-poll2 @robot-ch-test-poll3