Do not let stream exceptions unwind through the protobuf parser - #25
Open
groeneai wants to merge 1 commit into
Open
Do not let stream exceptions unwind through the protobuf parser#25groeneai wants to merge 1 commit into
groeneai wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reading a corrupt ORC file aborts the process in a debug or sanitizer build instead of raising a parse error:
parseProtobufFromStreamhands ORC's ownSeekableInputStreamimplementations to protobuf as aZeroCopyInputStream. That interface reports failure by returningfalse, butDecompressionStreamthrows instead (Compression.cc:491on a truncated stream,:589fromBackUp), so on corrupt input the exception originates inside protobuf's parse loop.TcParserkeeps has-bits in a register-cached local and writes them back only viaSyncHasbitson 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 whatVerifyHasBitConsistencyrejects. 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,SkiporByteCount, 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. TheCodedInputStreamis destroyed before the rethrow because its destructor callsBackUp, which can throw as well.ReadCordis deliberately not overridden: its inherited implementation reaches the wrapped stream only throughNextandBackUp, which are guarded.One header covers all nine
parseProtobufFromStreamcall 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::readBufferas 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.