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

Unfreed (but still reachable) allocations in std::io::stdin are reported by Valgrind if non-default --show-leak-kinds=all option is specified #80406

Open
boydjohnson opened this issue Dec 27, 2020 · 10 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@boydjohnson
Copy link

There was a memory leak introduced in between nightly-2020-09-27 and nightly-2020-09-28.

If you build this code with two different toolchains one will have a memory leak and the other not.

use std::io::stdin;
use std::io::Read;

fn main() {
    let mut buf = vec![];

    if let Err(e) = stdin().read(&mut buf) {
        println!("{}", e)
    }
}

this code built with nightly-2020-09-28

valgrind output

==11076== HEAP SUMMARY:
==11076==     in use at exit: 8,232 bytes in 2 blocks
==11076==   total heap usage: 13 allocs, 11 frees, 10,409 bytes allocated

while this code built with nightly-2020-09-27:

valgrind output

==11601== HEAP SUMMARY:
==11601==     in use at exit: 0 bytes in 0 blocks
==11601==   total heap usage: 18 allocs, 18 frees, 10,577 bytes allocated

valgrind --leak-check=full --show-leak-kinds=all

==11920== HEAP SUMMARY:
==11920==     in use at exit: 8,232 bytes in 2 blocks
==11920==   total heap usage: 13 allocs, 11 frees, 10,409 bytes allocated
==11920== 
==11920== 40 bytes in 1 blocks are still reachable in loss record 1 of 2
==11920==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11920==    by 0x120934: alloc (alloc.rs:74)
==11920==    by 0x120934: alloc_impl (alloc.rs:153)
==11920==    by 0x120934: alloc (alloc.rs:212)
==11920==    by 0x120934: exchange_malloc (alloc.rs:302)
==11920==    by 0x120934: new<std::io::buffered::BufReader<std::io::stdio::StdinRaw>> (mutex.rs:222)
==11920==    by 0x120934: {{closure}} (stdio.rs:299)
==11920==    by 0x120934: {{closure}}<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0> (lazy.rs:245)
==11920==    by 0x120934: {{closure}}<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0,!> (lazy.rs:372)
==11920==    by 0x120934: std::sync::once::Once::call_once_force::{{closure}} (once.rs:321)
==11920==    by 0x120F84: std::sync::once::Once::call_inner (once.rs:419)
==11920==    by 0x1211BB: call_once_force<closure-0> (once.rs:321)
==11920==    by 0x1211BB: std::lazy::SyncOnceCell<T>::initialize (lazy.rs:371)
==11920==    by 0x11E2CB: get_or_try_init<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0,!> (lazy.rs:292)
==11920==    by 0x11E2CB: get_or_init<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0> (lazy.rs:245)
==11920==    by 0x11E2CB: std::io::stdio::stdin (stdio.rs:298)
==11920==    by 0x10D902: mem_leak::main (main.rs:7)
==11920==    by 0x10DCAA: core::ops::function::FnOnce::call_once (function.rs:227)
==11920==    by 0x10D11D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137)
==11920==    by 0x10DBB0: std::rt::lang_start::{{closure}} (rt.rs:66)
==11920==    by 0x123F7F: call_once<(),Fn<()>> (function.rs:259)
==11920==    by 0x123F7F: do_call<&Fn<()>,i32> (panicking.rs:381)
==11920==    by 0x123F7F: try<i32,&Fn<()>> (panicking.rs:345)
==11920==    by 0x123F7F: catch_unwind<&Fn<()>,i32> (panic.rs:382)
==11920==    by 0x123F7F: std::rt::lang_start_internal (rt.rs:51)
==11920==    by 0x10DB86: std::rt::lang_start (rt.rs:65)
==11920==    by 0x10DB39: main (in /home/boydjohnson/mem-leak/target/debug/mem-leak)
==11920== 
==11920== 8,192 bytes in 1 blocks are still reachable in loss record 2 of 2
==11920==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11920==    by 0x120910: alloc (alloc.rs:74)
==11920==    by 0x120910: alloc_impl (alloc.rs:153)
==11920==    by 0x120910: alloc (alloc.rs:212)
==11920==    by 0x120910: allocate_in<u8,alloc::alloc::Global> (raw_vec.rs:188)
==11920==    by 0x120910: with_capacity_in<u8,alloc::alloc::Global> (raw_vec.rs:163)
==11920==    by 0x120910: with_capacity<u8> (raw_vec.rs:93)
==11920==    by 0x120910: with_capacity<u8> (vec.rs:363)
==11920==    by 0x120910: with_capacity<std::io::stdio::StdinRaw> (buffered.rs:103)
==11920==    by 0x120910: {{closure}} (stdio.rs:299)
==11920==    by 0x120910: {{closure}}<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0> (lazy.rs:245)
==11920==    by 0x120910: {{closure}}<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0,!> (lazy.rs:372)
==11920==    by 0x120910: std::sync::once::Once::call_once_force::{{closure}} (once.rs:321)
==11920==    by 0x120F84: std::sync::once::Once::call_inner (once.rs:419)
==11920==    by 0x1211BB: call_once_force<closure-0> (once.rs:321)
==11920==    by 0x1211BB: std::lazy::SyncOnceCell<T>::initialize (lazy.rs:371)
==11920==    by 0x11E2CB: get_or_try_init<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0,!> (lazy.rs:292)
==11920==    by 0x11E2CB: get_or_init<std::sync::mutex::Mutex<std::io::buffered::BufReader<std::io::stdio::StdinRaw>>,closure-0> (lazy.rs:245)
==11920==    by 0x11E2CB: std::io::stdio::stdin (stdio.rs:298)
==11920==    by 0x10D902: mem_leak::main (main.rs:7)
==11920==    by 0x10DCAA: core::ops::function::FnOnce::call_once (function.rs:227)
==11920==    by 0x10D11D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137)
==11920==    by 0x10DBB0: std::rt::lang_start::{{closure}} (rt.rs:66)
==11920==    by 0x123F7F: call_once<(),Fn<()>> (function.rs:259)
==11920==    by 0x123F7F: do_call<&Fn<()>,i32> (panicking.rs:381)
==11920==    by 0x123F7F: try<i32,&Fn<()>> (panicking.rs:345)
==11920==    by 0x123F7F: catch_unwind<&Fn<()>,i32> (panic.rs:382)
==11920==    by 0x123F7F: std::rt::lang_start_internal (rt.rs:51)
==11920==    by 0x10DB86: std::rt::lang_start (rt.rs:65)
==11920==    by 0x10DB39: main (in /home/boydjohnson/mem-leak/target/debug/mem-leak)
==11920== 
==11920== LEAK SUMMARY:
==11920==    definitely lost: 0 bytes in 0 blocks
==11920==    indirectly lost: 0 bytes in 0 blocks
==11920==      possibly lost: 0 bytes in 0 blocks
==11920==    still reachable: 8,232 bytes in 2 blocks
==11920==         suppressed: 0 bytes in 0 blocks
@jonas-schievink
Copy link
Contributor

