Skip to content

Fix UB in analysis/test #680

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
Closed
kkysen opened this issue Sep 25, 2022 · 0 comments · Fixed by #683
Closed

Fix UB in analysis/test #680

kkysen opened this issue Sep 25, 2022 · 0 comments · Fixed by #683
Assignees
@kkysen
Labels
bug Something isn't working

Comments

@kkysen
Copy link
Contributor

kkysen commented Sep 25, 2022 •
edited
Loading

There's a bunch of UB in analysis/test, which causes flakiness in the pdg snapshot test. We should fix all of the UB detected by cargo miri run, though there may be some more as well (it remains flaky in CI).

#650 is a WIP version that fixes the UB and most non-intentional memory leaks.
@kkysen kkysen added the bug Something isn't working label Sep 25, 2022
This was referenced Sep 25, 2022
Closed
Closed
Merged
Merged
@kkysen kkysen self-assigned this Sep 25, 2022
kkysen added a commit that referenced this issue Sep 28, 2022
@kkysen
(#683) Fix UB in analysis/test
4d7808e 
Fixes #680.

See the commits for the individual fixes.  The fixes are:
* 2 use after frees: 429962d, 95e483d
* 2 uninitialized reads: 50b1bb4, f6e5bce
* 2 nullptr derefs: f96bcb2, 69de3c3

Now `cd analysis/test && cargo miri run` reports no UB.  However, `miri` can't run variadic functions like `printf`, so running this requires the monomorphic `printf` shim in #686.
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kkysen kkysen

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix UB in analysis/test immunant/c2rust
1 participant
@kkysen