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

Inject pointer validity check when casting raw pointers to references #3221

Merged
merged 12 commits into from
Jun 7, 2024

Conversation

artemagvanian
Copy link
Contributor

@artemagvanian artemagvanian commented Jun 3, 2024

Resolves #2975

This PR makes Kani able to check if a raw pointer is valid before casting it to a reference.

To check for the pointer validity when casting it to a reference, inject asserting __CPROVER_r_ok(ptr, sz) into places where a raw pointer is dereferenced and a reference is immediately taken.

Since this check seems to cause some of the CIs to run out of memory, it is only enabled under -Z ptr-to-ref-cast-checks flag.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@github-actions github-actions bot added the Z-BenchCI Tag a PR to run benchmark CI label Jun 3, 2024
@artemagvanian artemagvanian marked this pull request as ready for review June 5, 2024 15:00
@artemagvanian artemagvanian requested a review from a team as a code owner June 5, 2024 15:00
@tautschnig
Copy link
Member

tautschnig commented Jun 5, 2024

I'm opposed to adding yet another flag for this adds more cognitive load for users. My understanding of the rationale for this flag is that the extra checks made a single test go out-of-memory in GitHub CI, but that test is exactly the one discussed in #3030: this test already is very close to running OOM and could really go over the limit with about any change, say a toolchain upgrade. My proposal is to disable this one test for the time being and instead independently investigate how to make it cheaper.

@artemagvanian
Copy link
Contributor Author

That makes sense given the fact that the harness was already close to running OOM. I disabled the test by changing the expected file to take into account its failure. Not sure I know of a better way to do it, but happy to learn.

@artemagvanian
Copy link
Contributor Author

Instead of changing the expected file, I added the copies of the files without the problematic harness into the overlay directory, so the harness will not run on the CI.

Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

Great work, @artemagvanian!

We need to avoid overriding the files in s2n-quic, as the regression is meant to ensure we don't break our customers' CI.

kani-compiler/src/codegen_cprover_gotoc/codegen/assert.rs Outdated Show resolved Hide resolved
tests/expected/ptr_to_ref_cast/expected Outdated Show resolved Hide resolved
tests/expected/ptr_to_ref_cast/expected Outdated Show resolved Hide resolved
@artemagvanian artemagvanian merged commit ffcb5ad into model-checking:main Jun 7, 2024
23 checks passed
@artemagvanian artemagvanian self-assigned this Jul 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2024
…and str's (#3513)

As pointed out in #3498, validity checks for pointer to reference casts
(added in #3221) were not instrumented in the case of fat pointers (e.g.
array and string slices). This PR extends the instrumentation of
validity checks to handle those cases.

Resolves #3498 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.

---------

Co-authored-by: Celina G. Val <celinval@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-BenchCI Tag a PR to run benchmark CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for pointer validity when casting raw pointer to reference.
3 participants