Skip to content

pxf, sbe: load a decode result past upb's 100-level limit - #51

Merged
trendvidia merged 2 commits into
mainfrom
fix/40-upb-depth
Nov 8, 2026
Merged

trendvidia merged 2 commits into
mainfrom
fix/40-upb-depth

Conversation

@bfrancojr

Copy link
Copy Markdown
Collaborator

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 DecodeError on it. A max_nesting_depth raised 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, whose load replaces the five msg.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, load clears the message, turns on upb's SetAllowOversizeProtos, parses again, and restores the switch.

  • In upb's source the switch sets only the decode depth limit, to 65535 (python/message.c:1470, protobuf v36.2).
  • The switch is process-wide with no getter. load probes it with a 101-level message and turns it off afterwards only if it found it off.
  • protowire-cpp still enforces the caller's max_nesting_depth. So this lifts upb's second, stricter check on bytes we produced, not the real limit.

Caveats

  • Another thread's parse during the retry also gets the lifted limit. A thread calling SetAllowOversizeProtos(True) during it may find it turned back off. This repo builds no free-threaded wheels.
  • google._upb is private. test_the_switch_still_exists fails 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 in tests/test_upb_depth.py:

  • test_upb_alone_refuses_the_document: the native decoder's bytes for the depth-100 Timestamp document fail a plain MergeFromString with upb_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 with max_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 load never 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.

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
@bfrancojr

Copy link
Copy Markdown
Collaborator Author

CI at a0957a2: all 18 checks pass (Linux, macOS and Windows on py3.10–3.13, the three cibuildwheel builds, CodeQL).

@trendvidia
trendvidia merged commit 251b73c into main Nov 8, 2026
18 checks passed
@trendvidia
trendvidia deleted the fix/40-upb-depth branch November 8, 2026 06:44
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.
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.

pxf.unmarshal: a document at the depth limit fails in upb when a literal sits at the bottom

2 participants