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

Fix uninit read benchmark #641

Merged
merged 3 commits into from
Mar 23, 2024
Merged

Conversation

ThomasHaas
Copy link
Collaborator

This fixes the uninitRead.c benchmark to work independent of the RemoveUnusedMemory pass and adds a clarifying warning that this test is about accessing uninitialized memory and not initialized memory via overflows (we do not handle the latter correctly).

@ThomasHaas
Copy link
Collaborator Author

Hmm, maybe it is better to have a malloc and read from the malloc`d memory without initialization. That might be more principled.

@hernanponcedeleon
Copy link
Owner

Hmm, maybe it is better to have a malloc and read from the malloc`d memory without initialization. That might be more principled.

Yes, I like this test rather than having that magic number constant in the array access

@ThomasHaas
Copy link
Collaborator Author

I changed the test. Now it doesn't distinguish between the previous Dartagnan version and the new one though.

@hernanponcedeleon
Copy link
Owner

I think there are two scenarios around the issue we discussed

  • accessing allocated, but uninitiated memory
  • accessing out-of-bound memory (independently if it was properly initialized by the allocator)

My understanding is that we properly handle (and did this before #624) the former and the benchmark in this PR shows this.

However, we do not properly handle the second situation (sometimes we do "by luck"). There are several things that influence this (e.g. does the address of the out-of-bound access matches the address or another object, are we using Caat or the eager encoding), but the culprit is that our alias analyses are unsound (by design) in the presence of out-of-bound accesses.

Since we only fully support one of the scenarios, it makes sense to me that the benchmark is about that one.

@ThomasHaas
Copy link
Collaborator Author

Since we only fully support one of the scenarios, it makes sense to me that the benchmark is about that one.

Yes, it makes sense.

  • accessing allocated, but uninitiated memory
  • ...

My understanding is that we properly handle (and did this before #624) the former and the benchmark in this PR shows this.

We did handle the former only because we created init events. If we had not created them (as we do for out-of-bounds memory) we would have erroneously returned PASS. Now we also handle it without creating init events. It's just that the test cannot distinguish "old version + inits" vs. "new version + no inits". It distinguishes from "old version + no inits" but that combination only happened on out-of-bounds memory accesses and that's why I had it initially like this.

Either way, it's still a simple and valid test.

@hernanponcedeleon hernanponcedeleon merged commit abafd9c into development Mar 23, 2024
1 check passed
@hernanponcedeleon hernanponcedeleon deleted the fix_uninitRead_benchmark branch March 23, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants