Leak checker misses pointers cast to integer

[RalfJung]

The leak checker considers allocations live when they can still be reached from a static. However, this scan can only consider pointers that are still stored as pointer values (with provenance), not pointers cast to an integer (where provenance is lost).

This causes false positives, at least, for

• Windows Mutex (Enable leak checker on Windows #1302)
• Any user of AtomicPtr
  • crossbeam (Memory leak from crossbeam_epoch::deferred::Deferred::new crossbeam-rs/crossbeam#464)
  • Leak checker doesn't see through AtomicPtr #1350

[RalfJung]

For the Windows Mutex, one option on the Miri side is to keep provenance on ptr-to-int casts, and only delete provenance later when the integer is actually used for an integer operation. I am not extremely happy about this but it is also not entirely awful. This is blocked on #841.

---

For crossbeam, it turns out the pointer is not just cast to an integer, it also has its last bits repurposed to store some flags. So, even if we do the above, we are still going to lose the provenance.

I would say trying to preserve provenance across these bitwise operations is conceptually just not the kind of semantics that Miri should give to Rust. It's also a game of whack-a-mole to get ever more effective at preserving provenance -- a game that the proper support for ptr-int-casts that we have now is supposed to just end, so I do not have a big interest in re-opening that game.

Another option might be to expect libraries that pull such tricks (storing the pointers that keep an allocation live without provenance) to give us some extra hints, via some Miri-specific instrumentation/intrinsics or so. (I recall @oli-obk proposed such operations a while ago for a different purpose and I found the idea quite disgusting. ;) However, the code doing the casting might not even be aware that it is working on data stored in a global static, so it is unclear how such an API could even look like.

Cc @jonhoo

[jonhoo]

I think if there were annotations libraries like crossbeam could give to miri to get tracking to work in these situations, they would be happy to add those :) At least I would be!

[RalfJung]

Some time later, I don't think we have a proposal on the table that is better than asking the library for help. I am imagining an intrinsic like miri_ignore_leak_root or so that takes a pointer, and then the allocation that pointer points to gets added to the list of "roots" that Miri considers when checking if memory is still reachable on program termination (currently, that list consists of all the statics). @oli-obk any thoughts?

What I am not sure about is if the intrinsic should require the pointer to point to the beginning of the allocation -- I think for now I'd like to be as restrictive as possible and make this requirement.

[oli-obk]

We don't even need to burden rustc with an intrinsic. We can just use an extern "C" function (which also helps stable code, since it doesn't have to invoke an intrinsic). Basically users would write

#[cfg(miri)]
extern "C" { fn miri_ignore_leak_root(ptr: usize); }
#[cfg(miri)]
unsafe { miri_ignore_leak_root(some_ptr as usize); }

What I am not sure about is if the intrinsic should require the pointer to point to the beginning of the allocation -- I think for now I'd like to be as restrictive as possible and make this requirement.

I agree, we can emit a nice error stating the offset required to obtain a root pointer in order to help users quickly adjust their code to work without having to figure out an earlier site for obtaining the pointer.

If we feel really fancy we can make the leak checker emit a note at the first ptr2intcast site

[RalfJung]

We don't even need to burden rustc with an intrinsic. We can just use an extern "C" function (which also helps stable code, since it doesn't have to invoke an intrinsic).

Oh wow.^^ Now I wonder if we should have used that for miri_start_panic. ;)

I'd make the type a pointer type though, not usize.

[RalfJung]

#1485 provides a solution to this: programs can call the extern "Rust" function miri_static_root to mark an allocation as being a root of static memory that should not be considered leaking when the program terminates.

@jonhoo if you could experiment with using that in crossbeam, that would be amazing. :)

[jonhoo]

@RalfJung That's exciting! Unfortunately I'm pretty swamped these days, but I posted a comment over in crossbeam-rs/crossbeam#464 (comment) to see if I could pique some interest.

[divergentdave]

I'm working on triaging errors running crossbeam's test suite in Miri. For a typical leak report when using crossbeam-epoch, the only leaking allocations are for two different kinds of linked lists held by the global garbage collection context. Both of these data structures use crossbeam_epoch::Atomic, and the leak report misses these because they are behind a pointer-to-int conversion.

If I understand correctly, calling miri_static_root on entries as they are added would empty out the leak report, at the expense of possibly missing bugs if an entry were popped from the globally held lists and leaked instead of deallocated.

My other ideas, to avoid false negatives:

• Conditionally add const pointer members to list entry data structures, update them after successfully updating the Atomic members, and never read them. Upon exit, the interpreter will be able to chase these extra pointers down the linked lists, and correctly ignore the entries. Downside: this will require adding conditional compilation blocks throughout the data structures, imposing some maintenance burden.
• Add another extern fn to register an "atexit" callback, then add one conditional compilation block to call it, and within the callback, chase down the linked lists, calling miri_static_root on each entry. I'm not a big fan of this idea because it adds Turing completeness in a strange place, and I don't know what sort of weird issues would arise from trying to run code after the interpreter has exited main.

[RalfJung]

If I understand correctly, calling miri_static_root on entries as they are added would empty out the leak report, at the expense of possibly missing bugs if an entry were popped from the globally held lists and leaked instead of deallocated.

Correct.

Add another extern fn to register an "atexit" callback, then add one conditional compilation block to call it, and within the callback, chase down the linked lists, calling miri_static_root on each entry. I'm not a big fan of this idea because it adds Turing completeness in a strange place, and I don't know what sort of weird issues would arise from trying to run code after the interpreter has exited main.

Such a mechanism already exists (at least per-thread) since Miri implements TLS dtors run on thread exit.