Repository: pion/rtp
Pull request #206 (2023-01-09 19:02:30)

Fix parsing of VP8 packets with degenerate header
This is a forward port of #203.

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/206?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Base: **87.60**% // Head: **87.43**% // Decreases project coverage by **`-0.16%`** :warning:
> Coverage data is based on head [(`4933edd`)](https://codecov.io/gh/pion/rtp/pull/206?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) compared to base [(`d13e8b6`)](https://codecov.io/gh/pion/rtp/commit/d13e8b61b96c5985359fd6cd6d88a3115b22cedd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Patch coverage: 72.72% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
- Coverage   87.60%   87.43%   -0.17%     
==========================================
  Files          20       20              
  Lines        1831     1854      +23     
==========================================
+ Hits         1604     1621      +17     
- Misses        197      201       +4     
- Partials       30       32       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `87.43% <72.72%> (-0.17%)` | :arrow_down: |
| wasm | `86.46% <72.72%> (-0.16%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/rtp/pull/206?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [codecs/vp8\_packet.go](https://codecov.io/gh/pion/rtp/pull/206?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL3ZwOF9wYWNrZXQuZ28=) | `91.08% <72.72%> (-2.95%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/pion/rtp/pull/206?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index cd86929..b831dd3 100644
--- a/codecs/vp8_packet.go
+++ b/codecs/vp8_packet.go
@@ -127,12 +127,11 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 
 	payloadLen := len(payload)
 
-	if payloadLen < 4 {
-		return nil, errShortPacket
-	}
-
 	payloadIndex := 0
 
+	if payloadIndex >= payloadLen {
+		return nil, errShortPacket
+	}
 	p.X = (payload[payloadIndex] & 0x80) >> 7
 	p.N = (payload[payloadIndex] & 0x20) >> 5
 	p.S = (payload[payloadIndex] & 0x10) >> 4
@@ -141,14 +140,25 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 	payloadIndex++
 
 	if p.X == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		p.I = (payload[payloadIndex] & 0x80) >> 7
 		p.L = (payload[payloadIndex] & 0x40) >> 6
 		p.T = (payload[payloadIndex] & 0x20) >> 5
 		p.K = (payload[payloadIndex] & 0x10) >> 4
 		payloadIndex++
+	} else {
+		p.I = 0
+		p.L = 0
+		p.T = 0
+		p.K = 0
 	}
 
 	if p.I == 1 { // PID present?
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		if payload[payloadIndex]&0x80 > 0 { // M == 1, PID is 16bit
 			p.PictureID = (uint16(payload[payloadIndex]&0x7F) << 8) | uint16(payload[payloadIndex+1])
 			payloadIndex += 2
@@ -156,35 +166,43 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 			p.PictureID = uint16(payload[payloadIndex])
 			payloadIndex++
 		}
-	}
-
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
+	} else {
+		p.PictureID = 0
 	}
 
 	if p.L == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		p.TL0PICIDX = payload[payloadIndex]
 		payloadIndex++
-	}
-
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
+	} else {
+		p.TL0PICIDX = 0
 	}
 
 	if p.T == 1 || p.K == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		if p.T == 1 {
 			p.TID = payload[payloadIndex] >> 6
 			p.Y = (payload[payloadIndex] >> 5) & 0x1
+		} else {
+			p.TID = 0
+			p.Y = 0
 		}
 		if p.K == 1 {
 			p.KEYIDX = payload[payloadIndex] & 0x1F
+		} else {
+			p.KEYIDX = 0
 		}
 		payloadIndex++
+	} else {
+		p.TID = 0
+		p.Y = 0
+		p.KEYIDX = 0
 	}
 
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
-	}
 	p.Payload = payload[payloadIndex:]
 	return p.Payload, nil
 }


Test patch:
diff --git a/codecs/vp8_packet_test.go b/codecs/vp8_packet_test.go
index b173cfb..e7a9610 100644
--- a/codecs/vp8_packet_test.go
+++ b/codecs/vp8_packet_test.go
@@ -27,15 +27,6 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 		t.Fatal("Error should be:", errShortPacket)
 	}
 
-	// Payload smaller than header size
-	raw, err = pck.Unmarshal([]byte{0x00, 0x11, 0x22})
-	if raw != nil {
-		t.Fatal("Result should be nil in case of error")
-	}
-	if !errors.Is(err, errShortPacket) {
-		t.Fatal("Error should be:", errShortPacket)
-	}
-
 	// Normal payload
 	raw, err = pck.Unmarshal([]byte{0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x90})
 	if raw == nil {
@@ -65,11 +56,11 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 
 	// Header size, X and I, PID 16bits
 	raw, err = pck.Unmarshal([]byte{0x80, 0x80, 0x81, 0x00})
-	if raw != nil {
-		t.Fatal("Result should be nil in case of error")
+	if raw == nil {
+		t.Fatal("Result shouldn't be nil in case of success")
 	}
-	if !errors.Is(err, errShortPacket) {
-		t.Fatal("Error should be:", errShortPacket)
+	if err != nil {
+		t.Fatal("Error should be nil in case of success")
 	}
 
 	// Header size, X and L
@@ -107,6 +98,30 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 	if !errors.Is(err, errShortPacket) {
 		t.Fatal("Error should be:", errShortPacket)
 	}
+
+	// According to RFC 7741 Section 4.4, the packetizer need not pay
+	// attention to partition boundaries.  In that case, it may
+	// produce packets with minimal headers.
+
+	// The next two have been witnessed in nature.
+	_, err = pck.Unmarshal([]byte{0x00})
+	if err != nil {
+		t.Errorf("Empty packet with trivial header: %v", err)
+	}
+	_, err = pck.Unmarshal([]byte{0x00, 0x2a, 0x94})
+	if err != nil {
+		t.Errorf("Non-empty packet with trivial header: %v", err)
+	}
+
+	// The following two were invented.
+	_, err = pck.Unmarshal([]byte{0x80, 0x00})
+	if err != nil {
+		t.Errorf("Empty packet with trivial extension: %v", err)
+	}
+	_, err = pck.Unmarshal([]byte{0x80, 0x80, 42})
+	if err != nil {
+		t.Errorf("Header with PictureID: %v", err)
+	}
 }
 
 func TestVP8Payloader_Payload(t *testing.T) {