Skip to content

text_format: enforce recursion limit for Any bracket syntax#26196

Open
1seal wants to merge 3 commits intoprotocolbuffers:mainfrom
1seal:codex/textformat-any-recursion-limit
Open

text_format: enforce recursion limit for Any bracket syntax#26196
1seal wants to merge 3 commits intoprotocolbuffers:mainfrom
1seal:codex/textformat-any-recursion-limit

Conversation

@1seal
Copy link

@1seal 1seal commented Mar 3, 2026

this makes TextFormat::Parser::SetRecursionLimit() apply to google.protobuf.Any bracket syntax ([type.googleapis.com/...]{...}) by decrementing/ restoring recursion_limit_ around ConsumeAnyValue(), matching the normal nested-message paths.\n\nadds a regression test: TextFormatParserTest.SetRecursionLimitAnyBracketSyntax.\n\nfixes: #26195

Copy link
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 contribution!

initial_recursion_limit_, "."));
return false;
}
const bool any_ok = ConsumeAnyValue(value_descriptor, &serialized_value);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave this as DO() and not interweave the return and recursion limit reset.

I can see its the preexisting pattern we don't restore the recursion limit in ConsumeFieldMessage() or SkipFieldMessage(), once we've failed there's not resuming and so unwinding the recursion limit isn't needed.

@1seal 1seal force-pushed the codex/textformat-any-recursion-limit branch from c49f44d to 3c4d3e7 Compare March 5, 2026 19:46
@1seal 1seal force-pushed the codex/textformat-any-recursion-limit branch from 3c4d3e7 to d114a51 Compare March 5, 2026 19:52
@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 5, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 5, 2026
@esrauchg
Copy link
Contributor

esrauchg commented Mar 5, 2026

I'm clicking the rebase button in the github UI to unblock the cI

(Writing the comment to avoid any confusion; it has surprised folks in the past that I can update their branch, its a github feature where the branch can be set to allow the upstream maintainers to make commits exactly for this reason)

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 5, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 5, 2026
@esrauchg
Copy link
Contributor

esrauchg commented Mar 6, 2026

Unfortunately looks like the latest snapshot regressed the intended behavior (the newly added unit test now fails)

@1seal
Copy link
Author

1seal commented Mar 6, 2026

i tracked the post-rebase failure down to the new test expectation rather than the recursion-limit change itself. pushed a follow-up fix adjusting the reported error location for this case.

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.

2 participants