Skip to content

Use correct comparison in SACK reordering to avoid wrapping errors#55

Merged
gasbytes merged 1 commit intowolfSSL:masterfrom
danielinux:wrapping-in-sack-reordering
Mar 2, 2026
Merged

Use correct comparison in SACK reordering to avoid wrapping errors#55
gasbytes merged 1 commit intowolfSSL:masterfrom
danielinux:wrapping-in-sack-reordering

Conversation

@danielinux
Copy link
Member

Found via fenrir/145

Copy link
Contributor

Copilot AI left a comment

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 fixes wrap-around bugs in TCP SACK block normalization by switching SACK block ordering/merging comparisons to use the stack’s wrap-aware TCP sequence comparison helpers, and adds regression tests to cover the wrap case.

Changes:

  • Use tcp_seq_lt() in tcp_sort_sack_blocks() to sort SACK blocks with wrap-aware ordering.
  • Use tcp_seq_lt() / tcp_seq_leq() in tcp_merge_sack_blocks() to merge ranges correctly when sequence numbers wrap.
  • Add unit tests that validate sort/merge behavior across a wrap boundary and register them in the suite.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/wolfip.c Updates SACK sort/merge logic to use wrap-aware TCP sequence comparisons to avoid incorrect ordering/merging near 32-bit wrap.
src/test/unit/unit.c Adds and wires up unit tests that reproduce/guard the wrap-around ordering behavior for SACK sort/merge.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@gasbytes gasbytes left a comment

Choose a reason for hiding this comment

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

confirmed locally as usual with gh pr checkout, lgtm.

@gasbytes gasbytes merged commit 01228b1 into wolfSSL:master Mar 2, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants