Skip to content

Fix spurious overflow checks in memOutOfBounds - #2079

Open
sim642 wants to merge 5 commits into
masterfrom
issue-1801
Open

sim642 wants to merge 5 commits into
masterfrom
issue-1801

Conversation

@sim642

@sim642 sim642 commented Jul 22, 2026

Copy link
Copy Markdown
Member

Closes #1801.

This is currently on top of #2077 because it (and some previous memOutOfBounds changes) already remove many of the ID operations from memOutOfBounds.

@sim642 sim642 added this to the v2.9.0 milestone Jul 22, 2026
@sim642 sim642 added bug sv-comp SV-COMP (analyses, results), witnesses precision pr-dependency Depends or builds on another PR, which should be merged before labels Jul 22, 2026
Base automatically changed from issue-2069 to master July 23, 2026 07:08
@sim642 sim642 removed the pr-dependency Depends or builds on another PR, which should be merged before label Jul 23, 2026
@michael-schwarz

Copy link
Copy Markdown
Member

This should come with a regression test.

@sim642

sim642 commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

This should come with a regression test.

The example in #1801 is actually already solved by some other PR which removed the corresponding ID operation in memOutOfBounds.

Adding tests for the remaining instances fixes in this PR turned out to be surprisingly difficult because the multiplication case involves array length, which CIL still returns as OCaml int (and throws otherwise), which implicitly sets a bound.
Luckily with a large-enough array element size the overflow still happens (and the warning is removed by this PR).

The size multiplication overflowing to top has an interesting consequence though: according to the interval, the size of something may also be negative. But I'm not sure it'd even make a difference to refine it to be non-negative: it could still be 0, so access at index 0 would be invalid, and access at negative index would be invalid anyway.

Comment on lines +29 to +30
arr[-1] = 42; // TODO WARN! (OOB)
*(arr - 1) = 42; // TODO WARN! (OOB)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These reveal unsoundness in memOutOfBounds. I think we just happen to be covered by ana.arrayoob for these very basic things, but there's no reason why memOutOfBounds shouldn't be able to handle these correctly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it was a deliberate choice to use ana.arrayoob for this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's hard to draw the line between that ana.arrayoob is supposed to do and what memOutOfBounds is supposed to do. And it's even harder to make sure that they actually cover everything with such a split.

I think memOutOfBounds should still be able to warn about these because it needs to be able to do the simple stuff anyway. Trying to avoid that and only handling the complex stuff is probably what makes memOutOfBounds so involved and error-prone.

ana.arrayoob also shoots us in the foot a lot with the single-element calloc arrays: #1766. Potentially all of memOutOfBounds could be done in base directly, like ana.arrayoob but that would just make base even more complicated.
We could still keep ana.arrayoob around but not use it with memOutOfBounds (if it did handle this stuff).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug precision sv-comp SV-COMP (analyses, results), witnesses

Projects

None yet

Development

Successfully merging this pull request may close these issues.

memOutOfBounds analysis causes integer overflow

2 participants