Repository: pion/dtls
Pull request #261 (2020-06-01 09:14:05)

Fragmentbuffer: Fix fragment parsing
#### Description

This commit is an attempt to support multiple handshake messages in single dtls record fragment.

DTLS connection failure happened when I was trying to connect to emqx coap dtls port, according to the wireshark capture result, the emqx server was sending multiple handshake messages in single dtls record, which resulted handshake unmarshal error in this library, and as noted in the last paragraph of https://tools.ietf.org/html/rfc6347#section-4.2.3

> multiple handshake messages may be placed in the same DTLS record,
> provided that there is room and that they are part of the same flight.

So I believe we should support such feature (also bug fix?)

#### Reference issue

none

/cc @at-wat , @Sean-Der 

*I have read https://github.com/pion/webrtc/wiki/Contributing#writing-a-good-pr but didn't try to find a reviewer, hope that's ok :)

jeffreystoke: seems the `.github/lint-disallowed-functions-in-library.sh` is flaky, I tried to run it on my local machine, the result was always correct, but CI says no
daenney: The tests aren't failing right now on disallowed-functions though, but:

```
Please add the following contributors to the README
Jeff Ctor

The command "bash .github/assert-contributors.sh" exited with 1.
```

It's confused about your name.
jeffreystoke: Hi @daenney , sorry, I pasted the wrong script name, it's actually `.github/assert-contributors.sh`, but the I'm not sure if it's something related to git, but if I run that script locally, the output is correct everytime, and here is the git output

```
# git log --format='%aN' | sort -u | grep Jeff
Jeffrey Stoke
```

