iframe-proxy

groeneai · 2026-07-02T13:28:36Z

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):

Fixed a possible out-of-bounds write in ColumnString::filter when a String column arrives with offsets inconsistent with its chars array (for example offsets.back() not equal to chars.size()). Such a column now raises an INCORRECT_DATA exception instead of reading and writing out of bounds.

Description

ColumnString::filter (both the const and in-place overloads) passed offsets and chars straight to filterArraysImpl / filterArraysImplInPlace without checking that they are consistent. The SIMD fast path in filterArraysImplGeneric (ColumnsCommon.cpp) trusts offsets to size its copies: for a full-passing 64-row chunk it computes chunk_size = offsets_pos[63] - offsets_pos[-1], then does resize + memcpy of that many bytes. If a corrupt column arrives with offsets.back() != chars.size(), chunk_size is bogus and the memcpy runs off the end of the buffer, an out-of-bounds write.

This was observed in CI as a SEGV ("address not mapped to object") with the stack:

__asan_memcpy
ColumnsCommon.cpp  ResultOffsetsBuilder::insertChunk<64> / filterArraysImpl<char8_t>
ColumnString.cpp   ColumnString::filter
ColumnNullable.cpp ColumnNullable::filter
FilterDescription.cpp FilterDescription::filter
FilterTransform.cpp   FilterTransform::doTransform

