Skip to content

Fix UB in mergeTreeAnalyzeIndexes() in case of invalid optimizations argument#101253

Merged
azat merged 2 commits intoClickHouse:masterfrom
azat:mergeTreeAnalyzeIndexes-fix-optimization-args
Mar 31, 2026
Merged

Fix UB in mergeTreeAnalyzeIndexes() in case of invalid optimizations argument#101253
azat merged 2 commits intoClickHouse:masterfrom
azat:mergeTreeAnalyzeIndexes-fix-optimization-args

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Mar 30, 2026

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 UB in mergeTreeAnalyzeIndexes() in case of invalid optimizations argument

CI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100901&sha=ea7a8e3f184a84849d198bb1a3666bf257a81f82&name_0=PR&name_1=Stress%20test%20%28arm_ubsan%29

@azat azat requested a review from shankar-iyer March 30, 2026 17:21
@azat azat mentioned this pull request Mar 30, 2026
Merged
1 task
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 30, 2026 •
edited
Loading

Workflow [PR], commit [5189a33]

Summary: ❌

job_name test_name status info comment
Integration tests (amd_llvm_coverage, 2/5) failure
test_overcommit_tracker/test.py::test_user_overcommit FAIL cidb
Stress test (amd_asan_ubsan) failure
Logical error: 'Unexpected return type from round. Expected UInt16. Got Const(UInt8)' (STID: 3262-40c8) FAIL cidb
Stress test (arm_release) failure
Logical error: Too large size (A) passed to allocator. It indicates an error. (STID: 1367-3193) FAIL cidb
Stress test (arm_msan) failure
Logical error: Shard number is greater than shard count: shard_num=A shard_count=B cluster=C (STID: 5066-564d) FAIL cidb

AI Review

Summary

This PR hardens mergeTreeAnalyzeIndexes argument parsing for optional optimization arguments, eliminating the undefined-behavior path on invalid args_array input and adding a regression test for the failure mode. The change is focused, behaviorally consistent with existing argument validation, and I did not find correctness, safety, or compatibility issues in the touched code.

ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 30, 2026
@azat azat mentioned this pull request Mar 30, 2026
Closed
@alexey-milovidov alexey-milovidov self-assigned this Mar 30, 2026
@azat azat enabled auto-merge March 30, 2026 20:17
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 30, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.20% 84.20% +0.00%
Functions 90.90% 90.90% +0.00%
Branches 76.70% 76.70% +0.00%

Changed lines: 92.00% (46/50) · Uncovered code

Full report · Diff report

@groeneai groeneai mentioned this pull request Mar 31, 2026
Merged
@azat
Copy link
Copy Markdown
Member Author

azat commented Mar 31, 2026

Integration tests (amd_llvm_coverage, 2/5)

Stress test (amd_asan_ubsan)

Stress test (arm_msan)

Stress test (arm_release)

@azat azat added this pull request to the merge queue Mar 31, 2026
Merged via the queue into ClickHouse:master with commit 18780c7 Mar 31, 2026
159 of 164 checks passed
@azat azat deleted the mergeTreeAnalyzeIndexes-fix-optimization-args branch March 31, 2026 07:22
@clickgapai
Copy link
Copy Markdown
Contributor

clickgapai commented Mar 31, 2026

Hi — this PR may need backporting to release branches 26.3, 26.2, 26.1, but no backport label was found.

Why: This fixes UB (out-of-bounds array access) in mergeTreeAnalyzeIndexes that could cause crashes on invalid input. The table function has existed since before 25.8. The fix is small and safe to backport.

If this should be backported, consider adding pr-must-backport or a version-specific label (e.g. v26.3-must-backport). Ignore this if backporting is not applicable.

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

@shankar-iyer shankar-iyer shankar-iyer approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-bugfix Pull request with bugfix, not backported by default 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.

5 participants

@azat @clickgapai @alexey-milovidov @shankar-iyer @robot-ch-test-poll4