Repository: pion/sctp
Pull request #311 (2024-02-28 16:29:18)

Fix incorrect fragmented chunk merge
#### Description
This fixes a case where a chunk with the same SSN that is not fragmented is combined with a fragmented chunk. This can happen in a case where we send so many chunks that the SSN wraps.



codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/sctp/pull/311?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 67.21%. Comparing base [(`32ef4a1`)](https://app.codecov.io/gh/pion/sctp/commit/32ef4a16c61db9e4d23750d2d09482f5c1681830?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`f58d621`)](https://app.codecov.io/gh/pion/sctp/pull/311?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

> :exclamation: Current head f58d621 differs from pull request most recent head 854730f. Consider uploading reports for the commit 854730f to get more accurate results


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


```diff
@@             Coverage Diff             @@
##           master     #311       +/-   ##
===========================================
- Coverage   80.77%   67.21%   -13.57%     
===========================================
  Files          49       49               
  Lines        4135     4145       +10     
===========================================
- Hits         3340     2786      -554     
- Misses        651     1200      +549     
- Partials      144      159       +15     
```

| [Flag](https://app.codecov.io/gh/pion/sctp/pull/311/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/sctp/pull/311/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/sctp/pull/311/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `67.21% <100.00%> (+0.07%)` | :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/sctp/pull/311?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).


Patch:
diff --git a/chunk_payload_data.go b/chunk_payload_data.go
index b6f1b61..a6e50db 100644
--- a/chunk_payload_data.go
+++ b/chunk_payload_data.go
@@ -206,3 +206,7 @@ func (p *chunkPayloadData) setAllInflight() {
 		}
 	}
 }
+
+func (p *chunkPayloadData) isFragmented() bool {
+	return !(p.head == nil && p.beginningFragment && p.endingFragment)
+}
diff --git a/reassembly_queue.go b/reassembly_queue.go
index c23e699..e0d527c 100644
--- a/reassembly_queue.go
+++ b/reassembly_queue.go
@@ -155,11 +155,22 @@ func (r *reassemblyQueue) push(chunk *chunkPayloadData) bool {
 		return false
 	}
 
-	// Check if a chunkSet with the SSN already exists
-	for _, set := range r.ordered {
-		if set.ssn == chunk.streamSequenceNumber {
-			cset = set
-			break
+	// Check if a fragmented chunkSet with the fragmented SSN already exists
+	if chunk.isFragmented() {
+		for _, set := range r.ordered {
+			// nolint:godox
+			// TODO: add caution around SSN wrapping here... this helps only a little bit
+			// by ensuring we don't add to an unfragmented cset (1 chunk). There's
+			// a case where if the SSN does wrap around, we may see the same SSN
+			// for a different chunk.
+
+			// nolint:godox
+			// TODO: this slice can get pretty big; it may be worth maintaining a map
+			// for O(1) lookups at the cost of 2x memory.
+			if set.ssn == chunk.streamSequenceNumber && set.chunks[0].isFragmented() {
+				cset = set
+				break
+			}
 		}
 	}
 


Test patch:
diff --git a/association_test.go b/association_test.go
index ed53ed7..8a50cf2 100644
--- a/association_test.go
+++ b/association_test.go
@@ -265,7 +265,10 @@ func createNewAssociationPair(br *test.Bridge, ackMode int, recvBufSize uint32)
 		handshake0Ch <- true
 	}()
 	go func() {
-		a1, err1 = Client(Config{
+		// we could have two "client"s here but it's more
+		// standard to have one peer starting initialization and
+		// another waiting for the initialization to be requested (INIT).
+		a1, err1 = Server(Config{
 			Name:                 "a1",
 			NetConn:              br.GetConn1(),
 			MaxReceiveBufferSize: recvBufSize,
diff --git a/reassembly_queue_test.go b/reassembly_queue_test.go
index 91fd530..02478f4 100644
--- a/reassembly_queue_test.go
+++ b/reassembly_queue_test.go
@@ -464,6 +464,44 @@ func TestReassemblyQueue(t *testing.T) {
 		assert.Equal(t, 1, len(rq.unorderedChunks), "there should be one chunk kept")
 		assert.Equal(t, 3, rq.getNumBytes(), "num bytes mismatch")
 	})
+
+	t.Run("fragmented and unfragmented chunks with the same ssn", func(t *testing.T) {
+		rq := newReassemblyQueue(0)
+
+		orgPpi := PayloadTypeWebRTCBinary
+
+		var chunk *chunkPayloadData
+		var complete bool
+		var ssn uint16 = 6
+
+		chunk = &chunkPayloadData{
+			payloadType:          orgPpi,
+			tsn:                  12,
+			beginningFragment:    true,
+			endingFragment:       true,
+			streamSequenceNumber: ssn,
+			userData:             []byte("DEF"),
+		}
+
+		complete = rq.push(chunk)
+		assert.True(t, complete, "chunk set should be complete")
+		assert.Equal(t, 3, rq.getNumBytes(), "num bytes mismatch")
+
+		chunk = &chunkPayloadData{
+			payloadType:          orgPpi,
+			beginningFragment:    true,
+			tsn:                  11,
+			streamSequenceNumber: ssn,
+			userData:             []byte("ABC"),
+		}
+
+		complete = rq.push(chunk)
+		assert.False(t, complete, "chunk set should not be complete yet")
+		assert.Equal(t, 6, rq.getNumBytes(), "num bytes mismatch")
+
+		assert.Equal(t, 2, len(rq.ordered), "there should be two chunks")
+		assert.Equal(t, 6, rq.getNumBytes(), "num bytes mismatch")
+	})
 }
 
 func TestChunkSet(t *testing.T) {