Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak checker misses AtomicPtr #1574

Closed
cynecx opened this issue Oct 5, 2020 · 18 comments · Fixed by rust-lang/rust#77611
Closed

Memory leak checker misses AtomicPtr #1574

cynecx opened this issue Oct 5, 2020 · 18 comments · Fixed by rust-lang/rust#77611
Assignees
Labels
A-leaks Area: affects the memory leak checker C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@cynecx
Copy link

cynecx commented Oct 5, 2020

use std::{
    cell::UnsafeCell,
    ptr,
    sync::atomic::{AtomicPtr, Ordering},
};

fn leak() {
    static LEAK: AtomicPtr<usize> = AtomicPtr::new(ptr::null_mut());
    LEAK.store(Box::into_raw(Box::new(0usize)), Ordering::SeqCst);
}

fn no_leak() {
    struct Local(UnsafeCell<*mut usize>);

    unsafe impl Send for Local {}
    unsafe impl Sync for Local {}

    static LEAK: Local = Local(UnsafeCell::new(ptr::null_mut()));
    *unsafe { &mut *LEAK.0.get() } = Box::into_raw(Box::new(0usize));
}

fn main() {
    no_leak();
    // leak();
}

Miri considers memory leaks reachable through a static global as non-leaking. I'd expect the Atomic* case be non-leaking as well, however miri reports it as leaked memory.

The following memory was leaked: alloc1834 (Rust heap, size: 8, align: 8) {
    00 00 00 00 00 00 00 00                         │ ........
}

error: the evaluated program leaked memory

error: aborting due to previous error; 1 warning emitted

