Add regression test for CString::clone_into unwind safety#5157
Add regression test for CString::clone_into unwind safety#5157Vastargazing wants to merge 1 commit into
Conversation
CString::clone_into reuses the target's allocation by moving the buffer into a Vec and growing it. If that growth's allocation fails and the alloc error hook unwinds, the target has to be left as a valid CString, but nothing covered that path. Add a test that fails the reallocation under a panicking alloc error hook and checks that the caught unwind leaves the target's nul terminator in place.
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @oli-obk (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
There was a problem hiding this comment.
- See my comment in your previous PR -- the test should contain references to the issue(s) it is about
- Since this tests the standard library, not Miri, then if we must have this test here (do we?), it should be in a folder. Maybe call it
lib?
There was a problem hiding this comment.
Will add the issue references
On the location: I originally wrote this as a library/alloctests test, but a native (non-Miri) alloctests run builds with -C prefer-dynamic, so the #[global_allocator] in the test binary doesn't intercept the reallocation inside CString::clone_into (which lives in libstd). As a result, the test no longer reproduces the bug - I verified this. It only works when the allocator is intercepted, i.e. under Miri.
So if it lives in library/alloctests, it would need to be Miri-only (e.g. #[cfg_attr(not(miri), ignore)]) and rely on the liballoc-under-Miri CI run. Would you prefer that, or should I keep it here under tests/pass/lib/? (oli-obk suggested moving it here from rust-lang/rust#158763)
There was a problem hiding this comment.
We have a bunch of tests in library/* that are mainly for Miri. They also run as regular tests, nothing wrong with that. So no need to make them not(miri). They only really test what the need to test under Miri, which is fine -- there can be a comment explaining that.
There was a problem hiding this comment.
makes sense - I'll move it to library/alloctests. I'll drop the assert that requires the unwind (so it passes as a normal test, where the allocator can't intercept libstd) and keep the check that the target is still a valid CString, with a comment that the failure path only really runs under Miri. Plus the issue references. I'll open that against rust-lang/rust and close this PR.
There was a problem hiding this comment.
Moved it locally and verified: passes as a regular test, and under ./x miri library/alloctests the allocator fires and it catches the bug.
One thing before I open the PR: the x86_64-gnu-miri job runs liballoc with --test-args notest, so as far as I can tell this test would compile under Miri in CI but not run there - the only CI run would be the trivial native one. Is that the coverage you intended, or would you rather keep it as a Miri test (it ran across the full matrix here), or wire it into CI differently?
There was a problem hiding this comment.
There is another job, something with gnu-aux, that runs this file.
There was a problem hiding this comment.
I'll drop the assert that requires the unwind
If it's useful you can also keep that under if cfg!(miri). Just run as much as possible also natively. :)
|
Closing - moving this to library/alloctests in rust-lang/rust per the discussion (rust-lang/rust#158807). Thanks @RalfJung, @oli-obk. |
…-alloc-error, r=RalfJung Add regression test for CString::clone_into unwind safety Regression test for the panic-safety fix in rust-lang#155707 rust-lang#70201 gave `<CStr as ToOwned>::clone_into` a path that moved the target `CString`'s buffer out (leaving it empty) before growing a `Vec`; if that growth's allocation failed and unwound, the target was left without its nul terminator, which is UB. rust-lang#155707 fixed this but didn't add a test. The failing allocator here is only honored under Miri - in a normal build a `#[global_allocator]` in a library test doesn't intercept the reallocation inside `CString::clone_into` (it lives in libstd, linked `-C prefer-dynamic`). So the test passes as a regular test and does the real check under Miri, with the unwind assertion gated on `cfg!(miri)`; it runs under Miri in CI via the library-tests-under-Miri job. This started as rust-lang/miri#5157, but per @RalfJung a std regression test belongs with the standard library. r? @RalfJung
…-alloc-error, r=RalfJung Add regression test for CString::clone_into unwind safety Regression test for the panic-safety fix in rust-lang#155707 rust-lang#70201 gave `<CStr as ToOwned>::clone_into` a path that moved the target `CString`'s buffer out (leaving it empty) before growing a `Vec`; if that growth's allocation failed and unwound, the target was left without its nul terminator, which is UB. rust-lang#155707 fixed this but didn't add a test. The failing allocator here is only honored under Miri - in a normal build a `#[global_allocator]` in a library test doesn't intercept the reallocation inside `CString::clone_into` (it lives in libstd, linked `-C prefer-dynamic`). So the test passes as a regular test and does the real check under Miri, with the unwind assertion gated on `cfg!(miri)`; it runs under Miri in CI via the library-tests-under-Miri job. This started as rust-lang/miri#5157, but per @RalfJung a std regression test belongs with the standard library. r? @RalfJung
4 participants
Regression test for the panic-unsoundness fixed in rust-lang/rust#155707
rust-lang/rust#70201 added a
clone_intooverride forCStrthat moved the targetCString's buffer out (leaving it empty) before growing aVec. On allocation failure the destinationCStringcould lose its trailing nul terminator, leading to UB. rust-lang/rust#155707 fixed this but didn't add a test.It only reproduces under Miri: a
#[global_allocator]in a library test doesn't intercept the reallocation insideCString::clone_intobecause library tests link std with-C prefer-dynamic, so it can't live inlibrary/alloctests. Moved here from rust-lang/rust#158763 per @oli-obk (no rustc change required).r? @oli-obk