Skip to content

upb: validate message tags before consuming depth - #29514

Closed
srkyn wants to merge 2 commits into
protocolbuffers:mainfrom
srkyn:research/fasttable-tag-depth-order
Closed

srkyn wants to merge 2 commits into
protocolbuffers:mainfrom
srkyn:research/fasttable-tag-depth-order

Conversation

@srkyn

@srkyn srkyn commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Validate a linked-message fasttable slot against the actual tag before entering the message path.
  • Keep mismatched tags on the existing fallback path.
  • Add a regression test for a valid unknown field at an exact recursion-depth boundary.

Without this ordering, the fast decoder can return MaxDepthExceeded for a valid message that the generic decoder accepts because it consumes depth before discovering that the tag does not match the message slot.

Testing

  • DecodeTest.MismatchedMessageSlotDoesNotConsumeDepth passes with the fast and generic decoders.
  • Full sanitized upb-test: 535 passed, 1 existing skip.
  • Patched current-main differential fuzzing: 1,000,000 executions with no decoder mismatch or sanitizer finding.
  • Patched v36 differential fuzzing: 3,000,000 executions with no decoder mismatch or sanitizer finding.

Check that a linked message fasttable slot matches the actual tag before looking up the subtable or entering the message path. This keeps mismatched tags on the fallback path and avoids rejecting valid unknown fields at an exact recursion boundary.

Signed-off-by: David Sarkisyan <[email protected]>
@srkyn

srkyn commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

The fork CI log says a protobuf team member needs to review this change and add safe for tests before the tests can run. I have not tried to bypass that gate. Could someone review the change and mark it safe if appropriate? The PR description includes the local regression, sanitizer, and differential-fuzz results.

@sbenzaquen sbenzaquen added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2026
@sbenzaquen
sbenzaquen requested a review from haberman October 13, 2026 19:47
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2026
@srkyn

srkyn commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I merged current main into the branch and reran the decoder tests locally. The focused regression and all 203 tests in //upb/wire:decode_test pass with fasttable enabled. The new commit needs a fresh 🅰️ safe for tests label before fork CI can run. Could a protobuf team member reapply it when convenient?

@karenwuz karenwuz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 20, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 20, 2026
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.

4 participants