Fix num_reports usage, see errata 8166

https://www.rfc-editor.org/errata/eid8166
pion · Jan 16, 2025 · cb030c7 · cb030c7
1 parent b5ab305
commit cb030c7
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 26 deletions.
diff --git a/rfc8888.go b/rfc8888.go
@@ -234,9 +234,6 @@ func (b CCFeedbackReportBlock) marshal() ([]byte, error) {
 	binary.BigEndian.PutUint16(buf[beginSequenceOffset:], b.BeginSequence)
 
 	length := uint16(len(b.MetricBlocks))
-	if length > 0 {
-		length--
-	}
 
 	binary.BigEndian.PutUint16(buf[numReportsOffset:], length)
 
@@ -258,18 +255,15 @@ func (b *CCFeedbackReportBlock) unmarshal(rawPacket []byte) error {
 	}
 	b.MediaSSRC = binary.BigEndian.Uint32(rawPacket[:beginSequenceOffset])
 	b.BeginSequence = binary.BigEndian.Uint16(rawPacket[beginSequenceOffset:numReportsOffset])
-	numReportsField := binary.BigEndian.Uint16(rawPacket[numReportsOffset:])
-	if numReportsField == 0 {
+	numReports := int(binary.BigEndian.Uint16(rawPacket[numReportsOffset:]))
+	if numReports == 0 {
 		return nil
 	}
 
-	if int(b.BeginSequence)+int(numReportsField) > math.MaxUint16 {
+	if numReports > math.MaxUint16 {
 		return errIncorrectNumReports
 	}
 
-	endSequence := b.BeginSequence + numReportsField
-	numReports := int(endSequence - b.BeginSequence + 1)
-
 	if len(rawPacket) < reportsOffset+numReports*2 {
 		return errIncorrectNumReports
 	}

diff --git a/rfc8888_test.go b/rfc8888_test.go
@@ -151,7 +151,7 @@ func TestCCFeedbackReportBlockUnmarshalMarshal(t *testing.T) {
 			Name: "ReceivedTwoOFFourBlocks",
 			Data: []byte{
 				0x00, 0x00, 0x00, 0x01, // SSRC
-				0x00, 0x02, 0x00, 0x03, // begin_seq, num_reports
+				0x00, 0x02, 0x00, 0x04, // begin_seq, num_reports
 				0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
 				0x00, 0x00, 0x00, 0x00, // reports[2], reports[3]
 			},
@@ -186,7 +186,7 @@ func TestCCFeedbackReportBlockUnmarshalMarshal(t *testing.T) {
 			Name: "ReceivedTwoOFThreeBlocksPadding",
 			Data: []byte{
 				0x00, 0x00, 0x00, 0x01, // SSRC
-				0x00, 0x02, 0x00, 0x02, // begin_seq, num_reports
+				0x00, 0x02, 0x00, 0x03, // begin_seq, num_reports
 				0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
 				0x00, 0x00, 0x00, 0x00, // reports[2], Padding
 			},
@@ -212,6 +212,41 @@ func TestCCFeedbackReportBlockUnmarshalMarshal(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name: "WrapAroundSequenceNumber",
+			Data: []byte{
+				0x00, 0x00, 0x00, 0x01, // SSRC
+				0xff, 0xfe, 0x00, 0x04, // begin_seq, num_reports
+				0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
+				0x00, 0x00, 0x00, 0x00, // reports[2], reports[3]
+			},
+			Want: CCFeedbackReportBlock{
+				MediaSSRC:     1,
+				BeginSequence: 65534,
+				MetricBlocks: []CCFeedbackMetricBlock{
+					{
+						Received:          true,
+						ECN:               0,
+						ArrivalTimeOffset: 8189,
+					},
+					{
+						Received:          true,
+						ECN:               0,
+						ArrivalTimeOffset: 8188,
+					},
+					{
+						Received:          false,
+						ECN:               0,
+						ArrivalTimeOffset: 0,
+					},
+					{
+						Received:          false,
+						ECN:               0,
+						ArrivalTimeOffset: 0,
+					},
+				},
+			},
+		},
 	} {
 		test := test
 		t.Run(fmt.Sprintf("Unmarshal-%v", test.Name), func(t *testing.T) {
@@ -270,19 +305,6 @@ func TestCCFeedbackReportBlockUnmarshalMarshal(t *testing.T) {
 		assert.ErrorIs(t, err, errIncorrectNumReports)
 	})
 
-	t.Run("overflowEndSequence", func(t *testing.T) {
-		var block CCFeedbackReportBlock
-		data := []byte{
-			0x00, 0x00, 0x00, 0x01, // SSRC
-			0xff, 0xfe, 0x00, 0x02, // begin_seq, num_reports
-			0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
-			0x00, 0x00, 0x00, 0x00, // reports[2], reports[3]
-		}
-		err := block.unmarshal(data)
-		assert.Error(t, err)
-		assert.ErrorIs(t, err, errIncorrectNumReports)
-	})
-
 	t.Run("overflowNumReports", func(t *testing.T) {
 		var block CCFeedbackReportBlock
 		data := append([]byte{
@@ -321,12 +343,12 @@ func TestCCFeedbackReportUnmarshalMarshal(t *testing.T) {
 				0x00, 0x00, 0x00, 0x01, // Sender SSRC=1
 
 				0x00, 0x00, 0x00, 0x01, // SSRC=1
-				0x00, 0x02, 0x00, 0x03, // begin_seq, num_reports
+				0x00, 0x02, 0x00, 0x04, // begin_seq, num_reports
 				0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
 				0x00, 0x00, 0x00, 0x00, // reports[2], reports[3]
 
 				0x00, 0x00, 0x00, 0x02, // Media SSRC=2
-				0x00, 0x02, 0x00, 0x02, // begin_seq=2, num_reports=3
+				0x00, 0x02, 0x00, 0x03, // begin_seq=2, num_reports=3
 				0x9F, 0xFD, 0x9F, 0xFC, // reports[0], reports[1]
 				0x00, 0x00, 0x00, 0x00, // reports[2], Padding