Skip to content

fix: use wrapping arithmetic for position#2

Merged
HaveFunTrading merged 3 commits intoHaveFunTrading:mainfrom
Librazy:fix/wrapping-arithmetic
May 12, 2025
Merged

fix: use wrapping arithmetic for position#2
HaveFunTrading merged 3 commits intoHaveFunTrading:mainfrom
Librazy:fix/wrapping-arithmetic

Conversation

@Librazy
Copy link
Contributor

@Librazy Librazy commented May 7, 2025

Position Arithmetic Overflow

The ring buffer position calculations were using regular arithmetic operations which could overflow. Just like Linux's jiffies, the writer position ran out quickly on i686, and possible to exhaust x86_64's usize in reasonable time. This has been fixed by using wrapping arithmetic operations (wrapping_add and wrapping_sub) to handle position calculations safely.

It's still technically possible that the position wraps all around and make the overrun test to return false negative, but just like the jiffies, we can safely ignore this problem in any production environment.

Improved Writer Overrun Detection

Fixed overrun detection by checking after message construction but before reader position updates. This prevents issues where the frame header might be overwritten by the writer.

Additional Improvements

  • Added reset method to Reader for recovering from overrun conditions
  • Improved error handling in example code to handle overrun conditions gracefully
  • Added explicit position alignment checks for into_writer_at and with_initial_position
  • Added comprehensive tests for position wrap-around and reader overrun scenarios

@HaveFunTrading
Copy link
Owner

Thanks for raising the PR @Librazy I have added few comments. The wrapping_add stuff - happy to explore more if I you share your use case.

@Librazy
Copy link
Contributor Author

Librazy commented May 9, 2025

For the wrapping arithmetic, it's exactly the same as normal arithmetic if it never wraps around, and the + operator and - operator are wrapping by default on release mode and it panics when overflow occurs on debug. Using wrapping_* will not changes the behavior but just make it clear.

@HaveFunTrading
Copy link
Owner

for the wrapping arithmetic, it's exactly the same if it never wraps around, and the + operator and - operator are wrapping by default on release mode and it panics when overflow occurs on debug. Using wrapping_* will not changes the behavior but just make it clear.

fair point; I might need to revise it at some point and split position into lap_count: u32 and lap_position: u64 to ensure we never loose the unique position within the stream; alternatively we can change to checked_add to prevent position overflow

@HaveFunTrading HaveFunTrading merged commit b1cca69 into HaveFunTrading:main May 12, 2025
4 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.

2 participants