(This is a special case of #1618.)

@bjorn3
Copy link
Member

bjorn3 commented Oct 5, 2020

AtomicPtr casts the pointer to an integer before storing it. This prevents miri from seeing that the AtomicPtr keeps it alive.

@RalfJung RalfJung added A-leaks Area: affects the memory leak checker C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Oct 6, 2020
@RalfJung RalfJung changed the title Memory leak with Atomic Memory leak checker misses AtomicPtr and other pointers cast to integers Oct 6, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 6, 2020

So... we can either not do a cast in AtomicPtr (I guess a transmute will keep the provenance?) or we need to teach AtomicPtr to set the miri_static_root (but that would be overly permissive, as you could now also just do that in non-static code).

@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2020

Yeah, I cannot think of a good way to fix this I am afraid... we cannot just randomly cast any integer we see to a pointer in the off chance that it represents an actual address. Ideally AtomicPtr would not cast pointers to integers, but the underlying problem here is that LLVM does not support atomic operations on pointers. I guess we could push this down to the LLVM codegen layer so that the Rust intrinsic does support pointers, not sure if that would be a good idea.

Probably I should not have closed #1318, which is the same problem. Crossbeam is also affected by this (crossbeam-rs/crossbeam#464).

Currently the best way to approach the problem is to call miri_static_root.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 6, 2020

I guess we could push this down to the LLVM codegen layer so that the Rust intrinsic does support pointers, not sure if that would be a good idea.

Oh, I like this. That seems entirely reasonable to me

@oli-obk
Copy link
Contributor

oli-obk commented Oct 6, 2020

the underlying problem here is that LLVM does not support atomic operations on pointers

I was unable to find any recent information on this being true and changing it in rustc to just use pointers worked without any additional changes.

@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2020

Uh I thought I had LLVM experts confirm that when I asked but maybe I misremember.

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2020

FWIW, this is also a problem when using -Zmiri-track-raw-pointers, which breaks int-to-ptr casts -- both Windows in general, and accessing the program arguments on Unix systems, involve AtomicPtr and thus fail with -Zmiri-track-raw-pointers.

@RalfJung RalfJung changed the title Memory leak checker misses AtomicPtr and other pointers cast to integers Memory leak checker misses AtomicPtr Nov 2, 2020
@RalfJung
Copy link
Member

Status: @oli-obk started some work on supporting raw pointer types in the atomic intrinsics in rust-lang/rust#77611, and then realized that this would need some support by the codegen backend since LLVM only supports atomic operations on integer types.

bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Dec 17, 2020
Directly use raw pointers in `AtomicPtr` store/load

I was unable to find any reason for this limitation in the latest source of LLVM or in the documentation [here](http://llvm.org/docs/Atomics.html#libcalls-atomic).

fixes rust-lang/miri#1574
@jonhoo
Copy link

jonhoo commented Feb 28, 2022

Just to double-check my understanding; is it expected that -Zmiri-track-raw-pointers will fail to track correctly when using AtomicPtr::compare_exchange and friends, since they still use pointers-as-integers under the hood (I think only load/store (rust-lang/rust#77611) and swap (rust-lang/rust#80236) have been "fixed")?

jonhoo added a commit to jonhoo/haphazard that referenced this issue Feb 28, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 28, 2022

Yes. We need to fix this in libstd/rustc, miri is doing the right thing ™️ and implementing workarounds just on the miri side is not feasible afaict

@RalfJung
Copy link
Member

RalfJung commented Mar 1, 2022

So looks like this comment was not accurate then?

If you have the time, could you remove all as *mut usize and as usize from the implementation of AtomicPtr and adjust the intrinsics accordingly?

I don't think there is anything left (though I might have missed something). Remaining operations are intentionally unsupported in atomic ptr, so I don't think we should support them in intrinsics either.

@RalfJung
Copy link
Member

RalfJung commented Mar 1, 2022

@jonhoo looking at the compare_exchange implementation, it does not seem to use int-to-ptr casts:

https://github.com/rust-lang/rust/blob/1cc0ae4cbbcb9909035b5b99fc20bf6b79f4010c/library/core/src/sync/atomic.rs#L1153

So actually I think I disagree with Oli, this is not expected. Do you have a small example of this going wrong?

@jonhoo
Copy link

jonhoo commented Mar 2, 2022

Hmm, interesting.. What led me down this path wasn't directly related to the leak check, but rather -Zmiri-tag-raw-pointers specifically, and this just seemed like the most related issue.

I don't have a reduced test case at the moment, but the code is in https://github.com/jonhoo/haphazard/, and it's not very complicated and has no dependencies, so reducing to a simple test case shouldn't be too bad. What made me think a ptr-to-int conversion was happening was the "but parent tag " part of the Miri error from the test below, which matches the README's note saying " You can recognize false positives by <untagged> occurring in the message -- this indicates a pointer that was cast from an integer, so Miri was unable to track this pointer." (note that without the -Z, Miri runs without errors):

$ env "MIRIFLAGS=-Zmiri-tag-raw-pointers" cargo +nightly miri test --test lib feels_good
    Finished test [unoptimized + debuginfo] target(s) in 0.03s
     Running tests/lib.rs (/home/jon/.cargo-target/miri/x86_64-unknown-linux-gnu/debug/deps/lib-c166183ccbed9275)

running 1 test
test feels_good ... error: Undefined Behavior: trying to reborrow for SharedReadWrite at alloc65366, but parent tag <untagged> does not have an appropriate item in the borrow stack
   --> /home/jon/dev/streams/haphazard/src/domain.rs:310:33
    |
310 |         let mut next = unsafe { &*tail }.available_next.load(Ordering::Relaxed);
    |                                 ^^^^^^ trying to reborrow for SharedReadWrite at alloc65366, but parent tag <untagged> does not have an appropriate item in the borrow stack
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

    = note: inside `haphazard::Domain::<haphazard::Global>::try_acquire_available_locked::<1_usize>` at /home/jon/dev/streams/haphazard/src/domain.rs:310:33
    = note: inside `haphazard::Domain::<haphazard::Global>::try_acquire_available::<1_usize>` at /home/jon/dev/streams/haphazard/src/domain.rs:284:45
    = note: inside `haphazard::Domain::<haphazard::Global>::acquire_many::<1_usize>` at /home/jon/dev/streams/haphazard/src/domain.rs:217:29
    = note: inside `haphazard::Domain::<haphazard::Global>::acquire` at /home/jon/dev/streams/haphazard/src/domain.rs:211:9
    = note: inside `haphazard::HazardPointer::new_in_domain` at /home/jon/dev/streams/haphazard/src/hazard.rs:54:21
    = note: inside `haphazard::HazardPointer::<'static>::new` at /home/jon/dev/streams/haphazard/src/hazard.rs:41:9
note: inside `feels_good` at tests/lib.rs:106:17
   --> tests/lib.rs:106:17
    |
106 |     let mut h = HazardPointer::new();
    |                 ^^^^^^^^^^^^^^^^^^^^
note: inside closure at tests/lib.rs:77:1
   --> tests/lib.rs:77:1
    |
76  |   #[test]
    |   ------- in this procedural macro expansion
77  | / fn feels_good() {
78  | |     let drops_42 = Arc::new(AtomicUsize::new(0));
79  | |
80  | |     let x = AtomicPtr::new(Box::into_raw(Box::new((
...   |
160 | |     assert_eq!(drops_9001.load(Ordering::SeqCst), 1);
161 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

Admittedly I haven't dug into it yet, so it could totally be a bug in my program. It was just the match with the README's suggestion for false positives that made me assume this was probably a Miri tracking problem.

@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2022

Yes, that message indicates an int-to-ptr cast. But the question is where that cast is happening...

@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2022

Anyway, AtomicPtr is likely innocent, so I will re-close this issue. Can you open a new issue for this problem?

@RalfJung RalfJung closed this as completed Mar 2, 2022
@jonhoo
Copy link

jonhoo commented Mar 2, 2022

Ugh, looking through the code again, I realized that I do cast between pointers and integers to manage a bit in a pointer as a lock. That's unfortunate, but explains the problem. Sorry for the noise!

Any news on whether Miri might gain capabilities for tracking pointers even in the presence of such casts and bit flips? Potentially with some extra code to tell Miri what's going on through an extern? These kinds of tricks are quite common in concurrency primitive libraries where Miri is invaluable.

@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2022

Please open a new issue with a pointer to the relevant code. :)

@jonhoo
Copy link

jonhoo commented Mar 2, 2022

Filed #1993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-leaks Area: affects the memory leak checker C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants