From 4025ba0ccc8d0b4bd227e27b2bb3a9ddd512b484 Mon Sep 17 00:00:00 2001 From: Sahana Bogar Date: Mon, 19 Oct 2026 02:50:18 +0530 Subject: [PATCH 1/2] keep rejected lengthSizeMinusOne out of AVCDecoderConfigurationRecord --- src/brpc/rtmp.cpp | 6 ++++-- test/brpc_rtmp_unittest.cpp | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/brpc/rtmp.cpp b/src/brpc/rtmp.cpp index b897aed759..919a8023c3 100644 --- a/src/brpc/rtmp.cpp +++ b/src/brpc/rtmp.cpp @@ -605,10 +605,12 @@ butil::Status AVCDecoderConfigurationRecord::Create(const void* data, size_t len // 5.2.4.1.2 Semantics // The value of this field shall be one of 0, 1, or 3 corresponding to a // length encoded with 1, 2, or 4 bytes, respectively. - length_size_minus1 = buf[4] & 0x03; - if (length_size_minus1 == 2) { + // Reject before storing: AVCNaluIterator CHECKs the stored value, and + // TsWriter keeps using its record after a failed Create(). + if ((buf[4] & 0x03) == 2) { return butil::Status(EINVAL, "lengthSizeMinusOne should never be 2"); } + length_size_minus1 = buf[4] & 0x03; // Parsing SPS const int num_sps = (int)(buf[5] & 0x1f); diff --git a/test/brpc_rtmp_unittest.cpp b/test/brpc_rtmp_unittest.cpp index 36e996b063..bbb5ff8ba1 100644 --- a/test/brpc_rtmp_unittest.cpp +++ b/test/brpc_rtmp_unittest.cpp @@ -1046,6 +1046,39 @@ TEST(RtmpTest, avc_seq_header_sps_exp_golomb) { ASSERT_EQ(80, avc.height); } +// Create() stored lengthSizeMinusOne into the record before rejecting the +// reserved value 2, so a rejected sequence header clobbered the field of a +// record that had been parsed successfully before. TsWriter keeps muxing with +// its record after a rejected header, and AVCNaluIterator then trips +// CHECK_NE(_length_size_minus1, 2u) on the next video frame. A rejected +// header must leave the record untouched. +TEST(RtmpTest, avc_seq_header_rejected_length_size_keeps_record) { + // Same baseline SPS as the exp-golomb test above, lengthSizeMinusOne=3. + const char sps[7] = { 0x67, 0x42, 0x00, 0x1E, (char)0xdc, 0x52, (char)0xc0 }; + const char len_be[2] = { 0x00, (char)sizeof(sps) }; + butil::IOBuf good; + const char good_head[6] = { 0x01, 0x42, 0x00, 0x1E, (char)0xff, (char)0xe1 }; + good.append(good_head, sizeof(good_head)); + good.append(len_be, sizeof(len_be)); + good.append(sps, sizeof(sps)); + good.push_back('\0'); // numPPS=0 + + brpc::AVCDecoderConfigurationRecord avc; + ASSERT_TRUE(avc.Create(good).ok()); + ASSERT_EQ(3, avc.length_size_minus1); + + // The same record with lengthSizeMinusOne=2, which Create() rejects. + butil::IOBuf bad; + const char bad_head[6] = { 0x01, 0x42, 0x00, 0x1E, (char)0xfe, (char)0xe1 }; + bad.append(bad_head, sizeof(bad_head)); + bad.append(len_be, sizeof(len_be)); + bad.append(sps, sizeof(sps)); + bad.push_back('\0'); // numPPS=0 + + ASSERT_FALSE(avc.Create(bad).ok()); + ASSERT_EQ(3, avc.length_size_minus1); +} + static void AppendBigEndian3Bytes(std::string* s, uint32_t v) { s->push_back((char)((v >> 16) & 0xFF)); s->push_back((char)((v >> 8) & 0xFF)); From 51e51546674599804b6bb17df8b5cfd3617e40e9 Mon Sep 17 00:00:00 2001 From: Sahana Bogar Date: Wed, 21 Oct 2026 15:09:45 +0530 Subject: [PATCH 2/2] parse AVCDecoderConfigurationRecord into a copy and commit on success --- src/brpc/rtmp.cpp | 18 ++++++++++---- src/brpc/rtmp.h | 1 + test/brpc_rtmp_unittest.cpp | 47 ++++++++++++++++++++++++++++--------- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/brpc/rtmp.cpp b/src/brpc/rtmp.cpp index 919a8023c3..b0a5f04a6e 100644 --- a/src/brpc/rtmp.cpp +++ b/src/brpc/rtmp.cpp @@ -590,6 +590,18 @@ butil::Status AVCDecoderConfigurationRecord::Create(const butil::IOBuf& buf) { } butil::Status AVCDecoderConfigurationRecord::Create(const void* data, size_t len) { + // Parse into a copy and commit only on success, so that a rejected + // header leaves *this untouched. TsWriter keeps muxing with its record + // after a failed Create(), and AVCNaluIterator CHECKs length_size_minus1. + AVCDecoderConfigurationRecord tmp(*this); + butil::Status st = tmp.Parse(data, len); + if (st.ok()) { + *this = std::move(tmp); + } + return st; +} + +butil::Status AVCDecoderConfigurationRecord::Parse(const void* data, size_t len) { butil::StringPiece buf((const char*)data, len); if (buf.size() < 6) { return butil::Status(EINVAL, "Length=%lu is not long enough", @@ -605,12 +617,10 @@ butil::Status AVCDecoderConfigurationRecord::Create(const void* data, size_t len // 5.2.4.1.2 Semantics // The value of this field shall be one of 0, 1, or 3 corresponding to a // length encoded with 1, 2, or 4 bytes, respectively. - // Reject before storing: AVCNaluIterator CHECKs the stored value, and - // TsWriter keeps using its record after a failed Create(). - if ((buf[4] & 0x03) == 2) { + length_size_minus1 = buf[4] & 0x03; + if (length_size_minus1 == 2) { return butil::Status(EINVAL, "lengthSizeMinusOne should never be 2"); } - length_size_minus1 = buf[4] & 0x03; // Parsing SPS const int num_sps = (int)(buf[5] & 0x1f); diff --git a/src/brpc/rtmp.h b/src/brpc/rtmp.h index 986d7539db..f58441bc25 100644 --- a/src/brpc/rtmp.h +++ b/src/brpc/rtmp.h @@ -325,6 +325,7 @@ struct AVCDecoderConfigurationRecord { std::vector pps_list; private: + butil::Status Parse(const void* data, size_t len); butil::Status ParseSPS(const butil::StringPiece& buf, size_t sps_length); }; std::ostream& operator<<(std::ostream&, const AVCDecoderConfigurationRecord&); diff --git a/test/brpc_rtmp_unittest.cpp b/test/brpc_rtmp_unittest.cpp index bbb5ff8ba1..9d721e798b 100644 --- a/test/brpc_rtmp_unittest.cpp +++ b/test/brpc_rtmp_unittest.cpp @@ -1046,37 +1046,62 @@ TEST(RtmpTest, avc_seq_header_sps_exp_golomb) { ASSERT_EQ(80, avc.height); } -// Create() stored lengthSizeMinusOne into the record before rejecting the -// reserved value 2, so a rejected sequence header clobbered the field of a -// record that had been parsed successfully before. TsWriter keeps muxing with -// its record after a rejected header, and AVCNaluIterator then trips -// CHECK_NE(_length_size_minus1, 2u) on the next video frame. A rejected -// header must leave the record untouched. +// Create() wrote fields of the record (lengthSizeMinusOne, profile, level, +// SPS/PPS lists) before all of its checks had passed, so a rejected sequence +// header clobbered a record that had been parsed successfully before. +// TsWriter keeps muxing with its record after a rejected header, and +// AVCNaluIterator then trips CHECK_NE(_length_size_minus1, 2u) on the next +// video frame. A rejected header must leave the record untouched. TEST(RtmpTest, avc_seq_header_rejected_length_size_keeps_record) { // Same baseline SPS as the exp-golomb test above, lengthSizeMinusOne=3. const char sps[7] = { 0x67, 0x42, 0x00, 0x1E, (char)0xdc, 0x52, (char)0xc0 }; - const char len_be[2] = { 0x00, (char)sizeof(sps) }; + const char sps_len_be[2] = { 0x00, (char)sizeof(sps) }; + const char pps[4] = { 0x68, (char)0xce, 0x38, (char)0x80 }; + const char pps_len_be[2] = { 0x00, (char)sizeof(pps) }; butil::IOBuf good; const char good_head[6] = { 0x01, 0x42, 0x00, 0x1E, (char)0xff, (char)0xe1 }; good.append(good_head, sizeof(good_head)); - good.append(len_be, sizeof(len_be)); + good.append(sps_len_be, sizeof(sps_len_be)); good.append(sps, sizeof(sps)); - good.push_back('\0'); // numPPS=0 + good.push_back('\1'); // numPPS=1 + good.append(pps_len_be, sizeof(pps_len_be)); + good.append(pps, sizeof(pps)); brpc::AVCDecoderConfigurationRecord avc; ASSERT_TRUE(avc.Create(good).ok()); ASSERT_EQ(3, avc.length_size_minus1); + ASSERT_EQ(brpc::AVC_PROFILE_BASELINE, avc.avc_profile); + ASSERT_EQ(1u, avc.sps_list.size()); + ASSERT_EQ(1u, avc.pps_list.size()); // The same record with lengthSizeMinusOne=2, which Create() rejects. butil::IOBuf bad; - const char bad_head[6] = { 0x01, 0x42, 0x00, 0x1E, (char)0xfe, (char)0xe1 }; + const char bad_head[6] = { 0x01, 0x4d, 0x00, 0x1F, (char)0xfe, (char)0xe1 }; bad.append(bad_head, sizeof(bad_head)); - bad.append(len_be, sizeof(len_be)); + bad.append(sps_len_be, sizeof(sps_len_be)); bad.append(sps, sizeof(sps)); bad.push_back('\0'); // numPPS=0 ASSERT_FALSE(avc.Create(bad).ok()); ASSERT_EQ(3, avc.length_size_minus1); + ASSERT_EQ(brpc::AVC_PROFILE_BASELINE, avc.avc_profile); + ASSERT_EQ(1u, avc.sps_list.size()); + ASSERT_EQ(1u, avc.pps_list.size()); + + // A valid lengthSizeMinusOne=1 but truncated before numPPS, so Create() + // fails after the SPS has been parsed. + butil::IOBuf truncated; + const char trunc_head[6] = { 0x01, 0x4d, 0x00, 0x1F, (char)0xfd, (char)0xe1 }; + truncated.append(trunc_head, sizeof(trunc_head)); + truncated.append(sps_len_be, sizeof(sps_len_be)); + truncated.append(sps, sizeof(sps)); + + ASSERT_FALSE(avc.Create(truncated).ok()); + ASSERT_EQ(3, avc.length_size_minus1); + ASSERT_EQ(brpc::AVC_PROFILE_BASELINE, avc.avc_profile); + ASSERT_EQ(1u, avc.sps_list.size()); + ASSERT_EQ(1u, avc.pps_list.size()); + ASSERT_EQ(std::string(pps, sizeof(pps)), avc.pps_list[0]); } static void AppendBigEndian3Bytes(std::string* s, uint32_t v) {