while filtering a nullable String column produced by the Parquet reader on a metadata-cache-hit path. The inconsistent column is produced upstream and the exact producer path is not deterministically reproducible, so this change does not touch the upstream producer. Instead it guards the point where the corruption turns into memory unsafety: ColumnString::filter now validates the same invariant the ColumnString copy constructor already enforces (offsets.back() == chars.size()), matching the guard recently added to the sibling ColumnString::insertRangeFrom path (#107196). A violated invariant becomes a clean INCORRECT_DATA exception, which is handled normally in release builds, rather than a silent out-of-bounds write.

The guard is O(1) (a single comparison on the already-loaded last offset), so there is no hot-path cost for well-formed columns.

Unit tests in gtest_column_string.cpp build a ColumnString whose offsets overshoot chars and assert that filter() (both overloads) throws INCORRECT_DATA instead of reading/writing out of bounds. Without the guard, the large-overshoot case is an AddressSanitizer heap-buffer-overflow. A positive test confirms a consistent column still filters correctly over both the SIMD and the tail paths.

No related open issue found.

ColumnString::filter (both the const and in-place overloads) handed `offsets` and `chars` straight to filterArraysImpl / filterArraysImplInPlace without checking that they are consistent. The SIMD fast path in filterArraysImplGeneric (ColumnsCommon.cpp) trusts `offsets` to compute chunk sizes: for a full-passing 64-row chunk it does chunk_size = offsets_pos[63] - offsets_pos[-1], then resize + memcpy that many bytes. If a corrupt column arrives with offsets.back() != chars.size() (offsets overshoot chars, or non-monotonic), chunk_size is bogus and the memcpy runs off the end of the destination/source buffer, an out-of-bounds write. Observed in CI as a SEGV ("address not mapped") with the stack __asan_memcpy ColumnsCommon.cpp ResultOffsetsBuilder::insertChunk<64> / filterArraysImpl<char8_t> ColumnString::filter -> ColumnNullable::filter -> FilterDescription::filter -> FilterTransform::doTransform when filtering a nullable String column produced by the parquet reader on a metadata-cache-hit path. The corrupt column is produced upstream; this change does not fix the upstream producer, it converts the silent out-of-bounds write into a clean INCORRECT_DATA exception so the corruption fails safely and is diagnosable instead of crashing (or silently corrupting memory) in release builds. This mirrors the invariant the ColumnString copy constructor already enforces (offsets.back() == chars.size()) and the guard recently added to the sibling ColumnString::insertRangeFrom path. Adds unit tests that build a ColumnString whose offsets overshoot chars and assert filter() throws INCORRECT_DATA instead of reading/writing out of bounds (the large-overshoot case is an ASan heap-buffer-overflow without the guard), plus a positive test that a consistent column still filters correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-02T13:29:10Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes for the crash mechanism: a unit test builds a ColumnString whose offsets overshoot chars and calls filter(); the large-overshoot case is a deterministic ASan heap-buffer-overflow without the guard. The upstream Parquet metadata-cache producer that yields the inconsistent column is NOT deterministically reproducible (ran the exact deltaLake reproducer 60x over the network, 0 crashes), so per the "no speculative changes to code you cannot reproduce" rule this PR does not touch the producer; it guards + tests the manifestation point.
b	Root cause explained?	filterArraysImplGeneric SIMD fast path computes chunk_size = offsets_pos[63] - offsets_pos[-1] and memcpys that many bytes. If offsets.back() != chars.size() (offsets overshoot chars, or non-monotonic), chunk_size is bogus and the resize+memcpy writes past the buffer end. ColumnString::filter passed offsets/chars in with no consistency check.
c	Fix matches root cause?	Yes. The guard validates the exact invariant the fast path relies on (offsets.back() == chars.size(), the same invariant the ColumnString copy constructor enforces) before calling filterArraysImpl, converting the OOB write into a clean INCORRECT_DATA exception.
d	Test intent preserved / new tests added?	New tests added: FilterInconsistentOffsetsThrows, FilterInconsistentOffsetsLargeOvershoot, FilterInPlaceInconsistentOffsetsThrows (assert INCORRECT_DATA), plus FilterConsistentColumnStillWorks (positive path, SIMD + tail). Modeled on the existing insertRangeFrom guard tests. No existing test weakened.
e	Both directions demonstrated?	Yes. Without the guard: FilterInconsistentOffsetsLargeOvershoot aborts with AddressSanitizer heap-buffer-overflow; the other two "Throws" tests FAIL ("filter did not detect..."). With the guard: all 11 ColumnString tests pass. Full deltaLake reproducer still returns correct results (99997497 rows) with no false-positive on the guarded binary.
f	Fix is general across code paths?	Both ColumnString::filter overloads (const via filterArraysImpl, in-place via filterArraysImplInPlace) are guarded. The sibling insertRangeFrom path already got the equivalent guard in #107196; filter was the remaining unguarded consumer of the same SIMD offsets math. ColumnArray::filter uses the same helper but with element-offset (not byte/chars) semantics, so the chars-specific invariant does not apply there.
g	Fix generalizes across inputs?	The guard is on the structural invariant itself (last offset vs chars size), independent of the string values, Nullable/LowCardinality wrapping (those wrap ColumnString, whose filter is called on the inner column), filter selectivity, or result_size_hint. Covered by tests with >= 64 rows (SIMD path), small and large (256 KB) overshoot, and a selective positive filter.
h	Backward compatible?	Yes. No setting, on-disk, wire, or metadata format change. The only behavior change is on an already-corrupt (invariant-violating) column, which previously invoked undefined behavior (OOB write) and now raises a catchable INCORRECT_DATA exception. Well-formed columns are unaffected (O(1) check on the error path only).
i	Invariants and contracts preserved?	Yes. The guard enforces the documented ColumnString invariant offsets.back() == chars.size() (the copy constructor already throws on its violation) before filterArraysImpl consumes offsets. It adds no new state and holds on all paths (early-return on empty offsets is unchanged; the check runs before any allocation/copy).

Session id: cron:clickhouse-worker-slot-5:20260702-124200

groeneai · 2026-07-02T13:29:39Z

cc @Avogar @CurtizJ @davenger could you review this? It adds an O(1) consistency guard to ColumnString::filter (both overloads) that rejects a column whose offsets.back() != chars.size() with INCORRECT_DATA, instead of letting the filterArraysImpl SIMD fast path do an out-of-bounds memcpy (seen in CI as an ASan SEGV under FilterTransform on a nullable String from the Parquet reader). It mirrors the invariant the copy constructor enforces and the sibling guard added to insertRangeFrom in #107196.

clickhouse-gh · 2026-07-02T13:39:11Z

Workflow [PR], commit [7ce9c3a]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Integration tests (amd_msan, 8/8)		FAIL
	test_keeper_session_refuse_stale_server/test.py::test_session_refused_on_stale_keeper	FAIL	cidb

AI Review

Summary

This PR adds invariant checks in both ColumnString::filter overloads and regression tests for the offsets.back() != chars.size() corruption case. The direction is correct, but the guard is incomplete: filterArraysImpl also assumes the offsets are monotonic, so a corrupted ColumnString with a valid final offset can still underflow arr_size and hit the same out-of-bounds resize/memcpy path.

Findings

❌ Blockers

[src/Columns/ColumnString.cpp:208] filterArraysImpl and filterArraysImplInPlace subtract each offset from the previous one, so offsets.back() == chars.size() is not enough to make the data safe. A column like offsets = [4, 2, 12] passes the new guard, but filtering the second row computes arr_size = 2 - 4, underflows, and drives a huge resize/memcpy in ColumnsCommon.cpp. Because the upstream corruption source is still unknown, ColumnString::filter needs to reject non-monotonic offsets here as well, and the tests should cover the decreasing/dipping cases the same way insertRangeFrom does.

Final Verdict

❌ Changes requested.

clickhouse-gh · 2026-07-02T13:56:51Z

+    /// Enforce the ColumnString invariant (offsets.back() == chars.size(), same as the copy
+    /// constructor) before filterArraysImpl trusts `offsets` to size its memcpys; a violated
+    /// invariant would otherwise read/write out of bounds instead of failing cleanly.
+    if (offsets.back() != chars.size())


filterArraysImpl and filterArraysImplInPlace rely on more than offsets.back() == chars.size(): copy_array subtracts every offset from the previous one. A corrupt column like offsets = [4, 2, 12] still passes this new check, but filtering the second row makes arr_size = 2 - 4, which underflows and drives a huge resize/memcpy in both paths. Since the upstream producer is still unknown, the guard here needs to reject non-monotonic offsets too, and the new tests should cover the decreasing/dipping cases the same way insertRangeFrom already does.

clickhouse-gh · 2026-07-02T16:56:14Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.50%	85.50%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.70%	77.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 52/53 (98.11%) · Uncovered code

Full report · Diff report

groeneai · 2026-07-02T17:46:50Z

CI finish ledger — `7ce9c3a`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Integration tests (amd_msan, 8/8) / test_keeper_session_refuse_stale_server::test_session_refused_on_stale_keeper	flaky (wait_zookeeper_to_start ZK-restart race; 30d: 6 hits / 6 unrelated PRs / 0 master; unrelated to this PR's ColumnString::filter guard)	#109037 (ours, open)

Note: Mergeable Check / PR are rollups of the single flaky Integration line above, owned by #109037.

Session id: cron:our-pr-ci-monitor:20260702-173000

groeneai · 2026-07-03T05:03:23Z

CI finish ledger — `7ce9c3a`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Session id: cron:our-pr-ci-monitor:20260703-043000

PedroTadim added the can be tested Allows running workflows for external contributors label Jul 2, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 2, 2026

clickhouse-gh Bot reviewed Jul 2, 2026

View reviewed changes

groeneai mentioned this pull request Jul 2, 2026

Guard ColumnArray::getExtremes against offsets inconsistent with nested column #109212

Open

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Guard ColumnString::filter against offsets inconsistent with chars array#109184

Guard ColumnString::filter against offsets inconsistent with chars array#109184
groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:fix-columnstring-filter-oob-inconsistent-offsets

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot Jul 2, 2026

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jul 2, 2026

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Final Verdict

Uh oh!

clickhouse-gh Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 2, 2026

CI finish ledger — 7ce9c3a

Uh oh!

groeneai commented Jul 3, 2026

CI finish ledger — 7ce9c3a

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clickhouse-gh Bot commented Jul 2, 2026 •

edited

Loading

CI finish ledger — `7ce9c3a`

CI finish ledger — `7ce9c3a`