Skip to content

Add recursion depth limit to TProtocol.skip() in Python#3411

Merged
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:py-skip-depth
Apr 22, 2026
Merged

Add recursion depth limit to TProtocol.skip() in Python#3411
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:py-skip-depth

Conversation

@Jens-G
Copy link
Copy Markdown
Member

@Jens-G Jens-G commented Apr 22, 2026

Summary

TProtocolBase.skip() in Python had no recursion depth limit. When deserializing an unknown field, generated code calls skip(), which recursively calls itself for each nested container or struct element. C++, Go, and Node.js all cap this recursion at 64 levels and raise a protocol exception when the limit is exceeded.

This change brings Python into parity:

  • lib/py/src/protocol/TProtocol.py: skip() gains an optional max_depth parameter (default 64). At the start of each call, if max_depth <= 0 a TProtocolException(DEPTH_LIMIT) is raised. All recursive calls pass max_depth - 1.

Tests added in lib/py/test/thrift_TBinaryProtocol.py:

  • test_skip_rejects_deeply_nested_struct: a payload with 64 levels of nesting raises DEPTH_LIMIT.
  • test_skip_accepts_struct_within_depth_limit: a payload with 63 levels succeeds.

Test plan

  • python3 lib/py/test/thrift_TBinaryProtocol.py -v — two new skip depth tests pass

🤖 Generated with Claude Code

Client: py

C++ and Go already enforce a limit of 64 recursive skip() calls via
TInputRecursionTracker and maxDepth respectively. This change adds an
equivalent max_depth=64 parameter to the Python skip() implementation
in TProtocolBase, raising TProtocolException(DEPTH_LIMIT) when the
limit is exceeded.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Jens-G Jens-G requested a review from mhlakhani as a code owner April 22, 2026 00:29
@mergeable mergeable Bot added the python label Apr 22, 2026
@Jens-G Jens-G merged commit e00fb18 into apache:master Apr 22, 2026
86 checks passed
@Jens-G Jens-G deleted the py-skip-depth branch April 23, 2026 21:38
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.

2 participants