Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SV-COMP "Memory Safety" benchmark additions #1201

Merged
merged 83 commits into from
Oct 14, 2023

Conversation

mrstanb
Copy link
Member

@mrstanb mrstanb commented Oct 2, 2023

This PR:

  • Builds upon changes from the https://github.com/goblint/analyzer/tree/staging_memsafety branch that was used for the first benchmark runs of Goblint against the SV-COMP "Memory Safety" benchmarks
  • Adds (first and hacky) support for recognizing all three memory safety properties of SV-COMP (i.e., valid-free, valid-deref, valid-memtrack)
  • Adds support for the valid-memcleanup property of SV-COMP

TODO:

michael-schwarz and others added 23 commits September 6, 2023 09:54
Make sure to do this only if we're in postsolving
@sim642 sim642 self-requested a review October 9, 2023 09:20
goblint.opam.locked Outdated Show resolved Hide resolved
src/analyses/memOutOfBounds.ml Outdated Show resolved Hide resolved
src/analyses/useAfterFree.ml Outdated Show resolved Hide resolved
Comment on lines 292 to 296
"addNestedScopeAttr": {
"title": "cil.addNestedScopeAttr",
"type": "boolean",
"description": "Indicates whether variables that CIL pulls out of their scope should be marked.",
"default": false
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be an option at all? There's no harm in having the attribute.

Copy link
Member

Choose a reason for hiding this comment

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

There is when you do justcil, as it will cause many compiler warnings and pollute files. If we want to keep using Goblint to create merged files, we should keep this option imo.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to me that it'd be better to make CIL's printer not just print these special attributes like it already does with some others.
That would avoid this annotation from ever making it to the output, even when you want to both enable it (for memory analyses) and also print the merged file. Currently those annotations would be in the merged file explicitly, so reanalyzing the merged file would incorrectly have some of these attributes on variables which are all pulled up to the function level. If the attribute wasn't printed, there wouldn't be a spurious attribute which would be incorrectly placed onto pulled-up variables.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about it, but decided against it in the end: the reasoning was that I want some way to inspect where these attributes were put.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe always adding them and then adding an option to CIL (and here) to disable or enable printing them would be a way to achieve everything @sim642?
If yes, I can do this in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, but I don't consider this to be a blocking thing. I just hoped we could get away without introducing new options that allow Goblint to be used in some unintended ways. E.g. enable memory analyses but disable this option (even by default).

Right now the automation is only in the autotuner that isn't enabled by default. So maybe our check_arguments should have a case for it to prevent this.

Copy link
Member

Choose a reason for hiding this comment

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

I added a corresponding case to check_arguments now.

@sim642
Copy link
Member

sim642 commented Oct 12, 2023

Due to the module moves in #1206 GitHub shows conflicts that are "too complex" to solve in the web editor. Actually you can locally just merge master into this branch and there shouldn't be any conflicts. GitHub's merge thing seems to have some move detection disabled that git should do by default.

@michael-schwarz michael-schwarz merged commit ca6fcba into goblint:master Oct 14, 2023
10 checks passed
michael-schwarz added a commit that referenced this pull request Oct 14, 2023
@sim642
Copy link
Member

sim642 commented Oct 31, 2023

I thought this was supposed to be sound, but apparently it's not. For example

./goblint --conf conf/svcomp.json --set ana.specification ../sv-benchmarks/c/properties/valid-memsafety.prp ../sv-benchmarks/c/loops/invert_string-2.c

prints SV-COMP result: true but the expected verdict if false(valid-deref).

@michael-schwarz
Copy link
Member

It was sound when I last checked it, is this task new?

@sim642
Copy link
Member

sim642 commented Oct 31, 2023

No, it's a random one I just picked to check. It was present in some BenchExec table that was attached here before and there it correctly returned unknown.

@michael-schwarz
Copy link
Member

Maybe the autotuner is just not active in the setting you have given where svcomp.enabled is not set? I think it's something along these lines, we didn't do any fundamental changes since the last run.

@sim642
Copy link
Member

sim642 commented Oct 31, 2023

conf/svcomp.json enables all of that though.

@michael-schwarz
Copy link
Member

svcomp23-with-mem-safety.json

That was the config we used.

@michael-schwarz
Copy link
Member

Maybe it's arrayoob that isn't set there?

@sim642
Copy link
Member

sim642 commented Oct 31, 2023

Maybe it's arrayoob that isn't set there?

That seems to be it. I made the autotuner enable it automatically like it activates the analyses.

Although I'm surprised that the memOutOfBounds analysis doesn't check these bounds.

sim642 added a commit to sim642/opam-repository that referenced this pull request Nov 24, 2023
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SV-Comp: Fix unsoundness in MemSafety category caused by limitations due to scope
3 participants