This is an automated email from the ASF dual-hosted git repository.
wasphin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 07bcdad23 Fix AMF0 value readers that leave part of a value unread
(#3674)
07bcdad23 is described below
commit 07bcdad23a358a1370d9e6d3b28033e110c30bbe
Author: Sami Jawhar <[email protected]>
AuthorDate: Mon Oct 26 23:00:19 2026 -0700
Fix AMF0 value readers that leave part of a value unread (#3674)
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 <[email protected]>
---
src/brpc/amf.cpp | 39 +++++++++------
test/brpc_rtmp_unittest.cpp | 113 +++++++++++++++++++++++++++++---------------
2 files changed, 99 insertions(+), 53 deletions(-)
diff --git a/src/brpc/amf.cpp b/src/brpc/amf.cpp
index 808f42d36..4d57c0e8b 100644
--- a/src/brpc/amf.cpp
+++ b/src/brpc/amf.cpp
@@ -462,7 +462,7 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
LOG(ERROR) << "stream is not long enough";
return false;
}
- switch ((AMFMarker)marker) {
+ switch (marker) {
case AMF_MARKER_NUMBER: {
uint64_t val = 0;
if (stream->cut_u64(&val) != 8u) {
@@ -511,20 +511,20 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
std::string class_name;
if (!ReadAMFShortStringBody(&class_name, stream)) {
LOG(ERROR) << "Fail to read class_name";
+ return false;
}
}
// fall through
case AMF_MARKER_OBJECT: {
- if (field) {
- if (field->cpp_type() !=
google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) {
- LOG(WARNING) << "Can't set object to " << field->full_name();
- } else {
- google::protobuf::Message* m =
reflection->MutableMessage(message, field);
- if (!ReadAMFObjectBody(m, stream, depth + 1)) {
- return false;
- }
+ if (field && field->cpp_type() ==
google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) {
+ google::protobuf::Message* m = reflection->MutableMessage(message,
field);
+ if (!ReadAMFObjectBody(m, stream, depth + 1)) {
+ return false;
}
} else {
+ if (field) {
+ LOG(WARNING) << "Can't set object to " << field->full_name();
+ }
if (!SkipAMFObjectBody(stream, depth + 1)) {
return false;
}
@@ -563,6 +563,9 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
}
}
} break;
+ default:
+ LOG(ERROR) << "Unknown marker=" << (int)marker;
+ return false;
} // switch
return true;
}
@@ -583,7 +586,7 @@ static bool ReadAMFObjectBody(google::protobuf::Message*
message,
return false;
}
if ((AMFMarker)marker != AMF_MARKER_OBJECT_END) {
- LOG(ERROR) << "marker=" << marker
+ LOG(ERROR) << "marker=" << (int)marker
<< " after empty name is not object end";
return false;
}
@@ -612,7 +615,7 @@ static bool SkipAMFObjectBody(AMFInputStream* stream, int
depth) {
return false;
}
if ((AMFMarker)marker != AMF_MARKER_OBJECT_END) {
- LOG(ERROR) << "marker=" << marker
+ LOG(ERROR) << "marker=" << (int)marker
<< " after empty name is not object end";
return false;
}
@@ -701,7 +704,7 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
LOG(ERROR) << "stream is not long enough";
return false;
}
- switch ((AMFMarker)marker) {
+ switch (marker) {
case AMF_MARKER_NUMBER: {
uint64_t val = 0;
if (stream->cut_u64(&val) != 8u) {
@@ -730,6 +733,7 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
std::string class_name;
if (!ReadAMFShortStringBody(&class_name, stream)) {
LOG(ERROR) << "Fail to read class_name";
+ return false;
}
}
// fall through
@@ -776,6 +780,9 @@ static bool ReadAMFObjectField(AMFInputStream* stream,
}
obj->SetString(name, val);
} break;
+ default:
+ LOG(ERROR) << "Unknown marker=" << (int)marker;
+ return false;
} // switch
return true;
}
@@ -793,7 +800,7 @@ static bool ReadAMFObjectBody(AMFObject* obj,
AMFInputStream* stream, int depth)
return false;
}
if ((AMFMarker)marker != AMF_MARKER_OBJECT_END) {
- LOG(ERROR) << "marker=" << marker
+ LOG(ERROR) << "marker=" << (int)marker
<< " after empty name is not object end";
return false;
}
@@ -862,7 +869,7 @@ static bool ReadAMFArrayItem(AMFInputStream* stream,
AMFArray* arr, int depth) {
LOG(ERROR) << "stream is not long enough";
return false;
}
- switch ((AMFMarker)marker) {
+ switch (marker) {
case AMF_MARKER_NUMBER: {
uint64_t val = 0;
if (stream->cut_u64(&val) != 8u) {
@@ -891,6 +898,7 @@ static bool ReadAMFArrayItem(AMFInputStream* stream,
AMFArray* arr, int depth) {
std::string class_name;
if (!ReadAMFShortStringBody(&class_name, stream)) {
LOG(ERROR) << "Fail to read class_name";
+ return false;
}
}
// fall through
@@ -937,6 +945,9 @@ static bool ReadAMFArrayItem(AMFInputStream* stream,
AMFArray* arr, int depth) {
}
arr->AddString(val);
} break;
+ default:
+ LOG(ERROR) << "Unknown marker=" << (int)marker;
+ return false;
} // switch
return true;
}
diff --git a/test/brpc_rtmp_unittest.cpp b/test/brpc_rtmp_unittest.cpp
index 56737500b..137dc53fb 100644
--- a/test/brpc_rtmp_unittest.cpp
+++ b/test/brpc_rtmp_unittest.cpp
@@ -901,47 +901,82 @@ TEST(RtmpTest, amf_rejects_oversized_strict_array_count) {
EXPECT_EQ(0u, arr.size());
}
-TEST(RtmpTest, amf_rejects_stray_object_end_marker) {
- // A stray object-end marker (0x09) in a value position used to hit
- // CHECK(false): the process aborted with -crash_on_fatal_log, and the
- // object/array readers then continued as if the input were valid.
- {
- // As the value of a field in a generic object.
- std::string req_buf;
- AppendAMFObjectHeader(&req_buf);
- AppendAMFShortStringBody(&req_buf, "a");
- req_buf.push_back((char)brpc::AMF_MARKER_OBJECT_END);
- AppendAMFObjectEnd(&req_buf);
-
- google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
- brpc::AMFInputStream istream(&zc_stream);
- brpc::AMFObject obj;
- EXPECT_FALSE(brpc::ReadAMFObject(&obj, &istream));
+TEST(RtmpTest, amf_rejects_invalid_value) {
+ // A value position must reject a stray object-end marker (0x09), any
+ // marker that AMF0 does not define (0x12 and up), and a typed object
+ // whose class name is longer than the limit.
+ ScopedAMFLimit scoped_limit(&brpc::FLAGS_amf_max_string_size, 4);
+ std::string typed_object(1, (char)brpc::AMF_MARKER_TYPED_OBJECT);
+ AppendAMFShortStringBody(&typed_object, "hello");
+ AppendAMFObjectEnd(&typed_object);
+ const std::string values[] = {
+ std::string(1, (char)brpc::AMF_MARKER_OBJECT_END),
+ std::string(1, (char)0x12),
+ std::string(1, (char)0xff),
+ typed_object,
+ };
+ for (size_t i = 0; i < arraysize(values); ++i) {
+ {
+ // As the value of a field in a generic object.
+ std::string req_buf;
+ AppendAMFObjectHeader(&req_buf);
+ AppendAMFShortStringBody(&req_buf, "a");
+ req_buf.append(values[i]);
+ AppendAMFObjectEnd(&req_buf);
+
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ brpc::AMFObject obj;
+ EXPECT_FALSE(brpc::ReadAMFObject(&obj, &istream)) << "i=" << i;
+ }
+ {
+ // As an item of a strict array.
+ std::string req_buf;
+ AppendAMFStrictArrayHeader(&req_buf, 1);
+ req_buf.append(values[i]);
+
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ brpc::AMFArray arr;
+ EXPECT_FALSE(brpc::ReadAMFArray(&arr, &istream)) << "i=" << i;
+ }
+ {
+ // As the value of a field mapped to a protobuf message.
+ std::string req_buf;
+ AppendAMFObjectHeader(&req_buf);
+ AppendAMFShortStringBody(&req_buf, "code");
+ req_buf.append(values[i]);
+ AppendAMFObjectEnd(&req_buf);
+
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ brpc::RtmpInfo info;
+ EXPECT_FALSE(brpc::ReadAMFObject(&info, &istream)) << "i=" << i;
+ }
}
- {
- // As an item of a strict array.
- std::string req_buf;
- AppendAMFStrictArrayHeader(&req_buf, 1);
- req_buf.push_back((char)brpc::AMF_MARKER_OBJECT_END);
+}
- google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
- brpc::AMFInputStream istream(&zc_stream);
- brpc::AMFArray arr;
- EXPECT_FALSE(brpc::ReadAMFArray(&arr, &istream));
- }
- {
- // As the value of a field mapped to a protobuf message.
- std::string req_buf;
- AppendAMFObjectHeader(&req_buf);
- AppendAMFShortStringBody(&req_buf, "code");
- req_buf.push_back((char)brpc::AMF_MARKER_OBJECT_END);
- AppendAMFObjectEnd(&req_buf);
-
- google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
- brpc::AMFInputStream istream(&zc_stream);
- brpc::RtmpInfo info;
- EXPECT_FALSE(brpc::ReadAMFObject(&info, &istream));
- }
+TEST(RtmpTest, amf_skips_object_value_of_non_message_field) {
+ // {code: {level: "inner"}, description: "outer"}: code is a string.
+ std::string req_buf;
+ AppendAMFObjectHeader(&req_buf);
+ AppendAMFShortStringBody(&req_buf, "code");
+ AppendAMFObjectHeader(&req_buf);
+ AppendAMFShortStringBody(&req_buf, "level");
+ req_buf.push_back((char)brpc::AMF_MARKER_STRING);
+ AppendAMFShortStringBody(&req_buf, "inner");
+ AppendAMFObjectEnd(&req_buf);
+ AppendAMFShortStringBody(&req_buf, "description");
+ req_buf.push_back((char)brpc::AMF_MARKER_STRING);
+ AppendAMFShortStringBody(&req_buf, "outer");
+ AppendAMFObjectEnd(&req_buf);
+
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ brpc::RtmpInfo info;
+ ASSERT_TRUE(brpc::ReadAMFObject(&info, &istream));
+ EXPECT_FALSE(info.has_level());
+ EXPECT_EQ("outer", info.description());
}
TEST(RtmpTest, amf_rejects_deep_nested_arrays) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
