Repository: pion/rtp
Pull request #62 (2020-05-18 04:42:26)

Fix AbsSendTime marshal/unmarshal in Packetizer
ID/len byte of AbsSendTime was duplicated
and ID of AbsSendTime extension was always unmarshalled as 0.
(regression from edd47ed5)

### Reference issue
Ref #56


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=h1) Report
> Merging [#62](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=desc) into [master](https://codecov.io/gh/pion/rtp/commit/ee3e7ab3d3222745a80bc10bdf6569da843d7bc3&el=desc) will **increase** coverage by `1.86%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/rtp/pull/62/graphs/tree.svg?width=650&height=150&src=pr&token=4KrbiBsspj)](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   88.85%   90.72%   +1.86%     
==========================================
  Files          13       13              
  Lines         628      625       -3     
==========================================
+ Hits          558      567       +9     
+ Misses         54       41      -13     
- Partials       16       17       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `90.72% <100.00%> (+1.86%)` | :arrow_up: |
| #wasm | `90.72% <100.00%> (+1.86%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [abssendtimeextension.go](https://codecov.io/gh/pion/rtp/pull/62/diff?src=pr&el=tree#diff-YWJzc2VuZHRpbWVleHRlbnNpb24uZ28=) | `94.44% <100.00%> (-0.30%)` | :arrow_down: |
| [packetizer.go](https://codecov.io/gh/pion/rtp/pull/62/diff?src=pr&el=tree#diff-cGFja2V0aXplci5nbw==) | `86.36% <100.00%> (+19.69%)` | :arrow_up: |
| [sequencer.go](https://codecov.io/gh/pion/rtp/pull/62/diff?src=pr&el=tree#diff-c2VxdWVuY2VyLmdv) | `68.42% <0.00%> (+15.78%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=footer). Last update [ee3e7ab...8d97114](https://codecov.io/gh/pion/rtp/pull/62?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

sgotti: @at-wat I was just thinking if, after #56, the various extensions should be updated to just parse the extension payload and not the header. This is probably a breaking API change but also #56 introduced  a breaking API change.

Patch:
diff --git a/abssendtimeextension.go b/abssendtimeextension.go
index 5f560e8..99a2fe9 100644
--- a/abssendtimeextension.go
+++ b/abssendtimeextension.go
@@ -5,26 +5,18 @@ import (
 )
 
 const (
-	absSendTimeExtensionSize = 4
+	absSendTimeExtensionSize = 3
 )
 
 // AbsSendTimeExtension is a extension payload format in
 // http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
-//  0 1 2 3 4 5 6 7
-// +-+-+-+-+-+-+-+-+
-// |  ID   |  len  |
-// +-+-+-+-+-+-+-+-+
-// per RFC 5285
-// Len is the number of bytes in the extension - 1.
 type AbsSendTimeExtension struct {
-	ID        uint8
 	Timestamp uint64
 }
 
 // Marshal serializes the members to buffer.
 func (t *AbsSendTimeExtension) Marshal() ([]byte, error) {
 	return []byte{
-		(t.ID << 4) | 2,
 		byte(t.Timestamp & 0xFF0000 >> 16),
 		byte(t.Timestamp & 0xFF00 >> 8),
 		byte(t.Timestamp & 0xFF),
@@ -36,8 +28,7 @@ func (t *AbsSendTimeExtension) Unmarshal(rawData []byte) error {
 	if len(rawData) < absSendTimeExtensionSize {
 		return errTooSmall
 	}
-	t.ID = rawData[0] >> 4
-	t.Timestamp = uint64(rawData[1])<<16 | uint64(rawData[2])<<8 | uint64(rawData[3])
+	t.Timestamp = uint64(rawData[0])<<16 | uint64(rawData[1])<<8 | uint64(rawData[2])
 	return nil
 }
 
diff --git a/error.go b/error.go
new file mode 100644
index 0000000..57f47db
--- /dev/null
+++ b/error.go
@@ -0,0 +1,8 @@
+package rtp
+
+import (
+	"errors"
+)
+
+var errHeaderSizeInsufficient = errors.New("RTP header size insufficient")
+var errHeaderSizeInsufficientForExtension = errors.New("RTP header size insufficient for extension")
diff --git a/go.mod b/go.mod
index 1046d76..afd606b 100644
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
 module github.com/pion/rtp
 
-go 1.12
+go 1.13
diff --git a/packet.go b/packet.go
index 6986623..ff6bd2a 100644
--- a/packet.go
+++ b/packet.go
@@ -105,6 +105,11 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 	h.Extension = (rawPacket[0] >> extensionShift & extensionMask) > 0
 	h.CSRC = make([]uint32, rawPacket[0]&ccMask)
 
+	currOffset := csrcOffset + (len(h.CSRC) * csrcLength)
+	if len(rawPacket) < currOffset {
+		return fmt.Errorf("size %d < %d: %w", len(rawPacket), currOffset, errHeaderSizeInsufficient)
+	}
+
 	h.Marker = (rawPacket[1] >> markerShift & markerMask) > 0
 	h.PayloadType = rawPacket[1] & ptMask
 
@@ -112,19 +117,17 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 	h.Timestamp = binary.BigEndian.Uint32(rawPacket[timestampOffset : timestampOffset+timestampLength])
 	h.SSRC = binary.BigEndian.Uint32(rawPacket[ssrcOffset : ssrcOffset+ssrcLength])
 
-	currOffset := csrcOffset + (len(h.CSRC) * csrcLength)
-	if len(rawPacket) < currOffset {
-		return fmt.Errorf("RTP header size insufficient; %d < %d", len(rawPacket), currOffset)
-	}
-
 	for i := range h.CSRC {
 		offset := csrcOffset + (i * csrcLength)
 		h.CSRC[i] = binary.BigEndian.Uint32(rawPacket[offset:])
 	}
 
 	if h.Extension {
-		if len(rawPacket) < currOffset+4 {
-			return fmt.Errorf("RTP header size insufficient for extension; %d < %d", len(rawPacket), currOffset)
+		if expected := currOffset + 4; len(rawPacket) < expected {
+			return fmt.Errorf("size %d < %d: %w",
+				len(rawPacket), expected,
+				errHeaderSizeInsufficientForExtension,
+			)
 		}
 
 		h.ExtensionProfile = binary.BigEndian.Uint16(rawPacket[currOffset:])
@@ -132,6 +135,13 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 		extensionLength := int(binary.BigEndian.Uint16(rawPacket[currOffset:])) * 4
 		currOffset += 2
 
+		if expected := currOffset + extensionLength; len(rawPacket) < expected {
+			return fmt.Errorf("size %d < %d: %w",
+				len(rawPacket), expected,
+				errHeaderSizeInsufficientForExtension,
+			)
+		}
+
 		switch h.ExtensionProfile {
 		// RFC 8285 RTP One Byte Header Extension
 		case extensionProfileOneByte:
@@ -301,7 +311,10 @@ func (h *Header) MarshalTo(buf []byte) (n int, err error) {
 		binary.BigEndian.PutUint16(buf[extHeaderPos+2:extHeaderPos+4], uint16(roundedExtSize/4))
 
 		// add padding to reach 4 bytes boundaries
-		n += roundedExtSize - extSize
+		for i := 0; i < roundedExtSize-extSize; i++ {
+			buf[n] = 0
+			n++
+		}
 	}
 
 	h.PayloadOffset = n
diff --git a/packetizer.go b/packetizer.go
index ccdb55a..140ad7b 100644
--- a/packetizer.go
+++ b/packetizer.go
@@ -82,13 +82,12 @@ func (p *packetizer) Packetize(payload []byte, samples uint32) []*Packet {
 		t := toNtpTime(p.timegen()) >> 14
 		//apply http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
 		b, err := (&AbsSendTimeExtension{
-			ID:        uint8(p.extensionNumbers.AbsSendTime),
 			Timestamp: t,
 		}).Marshal()
 		if err != nil {
 			return nil // never happens
 		}
-		err = packets[len(packets)-1].SetExtension(0, b)
+		err = packets[len(packets)-1].SetExtension(uint8(p.extensionNumbers.AbsSendTime), b)
 		if err != nil {
 			return nil // never happens
 		}


Test patch:
diff --git a/abssendtimeextension_test.go b/abssendtimeextension_test.go
index 5efb9b3..9bae6c3 100644
--- a/abssendtimeextension_test.go
+++ b/abssendtimeextension_test.go
@@ -41,11 +41,9 @@ func TestNtpConversion(t *testing.T) {
 func TestAbsSendTimeExtension_Roundtrip(t *testing.T) {
 	tests := []AbsSendTimeExtension{
 		{
-			ID:        1,
 			Timestamp: 123456,
 		},
 		{
-			ID:        2,
 			Timestamp: 654321,
 		},
 	}
@@ -58,9 +56,6 @@ func TestAbsSendTimeExtension_Roundtrip(t *testing.T) {
 		if err = out.Unmarshal(b); err != nil {
 			t.Fatal(err)
 		}
-		if in.ID != out.ID {
-			t.Errorf("[%d] ID differs, expected: %d, got: %d", i, in.ID, out.ID)
-		}
 		if in.Timestamp != out.Timestamp {
 			t.Errorf("[%d] Timestamp differs, expected: %d, got: %d", i, in.Timestamp, out.Timestamp)
 		}
@@ -77,7 +72,7 @@ func TestAbsSendTimeExtension_Estimate(t *testing.T) {
 	}
 	for i, in := range tests {
 		inTime := toTime(in.sendNTP)
-		send := &AbsSendTimeExtension{0, in.sendNTP >> 14}
+		send := &AbsSendTimeExtension{in.sendNTP >> 14}
 		b, err := send.Marshal()
 		if err != nil {
 			t.Fatal(err)
diff --git a/packet_test.go b/packet_test.go
index 89eba2b..a5f502f 100644
--- a/packet_test.go
+++ b/packet_test.go
@@ -3,6 +3,7 @@ package rtp
 import (
 	"bytes"
 	"encoding/hex"
+	"errors"
 	"reflect"
 	"testing"
 )
@@ -247,6 +248,33 @@ func TestRFC8285OneByteMultipleExtensionsWithPadding(t *testing.T) {
 	if !bytes.Equal(ext3, ext3Expect) {
 		t.Errorf("Extension has incorrect data. Got: %v+, Expected: %v+", ext3, ext3Expect)
 	}
+
+	rawPktReMarshal := []byte{
+		0x90, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
+		0x27, 0x82, 0xBE, 0xDE, 0x00, 0x03, 0x10, 0xAA, 0x21, 0xBB,
+		0xBB, 0x33, 0xCC, 0xCC, 0xCC, 0xCC, 0x00, 0x00, // padding is moved to the end by re-marshaling
+		// Payload
+		0x98, 0x36, 0xbe, 0x88, 0x9e,
+	}
+	dstBuf := map[string][]byte{
+		"CleanBuffer": make([]byte, 1000),
+		"DirtyBuffer": make([]byte, 1000),
+	}
+	for i := range dstBuf["DirtyBuffer"] {
+		dstBuf["DirtyBuffer"][i] = 0xFF
+	}
+	for name, buf := range dstBuf {
+		buf := buf
+		t.Run(name, func(t *testing.T) {
+			n, err := p.MarshalTo(buf)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !bytes.Equal(buf[:n], rawPktReMarshal) {
+				t.Errorf("Marshal failed raw \nMarshaled:\n%s\nrawPkt:\n%s", hex.Dump(buf[:n]), hex.Dump(rawPktReMarshal))
+			}
+		})
+	}
 }
 
 func TestRFC8285OneByteMultipleExtensions(t *testing.T) {
@@ -920,6 +948,69 @@ func TestRFC3550SetExtensionShouldRaiseErrorWhenSettingNonzeroID(t *testing.T) {
 		t.Error("SetExtension did not error on invalid id")
 	}
 }
+
+func TestUnmarshal_ErrorHandling(t *testing.T) {
+	cases := map[string]struct {
+		input []byte
+		err   error
+	}{
+		"ShortHeader": {
+			input: []byte{
+				0x80, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, // SSRC (one byte missing)
+			},
+			err: errHeaderSizeInsufficient,
+		},
+		"MissingCSRC": {
+			input: []byte{
+				0x81, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+			},
+			err: errHeaderSizeInsufficient,
+		},
+		"MissingExtension": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+		"MissingExtensionData": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+				0xBE, 0xDE, 0x00, 0x03, // specified to have 3 extensions, but actually not
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+		"MissingExtensionDataPayload": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+				0xBE, 0xDE, 0x00, 0x01, // have 1 extension
+				0x12, 0x00, // length of the payload is expected to be 3, but actually have only 1
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+	}
+
+	for name, testCase := range cases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			h := &Header{}
+			err := h.Unmarshal(testCase.input)
+			if !errors.Is(err, testCase.err) {
+				t.Errorf("Expected error: %v, got: %v", testCase.err, err)
+			}
+		})
+	}
+}
+
 func TestRoundtrip(t *testing.T) {
 	rawPkt := []byte{
 		0x00, 0x10, 0x23, 0x45, 0x12, 0x34, 0x45, 0x67, 0xCC, 0xDD, 0xEE, 0xFF,
diff --git a/packetizer_test.go b/packetizer_test.go
index 8036d93..cf57351 100644
--- a/packetizer_test.go
+++ b/packetizer_test.go
@@ -2,7 +2,9 @@ package rtp
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
+	"time"
 
 	"github.com/pion/rtp/codecs"
 )
@@ -21,3 +23,47 @@ func TestPacketizer(t *testing.T) {
 		t.Fatalf("Generated %d packets instead of 2\n%s", len(packets), packetlengths)
 	}
 }
+
+func TestPacketizer_AbsSendTime(t *testing.T) {
+	//use the G722 payloader here, because it's very simple and all 0s is valid G722 data.
+	pktizer := NewPacketizer(100, 98, 0x1234ABCD, &codecs.G722Payloader{}, NewFixedSequencer(1234), 90000)
+	pktizer.(*packetizer).Timestamp = 45678
+	pktizer.(*packetizer).timegen = func() time.Time {
+		return time.Date(1985, time.June, 23, 4, 0, 0, 0, time.FixedZone("UTC-5", -5*60*60))
+		// (0xa0c65b1000000000>>14) & 0xFFFFFF  = 0x400000
+	}
+	pktizer.EnableAbsSendTime(1)
+
+	payload := []byte{0x11, 0x12, 0x13, 0x14}
+	packets := pktizer.Packetize(payload, 2000)
+
+	expected := &Packet{
+		Header: Header{
+			Version:          2,
+			Padding:          false,
+			Extension:        true,
+			Marker:           true,
+			PayloadOffset:    0, // not set by Packetize() at now
+			PayloadType:      98,
+			SequenceNumber:   1234,
+			Timestamp:        45678,
+			SSRC:             0x1234ABCD,
+			CSRC:             nil,
+			ExtensionProfile: 0xBEDE,
+			Extensions: []Extension{
+				{
+					id:      1,
+					payload: []byte{0x40, 0, 0},
+				},
+			},
+		},
+		Payload: []byte{0x11, 0x12, 0x13, 0x14},
+	}
+
+	if len(packets) != 1 {
+		t.Fatalf("Generated %d packets instead of 1", len(packets))
+	}
+	if !reflect.DeepEqual(expected, packets[0]) {
+		t.Errorf("Packetize failed\nexpected: %v\n     got: %v", expected, packets[0])
+	}
+}