Fixed packet length checks#220
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
==========================================
+ Coverage 77.40% 77.50% +0.10%
==========================================
Files 22 22
Lines 2031 2032 +1
==========================================
+ Hits 1572 1575 +3
+ Misses 362 360 -2
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
caf9a96 to
edb2425
Compare
There was a problem hiding this comment.
Pull request overview
This PR corrects RTCP packet-length computations/checks by ensuring Header.Length is converted to int before multiplication/addition, avoiding overflow/truncation in several Unmarshal/Marshal paths.
Changes:
- Fix length comparisons/loop bounds by using
4*int(header.Length)(and similar) instead ofint(4*header.Length). - Normalize total-length calculations to
intin TransportLayerCC and CCFeedbackReport. - Minor cleanup of int conversions in generic packet unmarshalling and APP length validation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| transport_layer_nack.go | Fixes length checks/loop bounds to avoid overflow when expanding RTCP Length to bytes. |
| transport_layer_cc.go | Uses int for computed packet sizes/positions to prevent overflow/truncation in bounds checks. |
| slice_loss_indication.go | Fixes length checks/loop bounds to avoid overflow when expanding RTCP Length to bytes. |
| rfc8888.go | Computes Marshal buffer length using int(header.Length) to prevent overflow/truncation. |
| packet.go | Computes processed byte count using int(header.Length) before scaling. |
| full_intra_request.go | Fixes FIR length validation and loop bounds with safe int conversions. |
| application_defined.go | Fixes APP declared-length validation using safe int conversion order. |
Comments suppressed due to low confidence (1)
transport_layer_cc.go:501
- The bounds check for reading a 2-byte packet status chunk is off by one:
packetStatusPos+packetStatusChunkLength == totalLengthis still valid (slice high bound can equaltotalLength). Using>=can incorrectly reject packets where the final chunk ends exactly attotalLength(e.g., two chunks and no recv deltas/padding beyond the declared length).
if packetStatusPos+packetStatusChunkLength >= totalLength {
return errPacketTooShort
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JoTurk
left a comment
There was a problem hiding this comment.
Sorry for losing track of this, I double checked all the correctness and they look good to me, can we add a few tests?
edb2425 to
b200492
Compare
b200492 to
f7f5e58
Compare
Thanks, added new tests. |
No description provided.