don't report memory leaks for static data

[matklad]

Currently, miri reports a memory leak for a static with heap allocated data. This is somewhat expected, as statics don't run destructors and indeed leak memory. However, this is not error, and ideally shouldn't be reported by miri as such.

See matklad/once_cell@327f4e2 for an example.

[oli-obk]

Oh, heh. I never thought about heap pointers being leaked into statics. Also calls to Box::leak should probably not trigger the leak errors.

Maybe we should, before running the check, recursively mark anything reachable by a static as static itself.

And we should add a hook to Box::leak to make it mark its memory as static (we can use rustc_diagnostic_item to get the DefId of Box::leak

[RalfJung]

We probably also should make the leak check optional. A quick solution would be -Zmiri-allow-leaks; or we could make the leak error a "deny-by-default lint" and then use -Aleak or so.

And we should add a hook to Box::leak to make it mark its memory as static

Why that? I think our leak check should trigger for Box::leak memory that is actually leaked.

[oli-obk]

Why that? I think our leak check should trigger for Box::leak memory that is actually leaked.

I assumed that Box::leak could never be undone, but even stacked borrows is fine with it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=cae54e950dfd613b3c255263f352795f

I'm assuming &'static mut T and *mut T are equal in the eyes of stacked borrows

My statement came from assuming that people don't use Box::leak and expect to ever take back ownership of that. If you ever stored it in a global it seems impossible to ever safely undo the leak because any other code may have an immutable reference to the thing.

Nixing the entire leak check, just because some code found it ok to leak a single value out of lazy static like reasons, seems a bit extreme.

[RalfJung]

I'm assuming &'static mut T and *mut T are equal in the eyes of stacked borrows

No they are not (the former disallows accesses through other aliases).

My statement came from assuming that people don't use Box::leak and expect to ever take back ownership of that. If you ever stored it in a global it seems impossible to ever safely undo the leak because any other code may have an immutable reference to the thing.

I see. It would seem odd though, I think, to enshrine this in the operational semantics.

Nixing the entire leak check, just because some code found it ok to leak a single value out of lazy static like reasons, seems a bit extreme.

We could also have a magic crazy intrinsic "tell Miri this here is allowed to leak"... I think I will regret suggesting this, though. ;)

[RalfJung]

I just ran into another instance of this. The following program is considered leaking:

fn main() {
    let _val = 0;
    // Capture something to make closure a non-ZST
    std::panic::set_hook(Box::new(move |_info| { _val; }));
}

[RalfJung]

On Windows, we need this even to get an empty fn main() {} working with the leak checker enabled: there are some statics in libstd that are otherwise considered leaking.

[jonhoo]

Hmm, I still get a reported leak through crossbeam-epoch when using statics. I don't know enough about the crossbeam internals to say whether it is legitimate or not, but I know they run a leak sanitizer which hasn't caught anything? I haven't tried running the crossbeam tests themselves on miri yet, but maybe that'd be worth doing?

[RalfJung]

@jonhoo could that be the same problem as #1302, i.e., the pointer is cast to an integer before being stored and thus Miri cannot "see" it and cannot figure out that the allocation is reachable?

[jonhoo]

Ah, quite possible! I think crossbeam-utils does that in a few places from skimming the code. Sounds like maybe it'd be worthwhile to change those to use raw pointers instead. I don't know why the code is using usize in the first place..

[RalfJung]

They might also be using AtomicPtr which under the hood does a cast to usize -- see rust-lang/rust#70765.

[jonhoo]

I suspect the offender here might be
https://github.com/crossbeam-rs/crossbeam/blob/0599b8e88cccc88525cd430c806b057cc4be2829/crossbeam-epoch/src/atomic.rs#L128

I suspect it was stored as a usize due to using alignment bits to store a tag:
https://github.com/crossbeam-rs/crossbeam/blob/0599b8e88cccc88525cd430c806b057cc4be2829/crossbeam-epoch/src/atomic.rs#L120-L122

Is that also legal for * pointers?

crossbeam-channel and crossbeam-deque both use AtomicPtr, though none of that is used in the code that reports the leak I linked to.

[RalfJung]

Is that also legal for * pointers?

It is only legal for raw pointers -- references must be aligned, so their last bits must be 0.

[jonhoo]

Sorry, yes, I meant the "also" in comparison to usize, not in comparison to references which I know must be aligned :)

[RalfJung]

Ah, yes raw pointers may be NULL, dangling and unaligned, that is fine, as long as they do not get dereferenced (* operator -- even when no memory access happens).

[jonhoo]

Digging some more into this, it's actually tricky to make crossbeam use AtomicPtr since it relies on operations like AtomicUsize::fetch_xor, which do not exist for AtomicPtr :/

[RalfJung]

Yeah, that sounds tricky indeed. Hm.

[jonhoo]

Especially since it seems these were once supported, and then removed: rust-lang/rust#10154

[RalfJung]

Honestly I am not sure any of this will suffice. XOR is fundamentally an integer operation that loses provenance, and while we could re-introduce awful hacks in Miri that enable provenance tracking over XOR, I do not think that is a good idea.

[RalfJung]

See ptr_int_arithmetic removed in this commit for the awful things we used to do.

[jonhoo]

crossbeam only specifically does xor of the alignment bits though, not of the "real" part of the pointer. Though I recognize that that might not really help 😅 It would be really sad if miri simply could not support checking crossbeam-epoch, or anything that depends on it.

[RalfJung]

It would be really sad if miri simply could not support checking crossbeam-epoch, or anything that depends on it.

Yeah, I agree that would be sad.
However, re-adding that code might be even more sad.^^

[RalfJung]

To avoid continuing this discussion in a closed issue, I opened #1318.