daenney: Mm, peculiar. If I look you up in Travis CI, you show up as Jeffrey Cstor. Which might be why: https://travis-ci.org/github/jeffreystoke
jeffreystoke: but if you look at this [build](https://travis-ci.org/github/pion/dtls/jobs/693381203), it actually found the correct name, I will try to push it again.
daenney: Hmmm indeed. Going to summon @Sean-Der here.
codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/261?src=pr&el=h1) Report
> Merging [#261](https://codecov.io/gh/pion/dtls/pull/261?src=pr&el=desc) into [master](https://codecov.io/gh/pion/dtls/commit/6dc4ee3f3fbeb2fae635aa327b634e5cf9eaf8d8&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `81.81%`.

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

```diff
@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   77.75%   77.78%   +0.02%     
==========================================
  Files          76       76              
  Lines        4055     4060       +5     
==========================================
+ Hits         3153     3158       +5     
  Misses        622      622              
  Partials      280      280              
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `77.82% <81.81%> (+0.02%)` | :arrow_up: |
| #wasm | `72.55% <81.81%> (+0.03%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/261?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [fragment\_buffer.go](https://codecov.io/gh/pion/dtls/pull/261/diff?src=pr&el=tree#diff-ZnJhZ21lbnRfYnVmZmVyLmdv) | `87.50% <81.81%> (+1.45%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/261?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/dtls/pull/261?src=pr&el=footer). Last update [6dc4ee3...c6591e3](https://codecov.io/gh/pion/dtls/pull/261?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Sean-Der: Fantastic work @jeffreystoke, merging!

Sorry about the busted CI @daenney :( 

Patch:
diff --git a/README.md b/README.md
index 6ce930d..aebbb39 100644
--- a/README.md
+++ b/README.md
@@ -141,6 +141,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [Julien Salleyron](https://github.com/juliens) - *Server Name Indication*
 * [Jeroen de Bruijn](https://github.com/vidavidorra)
 * [bjdgyc](https://github.com/bjdgyc)
+* [Jeffrey Stoke (Jeff Ctor)](https://github.com/jeffreystoke) - *Fragmentbuffer Fix*
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text
diff --git a/fragment_buffer.go b/fragment_buffer.go
index 95f9d47..41e3fc7 100644
--- a/fragment_buffer.go
+++ b/fragment_buffer.go
@@ -31,17 +31,27 @@ func (f *fragmentBuffer) push(buf []byte) (bool, error) {
 		return false, nil
 	}
 
-	if err := frag.handshakeHeader.Unmarshal(buf[recordLayerHeaderSize:]); err != nil {
-		return false, err
-	}
+	for buf = buf[recordLayerHeaderSize:]; len(buf) != 0; frag = new(fragment) {
+		if err := frag.handshakeHeader.Unmarshal(buf); err != nil {
+			return false, err
+		}
 
-	if _, ok := f.cache[frag.handshakeHeader.messageSequence]; !ok {
-		f.cache[frag.handshakeHeader.messageSequence] = []*fragment{}
-	}
+		if _, ok := f.cache[frag.handshakeHeader.messageSequence]; !ok {
+			f.cache[frag.handshakeHeader.messageSequence] = []*fragment{}
+		}
+
+		// end index should be the length of handshake header but if the handshake
+		// was fragmented, we should keep them all
+		end := int(handshakeHeaderLength + frag.handshakeHeader.length)
+		if size := len(buf); end > size {
+			end = size
+		}
 
-	// Discard all headers, when rebuilding the packet we will re-build
-	frag.data = append([]byte{}, buf[recordLayerHeaderSize+handshakeHeaderLength:]...)
-	f.cache[frag.handshakeHeader.messageSequence] = append(f.cache[frag.handshakeHeader.messageSequence], frag)
+		// Discard all headers, when rebuilding the packet we will re-build
+		frag.data = append([]byte{}, buf[handshakeHeaderLength:end]...)
+		f.cache[frag.handshakeHeader.messageSequence] = append(f.cache[frag.handshakeHeader.messageSequence], frag)
+		buf = buf[end:]
+	}
 
 	return true, nil
 }


Test patch:
diff --git a/fragment_buffer_test.go b/fragment_buffer_test.go
index 6b19e64..62c7ead 100644
--- a/fragment_buffer_test.go
+++ b/fragment_buffer_test.go
@@ -9,7 +9,7 @@ func TestFragmentBuffer(t *testing.T) {
 	for _, test := range []struct {
 		Name     string
 		In       [][]byte
-		Expected []byte
+		Expected [][]byte
 		Epoch    uint16
 	}{
 		{
@@ -17,16 +17,20 @@ func TestFragmentBuffer(t *testing.T) {
 			In: [][]byte{
 				{0x16, 0xfe, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0F, 0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
 			},
-			Expected: []byte{0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
-			Epoch:    0,
+			Expected: [][]byte{
+				{0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
+			},
+			Epoch: 0,
 		},
 		{
 			Name: "Single Fragment Epoch 3",
 			In: [][]byte{
 				{0x16, 0xfe, 0xff, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0F, 0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
 			},
-			Expected: []byte{0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
-			Epoch:    3,
+			Expected: [][]byte{
+				{0x03, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xfe, 0xff, 0x00},
+			},
+			Epoch: 3,
 		},
 		{
 			Name: "Multiple Fragments",
@@ -35,8 +39,10 @@ func TestFragmentBuffer(t *testing.T) {
 				{0x16, 0xfe, 0xfd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x81, 0x0b, 0x00, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x05, 0x05, 0x06, 0x07, 0x08, 0x09},
 				{0x16, 0xfe, 0xfd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x81, 0x0b, 0x00, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x0A, 0x00, 0x00, 0x05, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E},
 			},
-			Expected: []byte{0x0b, 0x00, 0x00, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e},
-			Epoch:    0,
+			Expected: [][]byte{
+				{0x0b, 0x00, 0x00, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e},
+			},
+			Epoch: 0,
 		},
 		{
 			Name: "Multiple Unordered Fragments",
@@ -45,8 +51,27 @@ func TestFragmentBuffer(t *testing.T) {
 				{0x16, 0xfe, 0xfd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x81, 0x0b, 0x00, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x0A, 0x00, 0x00, 0x05, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E},
 				{0x16, 0xfe, 0xfd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x81, 0x0b, 0x00, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x05, 0x05, 0x06, 0x07, 0x08, 0x09},
 			},
-			Expected: []byte{0x0b, 0x00, 0x00, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e},
-			Epoch:    0,
+			Expected: [][]byte{
+				{0x0b, 0x00, 0x00, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e},
+			},
+			Epoch: 0,
+		},
+		{
+			Name: "Multiple Handshakes in Signle Fragment",
+			In: [][]byte{
+				{
+					0x16, 0xfe, 0xfd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x30, /* record header */
+					0x03, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01, /*handshake msg 1*/
+					0x03, 0x00, 0x00, 0x04, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01, /*handshake msg 2*/
+					0x03, 0x00, 0x00, 0x04, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01, /*handshake msg 3*/
+				},
+			},
+			Expected: [][]byte{
+				{0x03, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01},
+				{0x03, 0x00, 0x00, 0x04, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01},
+				{0x03, 0x00, 0x00, 0x04, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0xfe, 0xff, 0x01, 0x01},
+			},
+			Epoch: 0,
 		},
 	} {
 		fragmentBuffer := newFragmentBuffer()
@@ -59,12 +84,14 @@ func TestFragmentBuffer(t *testing.T) {
 			}
 		}
 
-		out, epoch := fragmentBuffer.pop()
-		if !reflect.DeepEqual(out, test.Expected) {
-			t.Errorf("fragmentBuffer '%s' push/pop: got % 02x, want % 02x", test.Name, out, test.Expected)
-		}
-		if epoch != test.Epoch {
-			t.Errorf("fragmentBuffer returned wrong epoch: got %d, want %d", epoch, test.Epoch)
+		for _, expected := range test.Expected {
+			out, epoch := fragmentBuffer.pop()
+			if !reflect.DeepEqual(out, expected) {
+				t.Errorf("fragmentBuffer '%s' push/pop: got % 02x, want % 02x", test.Name, out, expected)
+			}
+			if epoch != test.Epoch {
+				t.Errorf("fragmentBuffer returned wrong epoch: got %d, want %d", epoch, test.Epoch)
+			}
 		}
 
 		if frag, _ := fragmentBuffer.pop(); frag != nil {