Repository navigation
pxf, sbe: load a decode result past upb's 100-level limit - #51
Merged
Merged
Conversation
The native decoders return PB bytes, which Python loads with upb. A well-known type written as a literal is no PXF level but one PB level, so a document at the PXF limit of 100 with a Timestamp at the bottom is 101 PB levels, and upb refused it with DecodeError. protowire-go decodes it, and protowire#537 decided that it decodes. A max_nesting_depth raised past 100 hit the same wall. _upb.load replaces the five Clear + MergeFromString sites. When upb refuses for depth alone, it parses again with SetAllowOversizeProtos on, upb's only depth switch (it sets upb_DecodeOptions_MaxDepth to 65535 and nothing else, python/message.c). The switch is process-wide with no getter, so load first probes it with a 101-level message and turns it back off only if it found it off. The native decoder has already enforced the caller's limit, so this lifts only upb's second, stricter check. Fixes #40
Collaborator
Author
|
CI at |
trendvidia
pushed a commit
that referenced
this pull request
Nov 8, 2026
Four lines of the Unreleased section said what the entries after them undo: - check_decode --limit: "pb rows take none", but --format pb now takes every limit but MaxBytesLiteralLength, through protowire.pb (#53). - DatasetReader: rows parsed under the default max_bytes_literal_length "until protowire-cpp#160 is fixed". Fixed lists that fix. - the decoded message "stops at 100 levels" (#40), and a document with a literal at the depth limit "fails in upb" (#40). #51 fixed #40. - "the 16 that skip Python today": protowire#591 ran them. Measured for the new --limit wording: a limit pb has no setting for exits 2, and the same row under MaxMessageSize=1024 rejects with exit 1.
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.
Fixes #40, by the option the owner chose there (lift upb's limit for our own parse).
What changes
A PXF document 100 levels deep with a Timestamp (or any literal-written well-known type) at the bottom now decodes, as protowire-go decodes it and as trendvidia/protowire#537 decided. Before, every decoder raised upb's
DecodeErroron it. Amax_nesting_depthraised past 100 now works too. Before, it hit the same wall.The cause: the native decoder returns PB bytes and Python loads them with upb. A literal is no PXF level but one PB level, so 100 PXF levels can be 101 PB levels. upb refuses anything past 100.
The fix is a new
protowire/_upb.py, whoseloadreplaces the fivemsg.Clear(); msg.MergeFromString(raw)sites (unmarshal,unmarshal_full,DatasetReader.scan,bind_row,sbe.Codec.unmarshal). Normal parses are unchanged. When upb refuses the bytes for depth alone,loadclears the message, turns on upb'sSetAllowOversizeProtos, parses again, and restores the switch.python/message.c:1470, protobuf v36.2).loadprobes it with a 101-level message and turns it off afterwards only if it found it off.max_nesting_depth. So this lifts upb's second, stricter check on bytes we produced, not the real limit.Caveats
SetAllowOversizeProtos(True)during it may find it turned back off. This repo builds no free-threaded wheels.google._upbis private.test_the_switch_still_existsfails first if a protobuf release drops the switch.Evidence
Built from this branch into a fresh venv (protowire-cpp v2.1.0, protobuf 7.36.2, CPython 3.15, macOS arm64):
pytest tests→ 209 passed, the 200 from main plus 9 new intests/test_upb_depth.py:test_upb_alone_refuses_the_document: the native decoder's bytes for the depth-100 Timestamp document fail a plainMergeFromStringwithupb_DecodeOptions_MaxDepth. This pins the problem. If upb ever raises its own limit, this test fails and the fix can go.test_a_literal_at_the_limit_decodes(unmarshal,unmarshal_full): a chain of 100 levels,t.seconds == 1704067200.test_the_limit_itself_is_still_enforced: 101 levels →ValueError ... MaxNestingDepth=100.test_a_raised_limit_decodes_past_100: 300 levels decode withmax_nesting_depth=300; 299 refuses them.test_the_switch_is_turned_back_off,test_a_callers_switch_is_left_on,test_the_message_is_replaced_not_merged.The switch-restore test caught a bug in the first version of the probe. It built 100 levels below the root, which upb accepts, so the probe always read "on" and
loadnever turned the switch back off. The probe now builds 101 levels, and reads off → False, on → True.Not covered here: the corpus row #537 asks for (a 100-level document ending in a Timestamp,
expect: accept) doesn't exist in the spec yet. When it lands, this is what lets Python pass it.