Repository navigation
[ObjC] Carry recursion depth across MessageSet item parsing - #27571
Closed
tonghuaroot wants to merge 1 commit into
Closed
tonghuaroot wants to merge 1 commit into
tonghuaroot wants to merge 1 commit into
Conversation
GPBMessage::parseMessageSet: allocates a fresh GPBCodedInputStream for each MessageSet item's payload because the type-id and payload tags are read out of order. The new stream started at recursionDepth=0 even when the parent stream was already several MessageSet items deep, so a chain of nested MessageSet items could grow the native call stack arbitrarily without ever tripping the documented kDefaultRecursionLimit of 100. Add a -initWithData:parentRecursionDepth: initializer that seeds the child stream's depth from the parent and checks the limit before parsing begins. Use it in parseMessageSet: so MessageSet item parsing inherits the parent's depth, matching the depth-inheritance convention used by the C++ ParseContext spawn helper. The existing testErrorRecursionDepthReached covers ordinary nested message recursion via a single stream; this change keeps that path unchanged and adds testParseMessageSetRecursionDepthCarriedFromParent to GPBWireFormatTests covering the cross-stream case. Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
tonyliaoss
force-pushed
the
harden/objc-messageset-recursion-depth
branch
from
July 7, 2026 17:03
c950e14 to
6969693
Compare
tonyliaoss
added
the
🅰️ safe for tests
github-actions
Bot
removed
the
🅰️ safe for tests
thomasvl
reviewed
Jul 7, 2026
thomasvl
left a comment
thomasvl
left a comment
Contributor
There was a problem hiding this comment.
Apologies for the delay in looking.
| // which increments and checks the depth before recursing into a payload | ||
| // that has been read into a fresh buffer. | ||
| state_.recursionDepth = parentDepth + 1; | ||
| CheckRecursionLimit(&state_); |
Contributor
There was a problem hiding this comment.
I think we've got a leak here. If this goes over the budget, this will throw, That means self doesn't get returned and nothing does the release. So this flow needs to be revised to avoid that leak case. Either by doing the release here (some way), or deferring where the check is done to some place that allows the initialization to happen first.
| // kDefaultRecursionLimit (100) to actually apply. A depth well past 100 | ||
| // must be rejected with GPBCodedInputStreamErrorRecursionDepthExceeded | ||
| // rather than silently parsed. | ||
| const NSUInteger kDepth = 200; |
Contributor
There was a problem hiding this comment.
It's really easy to get the recursion calculations slightly wrong, so there should be a test at 100 (kDefaultRecusionLimit), that passes; and the one at 101 (default + 1) that fails, to ensure the depth is being computed correctly.
copybara-service Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes #27571 PiperOrigin-RevId: 957331709
copybara-service Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes #27571 PiperOrigin-RevId: 957331709
copybara-service Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes #27571 PiperOrigin-RevId: 957331709
github-actions
Bot
added
untriaged
copybara-service Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes #27571 PiperOrigin-RevId: 957331709
copybara-service Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes #27571 PiperOrigin-RevId: 957331709
Member
|
To move this along, we fixed this at dd9d2ad. |
tonyliaoss
added a commit
that referenced
this pull request
Aug 4, 2026
…pick [ObjC] Carry recursion depth across MessageSet item parsing (#27571)
wheakerd
pushed a commit
to wheakerd/protobuf
that referenced
this pull request
Aug 5, 2026
…buffers#27571) ## Summary `GPBMessage::parseMessageSet:` reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a fresh `GPBCodedInputStream` to decode that buffer. The fresh stream's `state_.recursionDepth` starts at `0` even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documented `kDefaultRecursionLimit` (100). Other parser entry points that recurse on the same stream (`readGroup:`, `readMessage:`, `readMapEntry:`, `SkipToEndGroupInternal`) already increment and check `recursionDepth`; the MessageSet path was the only spot where depth tracking did not cross the stream boundary. This change adds a package-private `-initWithData:parentRecursionDepth:` initializer to `GPBCodedInputStream` that seeds the child stream's depth from the parent's and runs `CheckRecursionLimit` before parsing begins. To prevent memory leaks if `CheckRecursionLimit` raises an exception during initialization, the check is wrapped in a `@try/@catch` block that releases `self`. `parseMessageSet:` uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ `ParseContext` spawn helper. Regression coverage: `testParseMessageSetRecursionDepthCarriedFromParent` in `GPBWireFormatTests` builds MessageSet-of-MessageSet payloads (each layer is `MSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...`, adding 2 to the recursion depth per layer). It verifies that 50 layers (depth 100, `kDefaultRecursionLimit`) parses successfully, while 51 layers (depth 101, `kDefaultRecursionLimit + 1`) fails with `GPBCodedInputStreamErrorRecursionDepthExceeded`. Existing `testErrorRecursionDepthReached` and the rest of `GPBWireFormatTests` continue to pass unchanged. ## Test plan - Existing ObjC test suite passes (`GPBWireFormatTests`, `GPBMessageTests+Serialization`, `GPBCodedInputStreamTests`, `GPBUnknownFieldsTest`). - New `testParseMessageSetRecursionDepthCarriedFromParent` passes for both 50 layers (passing at kDefaultRecursionLimit) and 51 layers (failing at kDefaultRecursionLimit + 1). Closes protocolbuffers#27571 PiperOrigin-RevId: 959041690
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.
Summary
GPBMessage::parseMessageSet:reads each MessageSet item's payload bytes into a separate buffer (because the type-id and payload tags can appear in any order), then allocates a freshGPBCodedInputStreamto decode that buffer. The fresh stream'sstate_.recursionDepthstarts at0even when the parent stream is already several MessageSet items deep, so a chain of nested MessageSet items grows the native call stack without ever tripping the documentedkDefaultRecursionLimit(100). Other parser entry points that recurse on the same stream (readGroup:,readMessage:,readMapEntry:,SkipToEndGroupInternal) already increment and checkrecursionDepth; the MessageSet path was the only spot where depth tracking did not cross the stream boundary.This change adds a package-private
-initWithData:parentRecursionDepth:initializer toGPBCodedInputStreamthat seeds the child stream's depth from the parent's and runsCheckRecursionLimitbefore parsing begins.parseMessageSet:now uses that initializer, so depth tracking carries across the fresh stream. This mirrors the depth-inheritance done by the C++ParseContextspawn helper.Regression coverage:
testParseMessageSetRecursionDepthCarriedFromParentinGPBWireFormatTestsbuilds a 200-deep MessageSet-of-MessageSet payload (each layer isMSetMessage -> MSetMessageExtension1.recursive -> MSetMessage -> ...) and asserts the parse fails withGPBCodedInputStreamErrorRecursionDepthExceeded. ExistingtestErrorRecursionDepthReachedand the rest ofGPBWireFormatTestscontinue to pass unchanged.Test plan
GPBWireFormatTests,GPBMessageTests+Serialization,GPBCodedInputStreamTests,GPBUnknownFieldsTest).testParseMessageSetRecursionDepthCarriedFromParentpasses.GPBCodedInputStream.m,GPBMessage.m) compile cleanly viaclang -fno-objc-arc. Please verify via the project's macOS CI.