From 4c6151fd82590b95c9b8352a30414d9c0bae6d44 Mon Sep 17 00:00:00 2001 From: Sami Jawhar Date: Mon, 26 Oct 2026 18:44:31 +0000 Subject: [PATCH] Fix AMF0 value readers that leave part of a value unread 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 Co-authored-by: Claude Opus 5.5 --- 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 808f42d367..4d57c0e8be 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 56737500b0..137dc53fb7 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) {