Skip to content

fix(thrift): bound skip() recursion depth to prevent stack overflow#40

Open
moshap-firebolt wants to merge 1 commit into
release-24.0.0from
fix/thrift-skip-recursion-depth-limit
Open

fix(thrift): bound skip() recursion depth to prevent stack overflow#40
moshap-firebolt wants to merge 1 commit into
release-24.0.0from
fix/thrift-skip-recursion-depth-limit

Conversation

@moshap-firebolt

Copy link
Copy Markdown

Background

The storage fuzzer (griffin_parquet_column) found a stack overflow parsing a crafted Parquet footer: packdb run 29895460517.

This vendored Thrift copy stubbed Apache Thrift's recursion-depth trackers as no-ops (see the comment above TInputRecursionTracker), so the generic apache::thrift::protocol::skip() helper recurses without limit. The generated Parquet deserializer routes every unknown field through iprot->skip(ftype) (229 sites), so a deeply nested unknown struct/list/set/map field drives unbounded recursion and overflows the stack. A stack overflow is a signal, not a C++ exception, so the reader's try/catch around ParquetFileReader::Open cannot contain it.

Summary

Restore the upstream recursion guard in skip(): thread a depth_limit (defaulting to TConfiguration::DEFAULT_RECURSION_DEPTH = 64) and throw TProtocolException::DEPTH_LIMIT when it is exhausted. The two-argument skip(prot, type) overload is preserved for existing callers.

Test Plan

  • Standalone repro against this vendored Thrift (clang-22 + ASAN) feeding N nested-struct compact-protocol bytes through skip(T_STRUCT):
    • Before: AddressSanitizer: stack-overflow in the skip recursion (matches the CI stack trace).
    • After: throws TProtocolException type=6 (DEPTH_LIMIT); shallow/valid inputs parse unchanged.
  • The originating CI crash artifact is being committed as a griffin-fuzzer regression seed in packdb.

🤖 Generated with Claude Code

@moshap-firebolt
moshap-firebolt changed the base branch from release-24.0.0 to kop/fb-1819-thrift-libcxx-operator-eq July 23, 2026 00:52
lorenzhs
lorenzhs previously approved these changes Jul 23, 2026

@lorenzhs lorenzhs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) this change looks rather different than the removal, why? https://24d7106e4280c32d65165cd052b8309fd06478b7

b) what's with the random base branch? Why are we not using release-24.0? Please fix

Base automatically changed from kop/fb-1819-thrift-libcxx-operator-eq to release-24.0.0 July 23, 2026 08:21
@lorenzhs
lorenzhs dismissed their stale review July 23, 2026 08:21

The base branch was changed.

@lorenzhs

Copy link
Copy Markdown
Collaborator

merged the base branch PR

@lorenzhs
lorenzhs force-pushed the fix/thrift-skip-recursion-depth-limit branch from 822adc2 to 00e2a70 Compare July 23, 2026 08:49
…erflow

Commit apache#86 ("bare bones Thrift") removed the recursion-depth tracker, leaving
the generic skip() helper — and the generated Parquet deserializer, which
routes every unknown field through skip(ftype) — able to recurse without
limit. A crafted Parquet footer with deeply nested unknown Thrift fields
overflows the stack (SIGSEGV, uncatchable by the reader's try/catch). Found by
the griffin_parquet_column storage fuzzer.

Restore the mechanism that apache#86 dropped rather than reimplement it: make
TInputRecursionTracker / TOutputRecursionTracker real (templated on the
protocol, since there is no longer a common TProtocol base) and give
TCompactProtocolT the input/output depth counters + limit (from the
transport's TConfiguration, default 64) they drive, throwing
TProtocolException::DEPTH_LIMIT when exceeded. skip() reverts to its original
form with a TInputRecursionTracker at the top; the tracker instantiations
already present in the generated parquet_types.tcc are now effective again too.
@moshap-firebolt
moshap-firebolt force-pushed the fix/thrift-skip-recursion-depth-limit branch from 00e2a70 to 8b42b44 Compare July 23, 2026 14:09
@moshap-firebolt

Copy link
Copy Markdown
Author

Thanks @lorenzhs — both addressed:

(a) Good call. Rewrote it to restore the mechanism apache#86 removed rather than reimplement it: TInputRecursionTracker/TOutputRecursionTracker are real RAII guards again (templated on the protocol now, since apache#86 deleted the common TProtocol base), and TCompactProtocolT regains the input/output depth counters + limit (from the transport's TConfiguration, default 64). skip() reverts to its original form — just TInputRecursionTracker tracker(prot); at the top, no signature change. The 10 tracker instantiations already in the generated parquet_types.tcc are effective again too, so the generated read/write paths are guarded, not just skip().

(b) Already handled by your rebase onto release-24.0.0 — thanks. Force-pushed my new commit on the same parent, so the branch is a single commit on release-24.0.0.

Verified with a clang-22 + ASAN repro (incl. the generated-code-style CTAD call site): deep nested input throws DEPTH_LIMIT; before the fix it was an ASAN stack-overflow matching the CI trace; valid input unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants