automata: check for overflow in Span::offset - #1380
Open
tautschnig wants to merge 2 commits into
Open
tautschnig wants to merge 2 commits into
tautschnig wants to merge 2 commits into
Conversation
Span is documented as unconstrained ("There are no constraints on the
values of a span") and has public fields, but Span::offset performed
unchecked additions: with debug assertions it panics with 'attempt to add
with overflow', and in release builds it silently produces a wrapped,
nonsensical span (e.g. 0..0 from a span at usize::MAX offset by 1).
Use checked_add and document the panic, mirroring the equivalent fix in
aho-corasick (BurntSushi/aho-corasick@0f3f5da).
Found by running Kani's autoharness (model-checking/kani#3832) over
regex-automata 0.4.16.
Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens regex-automata’s Span::offset against usize overflow by switching from unchecked addition to checked arithmetic and documenting the resulting panic behavior, preventing silent wraparound spans in release builds.
Changes:
- Add
# Panicsdocumentation toSpan::offsetdescribing overflow behavior. - Replace
start + offset/end + offsetwithchecked_add(...).expect(...)to ensure overflow triggers a deterministic panic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+857
to
+858
| .expect("invalid start+offset"), | ||
| end: self.end.checked_add(offset).expect("invalid end+offset"), |
Comment on lines
852
to
+853
| pub fn offset(&self, offset: usize) -> Span { | ||
| Span { start: self.start + offset, end: self.end + offset } | ||
| Span { |
…Span::offset Review feedback: mention overflow and the method in the expect messages, and add a unit test pinning the overflow panic. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
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.
Spanis documented as unconstrained ("There are no constraints on the values of a span. It is, for example, legal to create a span wherestart > end.") and has public fields, butSpan::offsetperforms unchecked additions. With debug assertions enabled it panics with the generic "attempt to add with overflow"; in release builds it silently produces a wrapped, nonsensical span:This PR uses
checked_addand documents the panic, mirroring the equivalent fix already applied to the same code in aho-corasick (BurntSushi/aho-corasick@0f3f5da, and BurntSushi/aho-corasick#182 for the remainingMatch::offset). regex-automata lib tests and the regex crate's test suite pass.Found by running Kani's autoharness (model-checking/kani#3832) over regex-automata 0.4.16; reproduced with plain cargo (no Kani involved) before filing.