rustc 1.48.0-nightly (623fb90 2020-09-26)
rustc 1.48.0-nightly (7f7a1cb 2020-09-27)

Commits in that range: 623fb90...7f7a1cb

@jonas-schievink
Copy link
Contributor

#77154 looks like it might cause this. @m-ou-se can you tell if this is an actual leak or a false positive?

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 27, 2020
@m-ou-se m-ou-se self-assigned this Dec 27, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Dec 27, 2020

I'm a bit busy right now. I'll look at it later, but I think this is a false positive (leaked on purpose).

@m-ou-se
Copy link
Member

m-ou-se commented Dec 28, 2020

Before that change, the buffer for standard input was deallocated in an atexit() handler. After that change, it was simply not deallocated to simplify the code, since it'd always live to the end of the program anyway. So this leak is indeed intentional.

@joshtriplett
Copy link
Member

joshtriplett commented Jan 26, 2022

@m-ou-se That's still somewhat problematic; people use valgrind to track down leaks, and intentional leaks make that harder.

We could address this via a patch to valgrind's standard "suppressions" mechanism.

@yaahc yaahc added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. labels Feb 8, 2022
@nnethercote nnethercote assigned nnethercote and unassigned m-ou-se Feb 8, 2022
@nnethercote
Copy link
Contributor

First, this memory is still reachable, so it's not a true leak. You have to run Valgrind with --show-leak-kinds=all/--show-leak-kinds=reachable to even see these listed individually, and you have to run Valgrind with --errors-for-leak-kinds=all/--errors-for-leak-kinds=reachable to have then considered as actual errors. Only people who are very particular about not having any unfreed memory at termination are likely to use those options. I suspect the number of people like that is relatively small, so this is not as bad as first sounds. I have changed the title accordingly.

Having said that, Valgrind has a number of built-in suppressions for still-reachable "leaks" of this kind in system libraries, so it would be reasonable to add one for this case. I am happy to do that, so I have assigned this issue to me. Note that the fix will be in Valgrind, rather than in Rust. Also, Valgrind releases are unfortunately somewhat infrequent, so this change will almost certainly hit a Rust release before any fix makes it into a Valgrind release.

@joshtriplett
Copy link
Member

joshtriplett commented Feb 8, 2022

@nnethercote Thank you for clarifying the nature of the valgrind warning here; I agree that we should worry less about "reachable" leaks, though we should add a suppression for them.

I'd like to avoid "unreachable" leaks that valgrind would flag by default, and not just suppress those.

@nnethercote
Copy link
Contributor

nnethercote commented Feb 8, 2022

I'd like to avoid "unreachable" leaks that valgrind would flag by default, and not just suppress those.

Yes, these are "true" memory leaks and should be fixed in any code under our control.

@nnethercote nnethercote changed the title Memory leak in std::io::stdin. Memory unfreed in std::io::stdin. Feb 8, 2022
@nnethercote nnethercote changed the title Memory unfreed in std::io::stdin. Unfreed (but still reachable) allocations in std::io::stdin are reported by Valgrind if non-default --show-leak-kinds=all option is specified Feb 8, 2022
@nnethercote
Copy link
Contributor

Also, Valgrind releases are unfortunately somewhat infrequent, so this change will almost certainly hit a Rust release before any fix makes it into a Valgrind release.

Oh, I overlooked the fact that this PR is over a year old. I guess the lack of additional comments confirms that it's something of a niche issue.

@nnethercote
Copy link
Contributor

There's a Valgrind bug that's making this one hard to fix: https://bugs.kde.org/show_bug.cgi?id=452058 It affects suppressions involving inlined stack frame, which are possibly more common in Rust code than C/C++ code due to rustc's aggressive use of inlining.

@jieyouxu jieyouxu added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants