od: fix overflow panic in --traditional label accumulator#13241
Open
koopatroopa787 wants to merge 1 commit into
Open
od: fix overflow panic in --traditional label accumulator#13241koopatroopa787 wants to merge 1 commit into
koopatroopa787 wants to merge 1 commit into
Conversation
`InputOffset::increase_position` used plain `+=` / `+` for the running byte position and user-supplied label, which panics in debug builds (or any build with `-C overflow-checks=on`) when the label is near `u64::MAX`. C unsigned arithmetic wraps on overflow, and GNU od silently wraps in the same situation. Switch to `wrapping_add` for both fields to match GNU's behaviour and eliminate the debug-mode panic reported in uutils#13225. Adds a unit test that exercises the wrap-around path directly. Fixes uutils#13225
There was a problem hiding this comment.
Pull request overview
This PR fixes an overflow panic in od --traditional offset/label accumulation by switching the internal byte position and optional label arithmetic to wrapping semantics, matching GNU od’s unsigned overflow behavior.
Changes:
- Use
wrapping_addforInputOffset::increase_positionupdates tobyte_posandlabel. - Add a regression unit test to ensure near-
u64::MAXlabels don’t panic and wrap correctly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+100
to
+104
| // A label of u64::MAX must not panic — it wraps, matching C unsigned semantics. | ||
| let mut sut = InputOffset::new(Radix::Hexadecimal, 0, Some(u64::MAX)); | ||
| sut.increase_position(1); // would panic in debug builds before the fix | ||
| assert_eq!(sut.label, Some(0)); | ||
| } |
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InputOffset::increase_positionused plain+=/+for the runningbyte position and user-supplied
--traditionallabel. In debug builds(or any build compiled with
-C overflow-checks=on) a label nearu64::MAXcauses an "attempt to add with overflow" panic on the veryfirst read:
GNU
oduses Cunsigned longarithmetic, which silently wraps onoverflow. The fix switches both fields to
wrapping_addto match thatbehaviour and eliminate the panic.
Changes
src/uu/od/src/input_offset.rs:increase_positionnow useswrapping_addforbyte_posandlabel.test_increase_position_wraps_on_overflowunit test that wouldhave panicked before this fix.
Fixes #13225