Skip to content

Fix MessageSet recursion depth propagation in Python decoder#26710

Open
steadytao wants to merge 2 commits intoprotocolbuffers:mainfrom
steadytao:fix-26437-messageset-depth-propagation
Open

Fix MessageSet recursion depth propagation in Python decoder#26710
steadytao wants to merge 2 commits intoprotocolbuffers:mainfrom
steadytao:fix-26437-messageset-depth-propagation

Conversation

@steadytao
Copy link
Copy Markdown

Summary

Preserve recursion depth accounting when Python MessageSet decoders recurse into unknown groups.

Problem

MessageSetItemDecoder and UnknownMessageSetItemDecoder called _DecodeUnknownField(...) without forwarding current_depth.

That reset recursion accounting in the unknown-group path and allowed MessageSet parsing to bypass the intended nesting limit in this specific case.

Fix

  • forward current_depth from MessageSetItemDecoder
  • forward current_depth from UnknownMessageSetItemDecoder
  • add regression coverage for both paths

Testing

  • added regression tests for MessageSet depth propagation
  • verified the bypass path now raises DecodeError when recursion depth is already exhausted
  • verified the unknown MessageSet item path also preserves recursion depth

Fixes #26437

Preserve current_depth when MessageSet decoders recurse into unknown groups so the recursion limit is enforced consistently.

Add regression coverage for MessageSet and unknown MessageSet item depth propagation.
@steadytao steadytao requested a review from a team as a code owner April 3, 2026 10:04
@steadytao steadytao requested review from anandolee and removed request for a team April 3, 2026 10:04
@steadytao
Copy link
Copy Markdown
Author

The fork safety gate appears to be blocking CI before the actual test matrix runs. Could a protobuf maintainer please add the safe for tests label once reviewed so the forked PR checks can proceed?

Copy link
Copy Markdown
Contributor

@esrauchg esrauchg left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal!

Comment thread python/google/protobuf/internal/decoder.py Outdated
@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 6, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 6, 2026
Require explicit current_depth for UnknownMessageSetItemDecoder callers and fix the decoder test to avoid runtime-specific private fields.
@steadytao
Copy link
Copy Markdown
Author

Removed the implicit =0 from UnknownMessageSetItemDecoder and updated the fresh-entry call sites to pass 0 explicitly instead of silently resetting depth. I also fixed the failing decoder test which was reaching into a runtime-specific private field.

@steadytao steadytao requested a review from esrauchg April 6, 2026 15:22
Copy link
Copy Markdown
Contributor

@esrauchg esrauchg left a comment

Choose a reason for hiding this comment

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

Thanks!

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

Recursion Depth Limit: Can reach depth 200 via 100 nested unknown groups beneath a depth=100 messageset item

3 participants