Fix UB in analysis/test
#683
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…` by freeing the right variable.
…`calloc(0, ...)` with `calloc(1, ...)`, as the ptr is then dereferenced.
…acing `calloc(0, ...)` with `calloc(1, ...)`, as the ptr is then dereferenced.
kkysen
force-pushed
the
kkysen/analysis-test-fix-ub
branch
from
69de3c3 to
30c87f4
Compare
This was referenced Sep 25, 2022
|
Can you remind me whether or not this UB is the fault of the transpiler? This is where much of the code in the test file has come from. It would be great to treat the issue at the source, if so. |
aneksteind
approved these changes
Sep 27, 2022
I have no idea at this point. The file has diverged quite a bit from the initial transpiled code. That said, if the C has UB, I think the transpiled Rust would have UB, too. |
|
Yeah, none of the UB fixes here are in functions that are in |
|
Ok, that sounds fine then |
kkysen
commented
Sep 28, 2022
kkysen
force-pushed
the
kkysen/analysis-test-fix-ub
branch
from
f21764b to
d6eb540
Compare
kkysen
added a commit
that referenced
this pull request
Oct 10, 2022
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).
This was referenced Oct 10, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #680.
See the commits for the individual fixes. The fixes are:
Now
cd analysis/test && cargo miri runreports no UB. However,mirican't run variadic functions likeprintf, so running this requires the monomorphicprintfshim in #686.