Remove incorrect one-past-end distinction in memOutOfBounds#2029
Merged
Conversation
The memOutOfBounds analysis warns about invalid dereferences. Dereferencing one past end is never valid.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes memOutOfBounds’s offset-vs-size comparison so that dereferences at exactly one-past-end are no longer treated as safe, and extends the regression suite to cover one-past-end and beyond-end pointer cases (including implicit dereference contexts like printf arguments).
Changes:
- Remove the special-case distinction between dereference-offset and pointer-offset comparisons by using a single
size <= offsetout-of-bounds check. - Update
memOutOfBoundscall sites to use the unified offset-bounds helper. - Expand the
36-one-past-pointer.cregression test with multiplebuf + kscenarios and expected WARN/NOWARN annotations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/analyses/memOutOfBounds.ml |
Unifies offset bound checking to flag one-past-end as out-of-bounds for invalid dereference detection. |
tests/regression/74-invalid_deref/36-one-past-pointer.c |
Adds regression coverage for one-past-end and beyond-end pointer cases using printf calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
According to an sv-benchmarks run with level01, 60s and 1GB, this (surprisingly) makes no difference to verdicts. This probably explains why this unnecessary distinction flew under the radar. |
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.
This addresses the finding from #2017 (comment).
The memOutOfBounds analysis warns about invalid dereferences. Dereferencing one past end is never valid.
While adding the test I discovered so much more wrong about memOutOfBounds which in some cases completely ignores the offset altogether. I found this because initially I had hard time constructing a test where the off-by-one aspect even made a difference.
There's also a whole other rabbit hole that comes up with this test:
%pprints the pointer itself (not what it points to), so there's never a need to warn about out of bounds for that because it's not even dereferenced. Thus, the added tests use%swhich actually dereferences. However, we don't have an analysis for the format strings, so we won't be able to not warn for%p. Currently the analysis did warn there but for three wrong reasons combined.This is orthogonal to #2027: the point here is to simplify the existing thing (and make it correct!) such that the refactoring there doesn't need to preserve an artificial and incorrect distinction.