Repository navigation
reject stray AMF0 object-end marker in amf value readers - #3597
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The parser hardening is correct, consistently applied, and covered by focused regression tests.
0 open findings
What changed in this PR
Hardens AMF0 parsing by rejecting stray object-end markers instead of triggering fatal checks or accepting malformed input.
Changes:
- Return parse failures for object-end markers in value positions.
- Add regression coverage for protobuf objects, generic objects, and strict arrays.
| File | Description |
|---|---|
src/brpc/amf.cpp |
Replaces three wire-reachable fatal checks with parse errors. |
test/brpc_rtmp_unittest.cpp |
Tests all three affected reader paths. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wasphin
pushed a commit
that referenced
this pull request
Oct 27, 2026
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. #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>
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: N/A
Problem Summary:
While going through the AMF0 readers in
amf.cppafter the recent parser hardening there, I noticed the three value readers still handle an object-end marker (0x09) in a value position withCHECK(false):ReadAMFObjectFieldfor protobuf messages, itsAMFObjectoverload, andReadAMFArrayItem. The marker byte is cut straight off the wire, so any RTMP peer reaches these switches: the message variant through theconnect/play/publishcommand objects (RtmpChunkStream::OnConnectreads the command object viaReadAMFObjectatrtmp_protocol.cpp:2385), and theAMFObject/AMFArrayvariants throughonMetaData/onCuePointdata messages and client-sideonStatus/_resultprocessing. The root cause is that a stray0x09is treated as a programming invariant when it is really just malformed input; AMF0 only defines the marker after an empty field name.A field name followed by
0x09logs at FATAL level with a stack trace on every such message, and aborts the process outright when-crash_on_fatal_logis on (CWE-617), dropping every in-flight RPC on the server. Worse, theAMFObjectandAMFArrayvariants thenbreakout of the switch and return true, so with the flag off the malformed document is accepted as if it were valid and the reader carries on misaligned against the field stream.What is changed and the side effects?
Changed:
The three wire-controlled
CHECK(false)sites becomeLOG(ERROR)plusreturn false, the same handling as the adjacent unsupported-marker cases in the same switches, so the parse fails and every caller already surfaces that as a protocol error. The fourth occurrence, in theWriteAMFFieldserialiser, is not driven by network input and is left alone. Same reasoning as the mcpack2pb hardening in #3576.RtmpTest.amf_rejects_stray_object_end_markercovers all three readers. On master the object and array cases fail (the readers return true) and the FATAL trace fires; with the fix all three inputs are rejected. The fullbrpc_rtmp_unittestpasses.Side effects:
Performance effects: none
Breaking backward compatibility: no