Add regression test for CString::clone_into unwind safety#158807
Conversation
|
|
|
LGTM, thanks :) @bors r+ rollup |
…-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
Rollup of 20 pull requests Successful merges: - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158642 (Clarify some interning details) - #158743 (Look for cdb location in the registry first) - #158775 (bootstrap: only encode RUSTFLAGS when a flag contains a space) - #158782 (Add and use cfg(target_has_threads) to enforce no_thread impl usage) - #158785 (hook intrinsic-test into aarch64-gnu) - #157734 (Stabilize `local_key_cell_update`) - #158183 (std: allocate less memory in `current_exe` for OpenBSD) - #158671 (Move tests batch 17) - #158730 (Update `FIXME(static_mut_refs)` comments) - #158752 (Reorganize `tests/ui/issues` [18/N]) - #158755 (Use `ThinVec` more in the AST) - #158757 (Fix incorrect tracking issue for `read_le`/`read_be`) - #158765 (Fix ICE on non-ident path in `doc(auto_cfg values)`) - #158771 (library: expand HashSet::extract_if coverage) - #158772 (rustc-dev-guide subtree update) - #158776 (fix: emit diagnostic for AVR target without target-cpu) - #158786 (Add regression test for builtin attr macro values) - #158807 (Add regression test for CString::clone_into unwind safety) - #158825 (Fix typo)
|
Look like you've got some UB in the test. |
|
This pull request was unapproved. |
|
Thanks. The UB is in libtest's own teardown, not the test body: with a #[global_allocator] installed, libtest frees its internal allocations (the mpmc results channel) through it, and on Windows System::dealloc reads the allocation header just before the pointer, which trips Stacked Borrows since the freed Box's provenance doesn't cover the header. It fires even with 0 tests run - it's inherent to wrapping System in a global allocator under Miri on Windows, independent of the test itself. Two ways to resolve it, whichever you'd prefer:
Happy to do either - which do you prefer? |
|
Disabling the test on Windows with a link to rust-lang/miri#2104 seems good. |
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 in library/alloctests that fails the reallocation under a panicking alloc error hook and checks the target stays valid. The failing allocator is only honored under Miri - a global allocator in a library test doesn't intercept libstd's allocation in a normal build - so the unwind assertion is gated on cfg!(miri); the test still runs and passes as a regular test.
735cc3d to
6a9fb2d
Compare
|
Done - disabled under Miri on Windows via crate-level @rustbot ready |
|
@bors try jobs=x86_64-msvc-ext2 |
This comment has been minimized.
This comment has been minimized.
…r, r=<try> Add regression test for CString::clone_into unwind safety try-job: x86_64-msvc-ext2
|
@bors r+ rollup
|
…-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
…uwer Rollup of 18 pull requests Successful merges: - #158871 (add relnotes for 1.97.0) - #150946 (intrinsics: Add a fallback for non-const libm float functions) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158655 (Fix coroutine MIR saved local remapping) - #158666 (Carry the `b_offset` inside `BackendRepr::ScalarPair`) - #158920 (Update wasm-component-ld to 0.5.26) - #158926 (wrapping_sh* methods: clarify underspecified reference) - #158927 (add core test run with `-Zforce-intrinsic-fallback`) - #151379 (Stabilize `VecDeque::retain_back` from `truncate_front`) - #158807 (Add regression test for CString::clone_into unwind safety) - #158862 (Fix the span for parameter suggestion ) - #158883 (tests: fix enum-match.rs to handle LLVM 23) - #158894 (Make the ordering of non-terminal binds in ambiguity error messages deterministic) - #158902 (add codegen test for range length bound propagation) - #158913 (Update `browser-ui-test` version to `0.24.1`) - #158935 (std: support real fd methods on Emscripten) - #158951 (Merge three `MaxUniverse`s into one)
…uwer Rollup of 18 pull requests Successful merges: - #158871 (add relnotes for 1.97.0) - #150946 (intrinsics: Add a fallback for non-const libm float functions) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158655 (Fix coroutine MIR saved local remapping) - #158666 (Carry the `b_offset` inside `BackendRepr::ScalarPair`) - #158920 (Update wasm-component-ld to 0.5.26) - #158926 (wrapping_sh* methods: clarify underspecified reference) - #158927 (add core test run with `-Zforce-intrinsic-fallback`) - #151379 (Stabilize `VecDeque::retain_back` from `truncate_front`) - #158807 (Add regression test for CString::clone_into unwind safety) - #158862 (Fix the span for parameter suggestion ) - #158883 (tests: fix enum-match.rs to handle LLVM 23) - #158894 (Make the ordering of non-terminal binds in ambiguity error messages deterministic) - #158902 (add codegen test for range length bound propagation) - #158913 (Update `browser-ui-test` version to `0.24.1`) - #158935 (std: support real fd methods on Emscripten) - #158951 (Merge three `MaxUniverse`s into one)
…uwer Rollup of 18 pull requests Successful merges: - #158871 (add relnotes for 1.97.0) - #150946 (intrinsics: Add a fallback for non-const libm float functions) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158655 (Fix coroutine MIR saved local remapping) - #158666 (Carry the `b_offset` inside `BackendRepr::ScalarPair`) - #158920 (Update wasm-component-ld to 0.5.26) - #158926 (wrapping_sh* methods: clarify underspecified reference) - #158927 (add core test run with `-Zforce-intrinsic-fallback`) - #151379 (Stabilize `VecDeque::retain_back` from `truncate_front`) - #158807 (Add regression test for CString::clone_into unwind safety) - #158862 (Fix the span for parameter suggestion ) - #158883 (tests: fix enum-match.rs to handle LLVM 23) - #158894 (Make the ordering of non-terminal binds in ambiguity error messages deterministic) - #158902 (add codegen test for range length bound propagation) - #158913 (Update `browser-ui-test` version to `0.24.1`) - #158935 (std: support real fd methods on Emscripten) - #158951 (Merge three `MaxUniverse`s into one)
…-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
…uwer Rollup of 25 pull requests Successful merges: - #158871 (add relnotes for 1.97.0) - #158968 (stdarch subtree update) - #154445 (rustdoc: Represent `--output-format=json` coverage and ir differently) - #156370 (Reject linked dylib EII default overrides) - #157153 (allow `Allocator`s to be used as `#[global_allocator]`s) - #158495 (Rename HAS_CT_PROJECTION to HAS_CONST_ALIAS) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158655 (Fix coroutine MIR saved local remapping) - #158666 (Carry the `b_offset` inside `BackendRepr::ScalarPair`) - #158912 (Introduce new bootstrap config section for PGO configuration) - #158920 (Update wasm-component-ld to 0.5.26) - #158926 (wrapping_sh* methods: clarify underspecified reference) - #158927 (add core test run with `-Zforce-intrinsic-fallback`) - #158932 (Do not build the compiler when invoking `x perf compare`) - #158937 (Emit the emscripten entry point as `__main_argc_argv`) - #151379 (Stabilize `VecDeque::retain_back` from `truncate_front`) - #156548 ( Library support for aarch64-unknown-linux-pauthtest target) - #158307 (CI job for parallel frontend ui tests) - #158347 (Improve generic parameters handling for #[diagnostic::on_const]) - #158722 (delegation: do not always inherit `ConstArgHasType` predicates) - #158741 (Simplify `Option::into_flat_iter` signature) - #158807 (Add regression test for CString::clone_into unwind safety) - #158862 (Fix the span for parameter suggestion ) - #158883 (tests: fix enum-match.rs to handle LLVM 23)
…-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
Rollup of 23 pull requests Successful merges: - #158968 (stdarch subtree update) - #154445 (rustdoc: Represent `--output-format=json` coverage and ir differently) - #158495 (Rename HAS_CT_PROJECTION to HAS_CONST_ALIAS) - #158666 (Carry the `b_offset` inside `BackendRepr::ScalarPair`) - #158870 (std: merge the unix-like io::error modules into one file) - #158920 (Update wasm-component-ld to 0.5.26) - #158926 (wrapping_sh* methods: clarify underspecified reference) - #158927 (add core test run with `-Zforce-intrinsic-fallback`) - #158932 (Do not build the compiler when invoking `x perf compare`) - #158937 (Emit the emscripten entry point as `__main_argc_argv`) - #151379 (Stabilize `VecDeque::retain_back` from `truncate_front`) - #156144 (Better docs for PartialEq (includes macro rename)) - #156548 ( Library support for aarch64-unknown-linux-pauthtest target) - #157995 (`Vec::dedup_by` docs explicit function argument order) - #158307 (CI job for parallel frontend ui tests) - #158741 (Simplify `Option::into_flat_iter` signature) - #158807 (Add regression test for CString::clone_into unwind safety) - #158862 (Fix the span for parameter suggestion ) - #158894 (Make the ordering of non-terminal binds in ambiguity error messages deterministic) - #158902 (add codegen test for range length bound propagation) - #158913 (Update `browser-ui-test` version to `0.24.1`) - #158935 (std: support real fd methods on Emscripten) - #158978 (Add regression test for too-big by-value ABI args)
Rollup merge of #158807 - Vastargazing:tests/cstr-clone-into-alloc-error, r=RalfJung Add regression test for CString::clone_into unwind safety Regression test for the panic-safety fix in #155707 #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. #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
Regression test for the panic-safety fix in #155707
#70201 gave
<CStr as ToOwned>::clone_intoa path that moved the targetCString's buffer out (leaving it empty) before growing aVec; if that growth's allocation failed and unwound, the target was left without its nul terminator, which is UB. #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 insideCString::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 oncfg!(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