Skip to content

Fixed packet length checks#220

Merged
sirzooro merged 1 commit into
pion:mainfrom
sirzooro:fix_length_checks
Jun 30, 2026
Merged

Fixed packet length checks#220
sirzooro merged 1 commit into
pion:mainfrom
sirzooro:fix_length_checks

Conversation

@sirzooro

Copy link
Copy Markdown
Contributor

No description provided.

@sirzooro sirzooro requested a review from JoTurk June 21, 2026 11:04
@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.50%. Comparing base (d61b52c) to head (f7f5e58).

Files with missing lines Patch % Lines
full_intra_request.go 83.33% 0 Missing and 1 partial ⚠️
transport_layer_cc.go 66.66% 0 Missing and 1 partial ⚠️
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              
Flag Coverage Δ
go 77.50% <88.23%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of int(4*header.Length).
  • Normalize total-length calculations to int in 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 == totalLength is still valid (slice high bound can equal totalLength). Using >= can incorrectly reject packets where the final chunk ends exactly at totalLength (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 JoTurk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for losing track of this, I double checked all the correctness and they look good to me, can we add a few tests?

@sirzooro sirzooro force-pushed the fix_length_checks branch from edb2425 to b200492 Compare June 30, 2026 06:11
@sirzooro sirzooro force-pushed the fix_length_checks branch from b200492 to f7f5e58 Compare June 30, 2026 06:19
@sirzooro

Copy link
Copy Markdown
Contributor Author

Sorry for losing track of this, I double checked all the correctness and they look good to me, can we add a few tests?

Thanks, added new tests.

@sirzooro sirzooro merged commit a9b71ae into pion:main Jun 30, 2026
18 checks passed
@sirzooro sirzooro deleted the fix_length_checks branch June 30, 2026 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants