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 in LocalWaker #122180

Closed
RalfJung opened this issue Mar 8, 2024 · 3 comments · Fixed by #122244
Closed

Memory leak in LocalWaker #122180

RalfJung opened this issue Mar 8, 2024 · 3 comments · Fixed by #122244
Labels
I-memleak Issue: Runtime memory leak without `mem::forget`. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 8, 2024

The following code leaks memory:

#![feature(local_waker)]
use std::rc::Rc;
use std::task::LocalWake;
use std::task::LocalWaker;

fn main() {
    struct NoopWaker;

    impl LocalWake for NoopWaker {
        fn wake(self: Rc<Self>) {}
    }

    let _waker = LocalWaker::from(Rc::new(NoopWaker));
}

Miri error:

error: memory leaked: alloc1518 (Rust heap, size: 16, align: 8), allocated here:
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:100:9
    |
100 |         __rust_alloc(layout.size(), layout.align())
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: BACKTRACE:
    = note: inside `std::alloc::alloc` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:100:9: 100:52
    = note: inside `std::alloc::Global::alloc_impl` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:183:73: 183:86
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:243:9: 243:39
    = note: inside `alloc::alloc::exchange_malloc` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:332:11: 332:34
    = note: inside `std::boxed::Box::<std::rc::RcBox<main::NoopWaker>>::new` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:218:9: 218:20
    = note: inside `std::rc::Rc::<main::NoopWaker>::new` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/rc.rs:398:27: 398:94
note: inside `main`
   --> src/main.rs:13:35
    |
13  |     let _waker = LocalWaker::from(Rc::new(NoopWaker));
    |                                   ^^^^^^^^^^^^^^^^^^

The equivalent code with Waker is fine, so I think this is a bug.

Cc @dtolnay @tvallotton

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 8, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Mar 8, 2024

I guess this is not too surprising... Waker has

#[stable(feature = "futures_api", since = "1.36.0")]
impl Drop for Waker {
    #[inline]
    fn drop(&mut self) {
        // SAFETY: This is safe because `Waker::from_raw` is the only way
        // to initialize `drop` and `data` requiring the user to acknowledge
        // that the contract of `RawWaker` is upheld.
        unsafe { (self.waker.vtable.drop)(self.waker.data) }
    }
}

but LocalWaker has nothing like that.

@Noratrieb Noratrieb added T-libs Relevant to the library team, which will review and decide on the PR/issue. I-memleak Issue: Runtime memory leak without `mem::forget`. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 8, 2024
@tvallotton
Copy link
Contributor

Oh, that's may bad. I assumed that RawWaker handled ownership. I can push a PR today in the evening.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2024

This leads to a miri-test-libstd failure every day, would be nice to get a fix soon. :)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2024
…=Nilstrieb

fix: LocalWaker memory leak and some stability attributes

fixes rust-lang#122180.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
…=Nilstrieb

fix: LocalWaker memory leak and some stability attributes

fixes rust-lang#122180.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
…=Nilstrieb

fix: LocalWaker memory leak and some stability attributes

fixes rust-lang#122180.
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 10, 2024
…=Nilstrieb

fix: LocalWaker memory leak and some stability attributes

fixes rust-lang#122180.
@bors bors closed this as completed in 4faf535 Mar 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 10, 2024
Rollup merge of rust-lang#122244 - tvallotton:local_waker_leak_fix, r=Nilstrieb

fix: LocalWaker memory leak and some stability attributes

fixes rust-lang#122180.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-memleak Issue: Runtime memory leak without `mem::forget`. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants