Repository navigation
Fix AMF0 value readers that leave part of a value unread - #3674
Merged
Merged
Conversation
AMF0 defines the type markers 0x00 to 0x11. A larger marker falls out of the switches in ReadAMFObjectField and ReadAMFArrayItem, and the readers return true without reading a value. The bytes after the marker are parsed next, and the document is accepted if they happen to parse. apache#3597 fixed the same problem for a stray object-end marker. Reject the marker, as the unsupported-marker cases in the same switches do. Switch on the raw byte, so that the new default case does not depend on converting an undefined marker to AMFMarker. The "marker=" logs in the object readers now print the byte as a number, not as a character. Two more cases in these switches leave bytes unread. The class name of a typed object can be unreadable, for example when it is longer than a lowered amf_max_string_size. The readers then only log, and the class name is parsed as the object's fields. Return false, as the string cases do. In the protobuf reader, an object value for a field that is not a message is not read. Its fields are parsed as fields of the outer object, and its end marker ends the outer object. Skip it, as the reader does for an unknown field. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused parser fixes have targeted regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
This PR extends the earlier stray object-end marker fix by closing additional AMF0 parsing paths that could leave unread bytes and misalign subsequent RTMP values.
Changes:
- Reject undefined value markers and unreadable typed-object class names in all three value readers.
- Consume object values mapped to non-message protobuf fields without assigning them.
- Expand malformed-value regression coverage and test parsing alignment after skipping an object.
| File | Description |
|---|---|
test/brpc_rtmp_unittest.cpp |
Covers invalid values across all three readers and object skipping for scalar fields. |
src/brpc/amf.cpp |
Rejects malformed values, fixes object skipping, and logs marker values numerically. |
Review effort: Balanced
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.
What problem does this PR solve?
Issue Number: none
Problem Summary: AMF0 defines the markers 0x00 to 0x11. A larger marker falls out of the value switches in both
ReadAMFObjectFieldfunctions and inReadAMFArrayItem. The functions then return true without reading a value, and the next bytes are read as the next field or item. #3597 fixed this for a stray object-end marker. Two more cases leave bytes unread: an unreadable typed-object class name, and an object value for a protobuf field that is not a message.What is changed and the side effects?
Changed:
RtmpTest.amf_rejects_invalid_value, andRtmpTest.amf_skips_object_value_of_non_message_fieldis new. Both fail on master.Side effects:
Performance effects: none.
Breaking backward compatibility: an undefined marker or an unreadable class name is now rejected.
Tested:
brpc_rtmp_unittestpasses (33 tests), clang 20.Check List: