Make Miri aware of leaks of Box/Vec/String

[nvzqz]

Currently user code that uses leak needs to call miri_static_root themselves. This change removes that requirement.

r? @RalfJung

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

tests/fail/function_calls/exported_symbol_bad_unwind2.rs (revision `definition`) ... ok
tests/fail/function_calls/exported_symbol_bad_unwind2.rs (revision `both`) ... ok

FAILED TEST: tests/fail/memleak_rc.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-Eaqpax" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/tests/fail" "tests/fail/memleak_rc.rs" "--edition" "2021"
error: test got exit status: 0, but expected 1
 = note: compilation succeeded, but was expected to fail

error: actual output differed from expected
error: actual output differed from expected
Execute `./miri test --bless` to update `tests/fail/memleak_rc.stderr` to the actual output
--- tests/fail/memleak_rc.stderr
+++ <stderr output>
-error: memory leaked: ALLOC (Rust heap, SIZE, ALIGN), allocated here:
-  --> RUSTLIB/alloc/src/rc.rs:LL:CC
-   |
-LL |                 Box::leak(Box::new(RcInner { strong: Cell::new(1), weak: Cell::new(1), value }))
-   |
-   = note: BACKTRACE:
-   = note: BACKTRACE:
-   = note: inside `std::rc::Rc::<std::cell::RefCell<std::option::Option<Dummy>>>::new` at RUSTLIB/alloc/src/rc.rs:LL:CC
-  --> tests/fail/memleak_rc.rs:LL:CC
-   |
-   |
-LL |     let x = Dummy(Rc::new(RefCell::new(None)));
-
-note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
-
-note: set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check
---

error: `memory leaked` not found in diagnostics outside the testfile
##[error] --> tests/fail/memleak_rc.rs:1:25
  |
1 | //@error-in-other-file: memory leaked
  |


FAILED TEST: tests/fail/tls_macro_leak.rs
FAILED TEST: tests/fail/tls_macro_leak.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-Eaqpax" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/tests/fail" "tests/fail/tls_macro_leak.rs" "--edition" "2021"
error: test got exit status: 0, but expected 1
 = note: compilation succeeded, but was expected to fail

error: actual output differed from expected
error: actual output differed from expected
Execute `./miri test --bless` to update `tests/fail/tls_macro_leak.stderr` to the actual output
--- tests/fail/tls_macro_leak.stderr
+++ <stderr output>
-error: memory leaked: ALLOC (Rust heap, size: 4, align: 4), allocated here:
-   |
-   |
-LL |             cell.set(Some(Box::leak(Box::new(123))));
-   |
-   = note: BACKTRACE:
-   = note: inside closure at tests/fail/tls_macro_leak.rs:LL:CC
-   = note: inside closure at tests/fail/tls_macro_leak.rs:LL:CC
-   = note: inside `std::thread::LocalKey::<std::cell::Cell<std::option::Option<&i32>>>::try_with::<{closure@tests/fail/tls_macro_leak.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/local.rs:LL:CC
-   = note: inside `std::thread::LocalKey::<std::cell::Cell<std::option::Option<&i32>>>::with::<{closure@tests/fail/tls_macro_leak.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/local.rs:LL:CC
-  --> tests/fail/tls_macro_leak.rs:LL:CC
-   |
-   |
-LL | /         TLS.with(|cell| {
-LL | |             cell.set(Some(Box::leak(Box::new(123))));
-   | |__________^
-
-note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
-
---

error: `memory leaked` not found in diagnostics on line 12
##[error]  --> tests/fail/tls_macro_leak.rs:12:65
   |
12 |             cell.set(Some(Box::leak(Box::new(123)))); //~ERROR: memory leaked
   |


FAILED TEST: tests/fail/tls_static_leak.rs
FAILED TEST: tests/fail/tls_static_leak.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-Eaqpax" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/tests/fail" "tests/fail/tls_static_leak.rs" "--edition" "2021"
error: test got exit status: 0, but expected 1
 = note: compilation succeeded, but was expected to fail

error: actual output differed from expected
error: actual output differed from expected
Execute `./miri test --bless` to update `tests/fail/tls_static_leak.stderr` to the actual output
--- tests/fail/tls_static_leak.stderr
+++ <stderr output>
-error: memory leaked: ALLOC (Rust heap, size: 4, align: 4), allocated here:
-   |
-   |
-LL |         TLS.set(Some(Box::leak(Box::new(123))));
-   |
-   = note: BACKTRACE:
-   = note: inside closure at tests/fail/tls_static_leak.rs:LL:CC
-
---

error: `memory leaked` not found in diagnostics on line 14
##[error]  --> tests/fail/tls_static_leak.rs:14:60
   |
14 |         TLS.set(Some(Box::leak(Box::new(123)))); //~ERROR: memory leaked
   |

FAILURES:
    tests/fail/memleak_rc.rs
---

Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.26.5/src/lib.rs:357

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`
Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-c9642fae2e8182ec --quiet` (exit status: 1)
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-c9642fae2e8182ec --quiet` (exit status: 1)
Command has failed. Rerun with -v to see more details.
  local time: Tue Jan 21 10:08:13 UTC 2025
  network time: Tue, 21 Jan 2025 10:08:13 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[nvzqz]

Apparently Miri erroring on these leaks is expected behavior according to failed tests. Perhaps these tests should use another way to leak memory?

[RalfJung]

Thanks for the PR!

I am not convinced that leak should be specially recognized by Miri in any way. But if it did, that should also be documented, and t-libs-api should be involved in the decision.

Cc @rust-lang/miri @rust-lang/libs-api

[Amanieu]

This seems incorrect to me: we still want to treat data leaked by leak as a real memory leak if it is not kept (transitively) reachable via a static. Suppressing this should be opt-in depending on the specific way the leaked reference is used and not a blanket suppression for all uses of leak.

[nvzqz]

Usually the intention I've seen leak used for is to make an allocation live for the remainder of the program, regardless of whether the memory is reachable at program termination. So I believe it makes sense for Miri to ignore it.

Although it's documented that you can reclaim memory returned by leak (Box::from_raw(Box::leak(...))), most code I've seen that does this uses raw pointers from Box::into_raw instead of a mutable reference.

[RalfJung]

Although it's documented that you can reclaim memory returned by leak (Box::from_raw(Box::leak(...))), most code I've seen that does this uses raw pointers from Box::into_raw instead of a mutable reference.

Hm, yeah that does make sense -- though changing this now for a long-stable operation makes me feel a bit uneasy.

Could you go look around some of the widely used crates in the ecosystem and gather some data on how leak is used there, whether it is typically reclaimed / reachable from a static or not?

[Amanieu]

We discussed this in the libs-api meeting: we don't want leak to disable leak detection because there are valid use cases where you want to later free that allocation or bind it to a static. In those cases you do want to detect the case where the value is actually leaked instead of being bound or freed.

Instead, we would prefer to document miri_static_root or expose it via std::hint so that it can be used where needed.

@rfcbot close

[rfcbot]

Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.