Skip to content

Do not let stream exceptions unwind through the protobuf parser - #25

Open
groeneai wants to merge 1 commit into
ClickHouse:ClickHouse/2.3.1from
groeneai:orc-protobuf-stream-contract
Open

Do not let stream exceptions unwind through the protobuf parser#25
groeneai wants to merge 1 commit into
ClickHouse:ClickHouse/2.3.1from
groeneai:orc-protobuf-stream-contract

Conversation

@groeneai

Copy link
Copy Markdown

Reading a corrupt ORC file aborts the process in a debug or sanitizer build instead of raising a parse error:

Check failed: VerifyHasBitConsistency(msg, table) is OK
(INTERNAL: Has bits mismatch for Type=orc.proto.StripeFooter Field=1

parseProtobufFromStream hands ORC's own SeekableInputStream implementations to protobuf as a ZeroCopyInputStream. That interface reports failure by returning false, but DecompressionStream throws instead (Compression.cc:491 on a truncated stream, :589 from BackUp), so on corrupt input the exception originates inside protobuf's parse loop.

TcParser keeps has-bits in a register-cached local and writes them back only via SyncHasbits on its own exit paths, so an exception skips that write-back. The message is left with a repeated field already appended to and its has-bit still clear, which is what VerifyHasBitConsistency rejects. Since protobuf v35.1 that check also runs from the generated destructor, so tearing such a message down aborts.

This wraps the caller's stream in an adapter that catches anything escaping Next, BackUp, Skip or ByteCount, stores the first exception, and returns the value the contract defines for failure. Protobuf then finishes normally and leaves the message consistent, and the stored exception is rethrown afterwards, so callers see the original error unchanged. The CodedInputStream is destroyed before the rethrow because its destructor calls BackUp, which can throw as well.

ReadCord is deliberately not overridden: its inherited implementation reaches the wrapped stream only through Next and BackUp, which are guarded.

One header covers all nine parseProtobufFromStream call sites and every stream method protobuf may call, including streams supplied by embedders.

Validated in ClickHouse (which vendors this reader): the reproducing blob aborts with the has-bits check before the change and afterwards reports Read past EOF in DecompressionStream::readBuffer as a normal parse error, byte-identical to what a release build already prints. A 183-case corruption sweep and the ORC test suite show no behaviour change.

parseProtobufFromStream hands ORC's own SeekableInputStream implementations to
protobuf as a ZeroCopyInputStream. That interface reports "no more data" by
returning false, but DecompressionStream throws instead (Compression.cc:491 for
a truncated stream, :589 from BackUp), so on corrupt input the exception
originates inside protobuf's parse loop.

TcParser keeps has-bits in a register-cached local and writes them back only via
SyncHasbits on its own exit paths, so an exception skips that write-back. The
message is then left with a repeated field already appended to and its has-bit
still clear, which is exactly what VerifyHasBitConsistency rejects. Since
protobuf v35.1 that check also runs from the generated destructor, so tearing
down such a message aborts the process in a debug or sanitizer build:

  Check failed: VerifyHasBitConsistency(msg, table) is OK
  (INTERNAL: Has bits mismatch for Type=orc.proto.StripeFooter Field=1

Wrap the caller's stream in an adapter that catches anything escaping Next,
BackUp, Skip or ByteCount, stores the first exception, and returns the value the
contract defines for failure. Protobuf then finishes normally and leaves the
message consistent, and the stored exception is rethrown afterwards, so callers
see the original error unchanged. The CodedInputStream is destroyed before the
rethrow because its destructor calls BackUp, which can throw as well.

This covers all nine parseProtobufFromStream call sites and every stream method
protobuf may call, including streams supplied by embedders.
@github-actions github-actions Bot added the CPP label Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant