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

Valgrind full leak check reports a leak on Rust 1.83.0 #133574

Closed
sharksforarms opened this issue Nov 28, 2024 · 12 comments
Closed

Valgrind full leak check reports a leak on Rust 1.83.0 #133574

sharksforarms opened this issue Nov 28, 2024 · 12 comments
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@sharksforarms
Copy link

sharksforarms commented Nov 28, 2024

Hi all, creating this issue as I noticed our CI failing due to valgrind when updating from 1.82.0 to 1.83.0, the leak seems to be from rust and not from the application (reproducible with default cargo new --bin). Others may also hit this.

Code

# cargo new --bin testvalgrind

$ cat rust-toolchain.toml
[toolchain]
channel = "1.82.0"

$ cargo build && valgrind --leak-check=full target/debug/testvalgrind
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
==1002693== Memcheck, a memory error detector
==1002693== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1002693== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==1002693== Command: target/debug/testvalgrind
==1002693==
Hello, world!
==1002693==
==1002693== HEAP SUMMARY:
==1002693==     in use at exit: 0 bytes in 0 blocks
==1002693==   total heap usage: 10 allocs, 10 frees, 3,184 bytes allocated
==1002693==
==1002693== All heap blocks were freed -- no leaks are possible
==1002693==
==1002693== For lists of detected and suppressed errors, rerun with: -s
==1002693== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

---

$ cat rust-toolchain.toml
[toolchain]
channel = "1.83.0"

$ cargo build && valgrind --leak-check=full target/debug/testvalgrind
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
==1002883== Memcheck, a memory error detector
==1002883== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1002883== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==1002883== Command: target/debug/testvalgrind
==1002883==
Hello, world!
==1002883==
==1002883== HEAP SUMMARY:
==1002883==     in use at exit: 56 bytes in 1 blocks
==1002883==   total heap usage: 9 allocs, 8 frees, 3,152 bytes allocated
==1002883==
==1002883== 56 bytes in 1 blocks are possibly lost in loss record 1 of 1
==1002883==    at 0x4848853: malloc (vg_replace_malloc.c:381)
==1002883==    by 0x1290D0: std::rt::lang_start_internal (alloc.rs:99)
==1002883==    by 0x10EA19: std::rt::lang_start (rt.rs:194)
==1002883==    by 0x10E9DD: main (in /tmp/testvalgrind/target/debug/testvalgrind)
==1002883==
==1002883== LEAK SUMMARY:
==1002883==    definitely lost: 0 bytes in 0 blocks
==1002883==    indirectly lost: 0 bytes in 0 blocks
==1002883==      possibly lost: 56 bytes in 1 blocks
==1002883==    still reachable: 0 bytes in 0 blocks
==1002883==         suppressed: 0 bytes in 0 blocks
==1002883==
==1002883== For lists of detected and suppressed errors, rerun with: -s
==1002883== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I expected to see this happen: valgrind reports no leak

Instead, this happened: valgrind reports a leak

Version it worked on

It most recently worked on: Rust 1.82.0

Version with regression

rustc --version --verbose:

rustc 1.83.0 (90b35a623 2024-11-26)
@sharksforarms sharksforarms added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Nov 28, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Nov 28, 2024
@sharksforarms sharksforarms changed the title Regression Memory leak regression on Rust 1.83.0 Nov 28, 2024
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-memleak Issue: Runtime memory leak without `mem::forget`. labels Nov 28, 2024
@ChrisDenton
Copy link
Member

This is almost certainly a deliberate leak. You seem to be using a fairly old version of valgrind. It's quite possible a newer version would recognise it's not a leak.

@sharksforarms
Copy link
Author

This is almost certainly a deliberate leak.

looks like it could be, reported in case it isn't (different behavior than 1.82.0)

You seem to be using a fairly old version of valgrind. It's quite possible a newer version would recognise it's not a leak.

the latest valgrind gives us more info

