Repository: pion/sctp
Pull request #301 (2024-02-23 12:04:46)

Consider padding when bundling chunks in packets
Currently padding is not considered when decided how many chunks to bundle in to 1 packet. This causes some packets to exceed the MTU size. See the added test for a repro. 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/sctp/pull/301?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 80.63%. Comparing base [(`932b71a`)](https://app.codecov.io/gh/pion/sctp/commit/932b71a772d275c971727d022f0fe55b7916f7e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`22d4ebc`)](https://app.codecov.io/gh/pion/sctp/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Report is 1 commits behind head on master.


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


```diff
@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   80.56%   80.63%   +0.07%     
==========================================
  Files          49       49              
  Lines        4121     4126       +5     
==========================================
+ Hits         3320     3327       +7     
+ Misses        654      653       -1     
+ Partials      147      146       -1     
```

| [Flag](https://app.codecov.io/gh/pion/sctp/pull/301/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/301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `80.63% <100.00%> (+0.07%)` | :arrow_up: |
| [wasm](https://app.codecov.io/gh/pion/sctp/pull/301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `66.94% <100.00%> (+0.04%)` | :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/301?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/association.go b/association.go
index cf493ef..5ec597e 100644
--- a/association.go
+++ b/association.go
@@ -2238,14 +2238,15 @@ func (a *Association) bundleDataChunksIntoPackets(chunks []*chunkPayloadData) []
 		//   single packet.  Furthermore, DATA chunks being retransmitted MAY be
 		//   bundled with new DATA chunks, as long as the resulting packet size
 		//   does not exceed the path MTU.
-		if bytesInPacket+len(c.userData) > int(a.MTU()) {
+		chunkSizeInPacket := int(dataChunkHeaderSize) + len(c.userData)
+		chunkSizeInPacket += getPadding(chunkSizeInPacket)
+		if bytesInPacket+chunkSizeInPacket > int(a.MTU()) {
 			packets = append(packets, a.createPacket(chunksToSend))
 			chunksToSend = []chunk{}
 			bytesInPacket = int(commonHeaderSize)
 		}
-
 		chunksToSend = append(chunksToSend, c)
-		bytesInPacket += int(dataChunkHeaderSize) + len(c.userData)
+		bytesInPacket += chunkSizeInPacket
 	}
 
 	if len(chunksToSend) > 0 {


Test patch:
diff --git a/association_test.go b/association_test.go
index 2b6fb73..a12503d 100644
--- a/association_test.go
+++ b/association_test.go
@@ -3156,3 +3156,19 @@ func TestAssociation_ZeroChecksum(t *testing.T) {
 		require.NoError(t, a2.Close())
 	}
 }
+
+func TestDataChunkBundlingIntoPacket(t *testing.T) {
+	a := &Association{mtu: initialMTU}
+	chunks := make([]*chunkPayloadData, 300)
+	for i := 0; i < 300; i++ {
+		chunks[i] = &chunkPayloadData{userData: []byte{1}}
+	}
+	packets := a.bundleDataChunksIntoPackets(chunks)
+	for _, p := range packets {
+		raw, err := p.marshal(false)
+		require.NoError(t, err)
+		if len(raw) > int(initialMTU) {
+			t.Error("packet too long: ", len(raw))
+		}
+	}
+}