Skip to content

Bump orc to reject an out-of-range stream column in a stripe footer - #123464

Merged
PedroTadim merged 3 commits into
masterfrom
fix-orc
Oct 2, 2026
Merged

PedroTadim merged 3 commits into
masterfrom
fix-orc

Conversation

@PedroTadim

@PedroTadim PedroTadim commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

A stripe footer can declare a stream for a column id that is not in the file's type tree. orc::ReaderImpl::preBuffer indexed the selected-column mask with that id, which failed the libc++ hardening assertion and aborted the server. Stripes are prebuffered only for remote reads, so the crash needed s3 or a data lake table function; file and url read the same file normally. The library now throws orc::ParseError.

The test reads a 569-byte ORC file, embedded in the test, whose stripe footer declares a stream for column 20 in a file with 5 columns, through s3.

Related: ClickHouse/orc#29
Related: ClickHouse/orc#31

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 access when reading an ORC file.

Closes: #123450


Workflow [PR]
Sync PR [sync-upstream/pr/123464]

Version info

  • Merged into: 26.10.1.1345-master (included in 26.10 and later)

A stripe footer can declare a stream for a column id that is not in the file's type tree.
`orc::ReaderImpl::preBuffer` indexed the selected-column mask with that id, which failed the libc++
hardening assertion and aborted the server. Stripes are prebuffered only for remote reads, so the crash
needed `s3` or a data lake table function; `file` and `url` read the same file normally. The library now
throws `orc::ParseError`.

The test reads a 569-byte ORC file, embedded in the test, whose stripe footer declares a stream for
column 20 in a file with 5 columns, through `s3`.

Related: ClickHouse/orc#29

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [dc70abd]

Summary: ❌

job_name test_name status info comment
Performance Comparison (arm_release, master_head, 2/6) FAIL Performance dashboard
logical_functions_small #10 FAIL dashboard IGNORED
promql_set_operator_presence_mask #1 unstable query history IGNORED

AI Review

Summary

This PR bumps contrib/orc and adds a focused stateless regression so malformed stripe-footer stream ids raise ParseError instead of aborting the server on the remote prefetch and predicate-pushdown paths. The ClickHouse-visible crash path from the linked issue looks fixed, but one public ORC reader helper still accepts the same malformed footer, so I can't fully approve the broader “reject an out-of-range stream column in a stripe footer” contract yet.

Findings

⚠️ Majors

  • [contrib/orc:1] ReaderImpl::getRowGroupIndex still materializes ret[column] from stream.column() without validating that the stream belongs to the file's type tree (c++/src/Reader.cc:1805-1828 in the bumped submodule). A malformed ROW_INDEX stream for column 20 in a 5-column file is therefore still observable through the public row-group-index API instead of failing with ParseError, which leaves one stripe-footer consumer inconsistent with the new preBuffer / loadStripeIndex hardening.
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required actions:
    • Harden ReaderImpl::getRowGroupIndex against out-of-range stream.column() values so this malformed footer is rejected consistently across the reader helpers.

LLVM Coverage Report

Measured on commit dc70abd.

Metric Baseline Current Δ
Lines 88.10% 88.10% +0.00%
Functions 91.80% 91.80% +0.00%
Branches 79.40% 79.50% +0.10%

Changed lines: Uncovered code analysis did not run: No coverable C/C++ source files changed (contrib/ is excluded from coverage).

Newly covered: +187 lines in 56 files (-140 lines lost coverage) · Details

Full report

@clickhouse-gh clickhouse-gh Bot added pr-bugfix Pull request with bugfix, not backported by default submodule changed At least one submodule changed in this PR. comp-external-dependencies Third-party deps updates in contrib/, vendored code, and platform base libraries. labels Oct 2, 2026
…oter_stream_column`

The ORC reader prebuffers stripes only when `remote_filesystem_read_prefetch` is on, and CI
randomizes it. With it off the corrupt stream column is never reached on the prebuffer path, so
the expected error was missing.

https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=123464&sha=748a716418ce7ac4cba4adb758efb40395ba229e&name_0=PR

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Predicate pushdown loads the row indexes through `RowReaderImpl::loadStripeIndex`, which indexed the
selected-column mask with the stream column from the stripe footer without a bounds check, also for local
files that are never prebuffered. Cover it in `05317_orc_corrupt_stripe_footer_stream_column` with a local
`file()` read and `WHERE id >= 0`.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread contrib/orc
@PedroTadim
PedroTadim enabled auto-merge October 2, 2026 14:54
@clickhouse-gh

clickhouse-gh Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing dc70abd85 with master d046101ce (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

7 translation units recompiled, 10 s compile time in total, 7 of them have a recent master baseline.

Job report

@PedroTadim
PedroTadim added this pull request to the merge queue Oct 2, 2026
Merged via the queue into master with commit bfc1f59 Oct 2, 2026
147 of 149 checks passed
@PedroTadim
PedroTadim deleted the fix-orc branch October 2, 2026 22:55
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 2, 2026
clickgapai pushed a commit to clickgapai/ClickHouse that referenced this pull request Oct 3, 2026
…e log scan

Stress worker 1 (`--database=test_1`) runs with
`memory_tracker_fault_probability=0.05`. Work that the stress phase leaves
behind keeps the settings of the query that created it and is executed again
by the upgraded server after the restart:

- a distributed DDL entry stores the initiator's changed settings
  (`DDLLogEntry`) and `DDLTaskBase::makeQueryContext` applies them, so a
  queued `BACKUP ... ON CLUSTER` of `test_1` replays with fault injection and
  `BackupsWorker` logs `Failed to make internal backup ... fault injected` at
  `<Error>`;
- a pending `Distributed` batch is sent with its stored settings, and the
  receiving async-insert flush logs `AsynchronousInsertQueue: Failed insertion
  ... fault injected` at `<Error>` with an empty query id, so the existing
  `} <Error> executeQuery: Code:` entry does not cover it.

Seen on three unrelated PRs in 90 days (ClickHouse#123464, ClickHouse#117943, ClickHouse#120725); in each job
these lines were the only output of the scan. Nothing enables fault injection
on the upgraded server itself, so the message can only come from carried-over
stress work. Ignore it as a fixed string, the same string the stress smoke
check already tolerates (`ci/jobs/scripts/stress/stress.py`). It is
MemoryTracker's message for both injection sites and does not match a real
`memory limit exceeded` error.

The large DDL backlog in the ClickHouse#123464 job comes from the previous release
(26.9) wedging its DDLWorker on `KILL PART_MOVE_TO_SHARD ... ON CLUSTER`,
which ClickHouse#122132 fixed on master only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-external-dependencies Third-party deps updates in contrib/, vendored code, and platform base libraries. pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A 569-byte ORC file read from object storage aborts the server (libc++ hardening, orc::extractReadRangesForStripe)

2 participants