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.

Sign up for GitHub

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

Jump to bottom

Fix int-to-ptr casts in analysis/test #682

Open
kkysen opened this issue Sep 25, 2022 · 2 comments · May be fixed by #699
Open

Fix int-to-ptr casts in analysis/test #682

kkysen opened this issue Sep 25, 2022 · 2 comments · May be fixed by #699
Assignees
@kkysen

Comments

@kkysen
Copy link
Contributor

kkysen commented Sep 25, 2022

analysis/test contains many int-to-ptr casts for null pointers, like 0 as *const _ and 0 as *mut _. These use ptr::from_exposed_addr as part of strict provenance. Since we don't actually need to use ptr::from_exposed_addr here, we should switch to using str::ptr::null{,_mut}() instead. miri warns on these, so it's easier to find and fix UB bugs if these aren't in the way, and it's best practice to do this as well.

See the miri warning:

warning: integer-to-pointer cast
   --> src/pointers.rs:82:17
    |
82  |         field3: 0 as *const S,
    |                 ^^^^^^^^^^^^^ integer-to-pointer cast
    |
    = help: This program is using integer-to-pointer casts or (equivalently) `ptr::from_exposed_addr`,
    = help: which means that Miri might miss pointer bugs in this program.
    = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html for more details on that operation.
    = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead.
    = help: You can then pass the `-Zmiri-strict-provenance` flag to Miri, to ensure you are not relying on `from_exposed_addr` semantics.
    = help: Alternatively, the `-Zmiri-permissive-provenance` flag disables this warning.
    = note: backtrace:
    = note: inside `pointers::simple` at src/pointers.rs:82:17
This was referenced Sep 25, 2022
Closed
Merged
@kkysen kkysen self-assigned this Sep 25, 2022
@aneksteind
Copy link
Contributor

Similar to #683 (comment), it would be good to know if this is a result of the transpiler. I believe that should be the focus over changes to analysis/test specifically, if so.

kkysen added a commit that referenced this issue Oct 10, 2022
@kkysen
(#686) Make analysis/test miri-compatible and add a miri test for it
e657818 
Fixes #685.

This makes `analysis/test` `miri`-compatible when running with `--features miri`.  This is done by using a monomorphic `printf` shim, since `miri` can't handle variadic functions like `printf`.  Since all uses of `printf` in `analysis/test` are monomorphic (they all have the same format string), we can substitute a `fn printf` that is non-`extern "C"`, non-variadic (and thus `miri`-compatible), and that still has the same behavior for its call sites.

Then we add a test in `c2rust-pdg` that runs `miri` on `analysis/test` to ensure it stays UB-free.

However, we don't yet run this test by default (it's `#[ignore]`d for now) as there are issues with running `miri` in CI (it installs `xargo` every time and I'm getting a permission denied error (not sure from quite what exactly), and it'd be better to install `xargo` upfront, not on every run).  Thus, I'm `#[ignore]`ing it for now in 4152d34.  We can get it to run in CI correctly later in another PR, but I want to merge this now and avoid over-complicating it here.  The test can still be manually run with `cargo test -p c2rust-pdg -- --ignored analysis_test_miri`.  See #698 for the tracking issue to re-enable this test by default.

The new test passing is blocked on:
* #683 (f ixes #680)
* #684 (f ixes #681)

It would also be nice to f ix #682, but that's not completely necessary for this (though it would create a much less noisy output).
@kkysen
Copy link
Contributor Author

kkysen commented Oct 10, 2022

Similar to #683 (comment), it would be good to know if this is a result of the transpiler. I believe that should be the focus over changes to analysis/test specifically, if so.

This is a result of the transpiler, at least the 0 as *{const,mut} _s, but is intended for the time being. See #202 for a more thorough discussion. We had previously decided the current way was better, but that was before strict provenance was added, so I made this comment: #202 (comment), suggesting {core,std}::ptr::{null,null_mut}() be preferred.

@kkysen kkysen linked a pull request Oct 10, 2022 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kkysen kkysen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix simple strict provenance cases in analysis/test immunant/c2rust
2 participants
@aneksteind @kkysen