iframe-proxy

sahvx655-wq · 2026-10-19T08:37:54Z

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

AVCDecoderConfigurationRecord::Create stores lengthSizeMinusOne from byte 4 of an AVC sequence header into the record and only then rejects the reserved value 2. The rejected header still overwrites length_size_minus1 of a record that was parsed successfully before. TsWriter::Write keeps _has_avc_seq_header set when a later sequence header is rejected and goes on muxing frames with the same record, so the next video frame constructs an AVCNaluIterator with length_size_minus1 == 2 and fails CHECK_NE(_length_size_minus1, 2u) in next_as_ibmf.

An RTMP publisher reaches this with three messages: a valid sequence header, a copy of it with bits 0-1 of byte 4 set to 10, then any video frame. Every frame after that logs at FATAL level with a stack trace, and with -crash_on_fatal_log the process aborts. A small program feeding that sequence into TsWriter ends in:

F1018 21:17:10.145007  8195     0 src/brpc/rtmp.cpp:951 next_as_ibmf] Check failed: _length_size_minus1 != 2u (2 vs 2).

What is changed and the side effects?

Changed:

Create() now parses into a copy of the record and assigns it back only on success, so a rejected header leaves the record as it was on every failure path (the reserved lengthSizeMinusOne, a bad SPS, truncated PPS data, ...), and the CHECK in AVCNaluIterator keeps guarding a record invariant instead of wire input. The parsing body moved unchanged into a private Parse(). RtmpTest.avc_seq_header_rejected_length_size_keeps_record covers both a lengthSizeMinusOne=2 header and one truncated after the SPS; it fails on master and passes with the fix, and the TsWriter program above completes and reports the rejected header through the returned status. All 32 tests in brpc_rtmp_unittest pass locally.

Side effects:

Performance effects: none.
Breaking backward compatibility: none. Headers with lengthSizeMinusOne == 2 were already rejected; only the state left behind changes.

Check List:

Please make sure your changes are compilable.
When providing us with a new feature, it is best to add related tests.
Please follow Contributor Covenant Code of Conduct.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

1 open issue

The error message is slightly ambiguous about the allowed values. Consider making it explicitly… · New

What changed in this PR

This PR fixes a state-corruption bug in AVC sequence header parsing where a rejected lengthSizeMinusOne == 2 value could still overwrite the previously valid AVCDecoderConfigurationRecord, later causing a fatal CHECK in AVCNaluIterator during muxing.

Changes:

Update AVCDecoderConfigurationRecord::Create to validate lengthSizeMinusOne before assigning it to the record.
Add a regression unit test ensuring a failed Create() call does not clobber an already-valid record.

_{Review effort: Lite}

sahvx655-wq · 2026-10-20T07:41:58Z

+    // 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");


That message is the existing upstream wording, the patch only moves the check ahead of the assignment. I'd rather leave the text alone here so the diff stays limited to the stale-state fix; the comment block directly above already spells out that 0, 1 and 3 are the valid values.

wwbmmm

[replied by brpc-oncall robot] The reordering in Create() correctly stops a rejected lengthSizeMinusOne from clobbering a valid record, but the stated 'rejected header leaves the record untouched' invariant does not hold for the other failure paths of Create().

wwbmmm · 2026-10-20T07:49:35Z

+// 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.


[replied by brpc-oncall robot] This comment claims a rejected header leaves the record untouched, but Create() still assigns avc_profile/avc_level and clears sps_list (and later pps_list) before several other error returns (bad SPS length, ParseSPS failure, missing PPS data, ...). A sequence header that passes the lengthSizeMinusOne check but fails in SPS/PPS parsing therefore still corrupts a previously valid record, and TsWriter::Write keeps muxing with it because _has_avc_seq_header remains true, producing TS output with stale/partial SPS/PPS. Consider parsing into a local AVCDecoderConfigurationRecord (e.g. let ParseSPS fill a target record) and committing to *this only on success, so the invariant holds for every failure path; otherwise narrow this comment to length_size_minus1 only.

Right, the reorder only covered lengthSizeMinusOne. Going back through Create() I count avc_profile and avc_level being overwritten up front, sps_list cleared and partially refilled before the SPS-length and ParseSPS checks, and pps_list cleared before the PPS checks, so a header that clears the lengthSizeMinusOne check but is truncated after the SPS still leaves TsWriter muxing with the new profile and an empty PPS list, which it then writes into the TS output.

Took the first option: Create() now copies the record, runs the existing body (moved verbatim into a private Parse()) on the copy, and assigns it back only on success, so the reorder is gone and the record is untouched on every error return. Starting from a copy rather than a fresh record keeps width/height behaving as before when a header carries no SPS. The test now also feeds a header with lengthSizeMinusOne=1 that stops before numPPS; against the previous revision it fails on avc_profile (66 vs 77), and with this change it passes along with the other 31 brpc_rtmp_unittest cases.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

Copy-constructing tmp from *this risks unintentionally preserving stale fields on success if… · New
This test intends to alter only lengthSizeMinusOne to verify that a rejected header doesn’t… · New
The error message is slightly ambiguous about the allowed values. Consider making it explicitly…

_{Review effort: Lite}

+    // 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;


+    // 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));


keep rejected lengthSizeMinusOne out of AVCDecoderConfigurationRecord

4025ba0

wwbmmm requested a balanced review from Copilot October 19, 2026 11:58

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 19, 2026 13:18
The job was not acquired by a runner even after multiple attempts.

wwbmmm reviewed Oct 20, 2026

View reviewed changes

parse AVCDecoderConfigurationRecord into a copy and commit on success

51e5154

wwbmmm requested a balanced review from Copilot October 21, 2026 12:17

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 21, 2026 16:49
The job was not acquired by a runner even after multiple attempts.

File	Description
test/brpc_rtmp_unittest.cpp	Adds a regression test verifying rejected headers don’t mutate `length_size_minus1`.
src/brpc/rtmp.cpp	Reorders parsing to reject reserved `lengthSizeMinusOne == 2` before storing it in the record.

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

keep rejected lengthSizeMinusOne out of AVCDecoderConfigurationRecord - #3625

keep rejected lengthSizeMinusOne out of AVCDecoderConfigurationRecord#3625
sahvx655-wq wants to merge 2 commits into
apache:masterfrom
sahvx655-wq:avc-length-size-stale

sahvx655-wq commented Oct 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

sahvx655-wq Oct 20, 2026

Uh oh!

wwbmmm left a comment

Uh oh!

wwbmmm Oct 20, 2026

Uh oh!

sahvx655-wq Oct 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

sahvx655-wq commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

sahvx655-wq Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm left a comment

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

sahvx655-wq Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sahvx655-wq commented Oct 19, 2026 •

edited

Loading