Repository: pion/rtp
Pull request #254 (2024-03-29 13:22:10)

fix unmarshaling panic vp8
panic when unmarshal 0x81, 0x81, 0x94 panic: runtime error: index out of range [3] with length 3



Sean-Der: Thank you so much @vfilipchenkov! When the tests pass I will merge + tag and update pion/webrtc :) 
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/rtp/pull/254?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 86.26%. Comparing base [(`a18e24d`)](https://app.codecov.io/gh/pion/rtp/commit/a18e24dc84b5797056cb45c7cab0a6aff0118f32?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`df5983b`)](https://app.codecov.io/gh/pion/rtp/pull/254?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


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


```diff
@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
+ Coverage   86.25%   86.26%   +0.01%     
==========================================
  Files          22       22              
  Lines        2160     2163       +3     
==========================================
+ Hits         1863     1866       +3     
  Misses        261      261              
  Partials       36       36              
```

| [Flag](https://app.codecov.io/gh/pion/rtp/pull/254/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/rtp/pull/254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `86.26% <100.00%> (+0.01%)` | :arrow_up: |
| [wasm](https://app.codecov.io/gh/pion/rtp/pull/254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `85.76% <100.00%> (+0.01%)` | :arrow_up: |

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.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/rtp/pull/254?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

Sean-Der: Can you update your commit message for linter?

```
Fix out of range access in VP8 Unmarshal

When unmarshaling 0x81, 0x81, 0x94

    panic: runtime error: index out of range [3] with length 3

```
Sean-Der: Updated! After this passes will merge :) 

Patch:
diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index 5d0a654..4ddd15b 100644
--- a/codecs/vp8_packet.go
+++ b/codecs/vp8_packet.go
@@ -123,7 +123,7 @@ type VP8Packet struct {
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the VP8Packet this method is called upon
-func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
+func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) { //nolint: gocognit
 	if payload == nil {
 		return nil, errNilPacket
 	}
@@ -163,6 +163,9 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 			return nil, errShortPacket
 		}
 		if payload[payloadIndex]&0x80 > 0 { // M == 1, PID is 16bit
+			if payloadIndex+1 >= payloadLen {
+				return nil, errShortPacket
+			}
 			p.PictureID = (uint16(payload[payloadIndex]&0x7F) << 8) | uint16(payload[payloadIndex+1])
 			payloadIndex += 2
 		} else {


Test patch:
diff --git a/codecs/vp8_packet_test.go b/codecs/vp8_packet_test.go
index bb09ef1..3abb520 100644
--- a/codecs/vp8_packet_test.go
+++ b/codecs/vp8_packet_test.go
@@ -106,7 +106,7 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 	// attention to partition boundaries.  In that case, it may
 	// produce packets with minimal headers.
 
-	// The next two have been witnessed in nature.
+	// The next three have been witnessed in nature.
 	_, err = pck.Unmarshal([]byte{0x00})
 	if err != nil {
 		t.Errorf("Empty packet with trivial header: %v", err)
@@ -115,6 +115,13 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 	if err != nil {
 		t.Errorf("Non-empty packet with trivial header: %v", err)
 	}
+	raw, err = pck.Unmarshal([]byte{0x81, 0x81, 0x94})
+	if raw != nil {
+		t.Fatal("Result should be nil in case of error")
+	}
+	if !errors.Is(err, errShortPacket) {
+		t.Fatal("Error should be:", errShortPacket)
+	}
 
 	// The following two were invented.
 	_, err = pck.Unmarshal([]byte{0x80, 0x00})