Copilot commented on code in PR #3625:
URL: https://github.com/apache/brpc/pull/3625#discussion_r4336821680
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1046,6 +1046,64 @@ TEST(RtmpTest, avc_seq_header_sps_exp_golomb) {
ASSERT_EQ(80, avc.height);
}
+// 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 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(sps_len_be, sizeof(sps_len_be));
+ good.append(sps, sizeof(sps));
+ 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, 0x4d, 0x00, 0x1F, (char)0xfe, (char)0xe1
};
+ bad.append(bad_head, sizeof(bad_head));
Review Comment:
This test intends to alter only `lengthSizeMinusOne` to verify that a
rejected header doesn’t clobber an existing record, but `bad_head` also changes
profile/compat/level bytes compared to `good_head`. That makes the failure less
attributable to the reserved `lengthSizeMinusOne==2` value and can reduce test
clarity. Consider keeping bytes 1–3 identical to `good_head` and only changing
byte 4’s low 2 bits (and similarly for `trunc_head`) to isolate what’s being
exercised.
##########
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;
Review Comment:
Copy-constructing `tmp` from `*this` risks unintentionally preserving stale
fields on success if `Parse()` doesn’t explicitly overwrite every member (and
it also adds avoidable copy cost for `sps_list`/`pps_list`). Prefer parsing
into a fresh/default-initialized `AVCDecoderConfigurationRecord tmp;` (or
resetting fields at the start of `Parse()`), then commit via assignment/swap
only on success. This makes it harder for future fields to accidentally carry
over from previous state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