$ ./valgrind-3.24.0/inst/bin/valgrind --leak-check=full target/debug/testvalgrind
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
==1037298== Memcheck, a memory error detector
==1037298== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==1037298== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info
==1037298== Command: target/debug/testvalgrind
==1037298==
Hello, world!
==1037298==
==1037298== HEAP SUMMARY:
==1037298==     in use at exit: 56 bytes in 1 blocks
==1037298==   total heap usage: 9 allocs, 8 frees, 3,152 bytes allocated
==1037298==
==1037298== 56 bytes in 1 blocks are possibly lost in loss record 1 of 1
==1037298==    at 0x484880F: malloc (vg_replace_malloc.c:446)
==1037298==    by 0x1290D0: alloc (alloc.rs:99)
==1037298==    by 0x1290D0: alloc_impl (alloc.rs:192)
==1037298==    by 0x1290D0: allocate (alloc.rs:254)
==1037298==    by 0x1290D0: {closure#0}<std::thread::Inner> (sync.rs:483)
==1037298==    by 0x1290D0: allocate_for_layout<core::mem::maybe_uninit::MaybeUninit<std::thread::Inner>, alloc::sync::{impl#14}::new_uninit::{closure_env#0}<std::thread::Inner>, fn(*mut u8) -> *mut alloc::sync::ArcInner<core::mem::maybe_uninit::MaybeUninit<std::thread::Inner>>> (sync.rs:1925)
==1037298==    by 0x1290D0: new_uninit<std::thread::Inner> (sync.rs:481)
==1037298==    by 0x1290D0: new_inner (mod.rs:1343)
==1037298==    by 0x1290D0: new_main (mod.rs:1333)
==1037298==    by 0x1290D0: init (rt.rs:113)
==1037298==    by 0x1290D0: {closure#0} (rt.rs:172)
==1037298==    by 0x1290D0: do_call<std::rt::lang_start_internal::{closure_env#0}, ()> (panicking.rs:557)
==1037298==    by 0x1290D0: try<(), std::rt::lang_start_internal::{closure_env#0}> (panicking.rs:520)
==1037298==    by 0x1290D0: catch_unwind<std::rt::lang_start_internal::{closure_env#0}, ()> (panic.rs:358)
==1037298==    by 0x1290D0: std::rt::lang_start_internal (rt.rs:172)
==1037298==    by 0x10EA19: std::rt::lang_start (rt.rs:194)
==1037298==    by 0x10E9DD: main (in /tmp/testvalgrind/target/debug/testvalgrind)
==1037298==
==1037298== LEAK SUMMARY:
==1037298==    definitely lost: 0 bytes in 0 blocks
==1037298==    indirectly lost: 0 bytes in 0 blocks
==1037298==      possibly lost: 56 bytes in 1 blocks
==1037298==    still reachable: 0 bytes in 0 blocks
==1037298==         suppressed: 0 bytes in 0 blocks
==1037298==
==1037298== For lists of detected and suppressed errors, rerun with: -s
==1037298== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@lnicola
Copy link
Member

lnicola commented Nov 28, 2024

This actually seems to be fixed in beta, so I think it's a 1.83 regression. It seemed a bit annoying to deal with because I only noticed it in the unit test runner (cargo test in a binary crate), which is hard to pick on CI, and my local suppression didn't work on GHA.

EDIT, since that probably didn't make sense: we're using cargo-valgrind on CI, which uses a socket to talk to valgrind, and VALGRINDFLAGS="--gen-suppressions=all doesn't do anything. Running valgrind by hand works, but for unit tests the binary path is harder to find.

@lqd
Copy link
Member

lqd commented Nov 28, 2024

Can you cargo-bisect-rustc the PR that fixed it, to see if we'd like to backport it?

@workingjubilee
Copy link
Member

According to lnicola, this is fixed on beta.

@workingjubilee
Copy link
Member

As discussed regarding intentional "leaks", "valgrind reports a leak" does not mean "a memory leak occurred". There are many APIs in the standard library that create a small, permanent allocation, and then deliberately defer collection of that memory to process exit. They do not grow. A memory leak, as far as we are concerned, is unbounded memory usage, and this is not.

And this is already fixed, so.

Closing.

@workingjubilee workingjubilee changed the title Memory leak regression on Rust 1.83.0 Valgrind reports a leak on Rust 1.83.0 Nov 28, 2024
@workingjubilee workingjubilee added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-memleak Issue: Runtime memory leak without `mem::forget`. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 28, 2024
@Noratrieb
Copy link
Member

While it's not necessarily a bug, I think keepikg the Rust runtime free of memory leaks as far as Valgrind is concerned is good, to ensure people can use Valgrind to find leaks in their code.
I think we should keep this open in case someone figures out what the fix was and depending on what it is, I wouldn't exclude a stable backport from the possible actions in case a stable release happens. I expect other people to hit this too, so keeping it open makes it more visible too.

@Noratrieb Noratrieb reopened this Nov 28, 2024
@workingjubilee
Copy link
Member

@Noratrieb valgrind's documentation explains this problem, that they should be ignored via generating suppressions, and that it should not be run with the full leak check in most cases. Should we support using valgrind in contravention of its documentation?

@Noratrieb
Copy link
Member

Ah, this is a non-default full leak check, I see. Fair in that case, I will rename the issue to make that more explicit and close it again, I don't think we need to support that in this case.

@sharksforarms
Copy link
Author

sharksforarms commented Nov 28, 2024

Thanks for the comments. Given the above, I suppose the current options for valgrind (w/ --leak-check=full) users + rust stable are:

  • Update to 1.83.0 and create a suppression file to ignore this; or
  • Users can skip 1.83.0 and it should be fixed in the next release, with no guarantees in future releases when using --leak-check=full

@Noratrieb Noratrieb changed the title Valgrind reports a leak on Rust 1.83.0 Valgrind full leak check reports a leak on Rust 1.83.0 Nov 28, 2024
@rustbot rustbot added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Nov 28, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 28, 2024
@lnicola
Copy link
Member

lnicola commented Nov 28, 2024

There are many APIs in the standard library that create a small, permanent allocation, and then deliberately defer collection of that memory to process exit.

This is not a permanent allocation, those are reported as "still reachable" by Valgrind. This is reported as "possibly lost". One example is:

char *p = malloc(2);
p += 1;
// the allocation is possibly lost here
p -= 1;
// we recovered it, false alarm

I bisected, it was introduced in fd1f8aa and fixed in f61306d.

Unfortunately, cargo valgrind reports these as errors, and it's very hard to create a suppression for it.

@jieyouxu jieyouxu removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 28, 2024
@workingjubilee
Copy link
Member

Thank you for confirming that, @lnicola.

I don't think we should backport that commit if there is a stable release.

But if this regression been caught on beta or nightly, @sharksforarms, we would have made some effort to make it happen so it didn't reach the release. That's not a guarantee, no, but we do still appreciate that this is an inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. 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

9 participants