rust-lang / rust-lang/rust

std `stack_overflow` QOI benefits do not seem to justify current complexity

Open
#146,352 7 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-collections A-valgrind-full-leak C-bug T-libs
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

I have encountered this issue with rust 1.89.
It was NOT present with rust 1.86

This concerns the file: sys/pal/unix/stack_overflow/thread_info.rs

It contains a static BTreeMap which grows for active threads and shrinks again.
There is a global cleanup handler which gets called before a call to exit()
or by the runtime after main() returns and before the program exits.

Notably the main thread also inserts an element into this map which allocates some internal data structure.

The cleanup handler if all threads have properly terminated will leave the map empty. map.len() == 0.
However the cleanup handler does not get rid of the initial allocation for the now empty map.

Any rust program that uses std will trigger this.

fn main() {
println!("Hello World!");
}

Is enough and will trigger this.

This will trip valgrind on a full leak check (please read on I am aware of the implications of this), because this allocation of the BTreeMap is still reachable.
Valgrind output with build std:

==99723==    at 0x48417B4: malloc (vg_replace_malloc.c:381)
==99723==    by 0x29942A: std::sys::alloc::unix::<impl core::alloc::global::GlobalAlloc for std::alloc::System>::alloc (unix.rs:14)
==99723==    by 0x1DECE9: __rustc::__rdl_alloc (alloc.rs:402)
==99723==    by 0x277638: alloc::alloc::alloc (alloc.rs:95)
==99723==    by 0x2776C3: alloc::alloc::Global::alloc_impl (alloc.rs:190)
==99723==    by 0x279118: <alloc::alloc::Global as core::alloc::Allocator>::allocate (alloc.rs:251)
==99723==    by 0x27823F: alloc::boxed::Box<T,A>::try_new_uninit_in (boxed.rs:508)
==99723==    by 0x278164: alloc::boxed::Box<T,A>::new_uninit_in (boxed.rs:474)
==99723==    by 0x1CFCDE: alloc::collections::btree::node::LeafNode<K,V>::new (node.rs:83)
==99723==    by 0x1C8469: alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Owned,K,V,alloc::collections::btree::node::marker::Leaf>::new_leaf (node.rs:217)
==99723==    by 0x216BEC: alloc::collections::btree::map::entry::VacantEntry<K,V,A>::insert_entry (entry.rs:400)
==99723==    by 0x217074: alloc::collections::btree::map::entry::VacantEntry<K,V,A>::insert (entry.rs:373)

==99723== LEAK SUMMARY:
==99723==    definitely lost: 0 bytes in 0 blocks
==99723==    indirectly lost: 0 bytes in 0 blocks
==99723==      possibly lost: 0 bytes in 0 blocks
==99723==    still reachable: 456 bytes in 1 blocks
==99723==         suppressed: 0 bytes in 0 blocks

Valgrind output without build std:

==104772==    at 0x48417B4: malloc (vg_replace_malloc.c:381)
==104772==    by 0x14AAE6: alloc (alloc.rs:95)
==104772==    by 0x14AAE6: alloc_impl (alloc.rs:190)
==104772==    by 0x14AAE6: allocate (alloc.rs:251)
==104772==    by 0x14AAE6: try_new_uninit_in<alloc::collections::btree::node::LeafNode<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo>, alloc::alloc::Global> (boxed.rs:508)
==104772==    by 0x14AAE6: new_uninit_in<alloc::collections::btree::node::LeafNode<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo>, alloc::alloc::Global> (boxed.rs:474)
==104772==    by 0x14AAE6: new<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo, alloc::alloc::Global> (node.rs:83)
==104772==    by 0x14AAE6: new_leaf<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo, alloc::alloc::Global> (node.rs:217)
==104772==    by 0x14AAE6: insert_entry<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo, alloc::alloc::Global> (entry.rs:400)
==104772==    by 0x14AAE6: insert<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo, alloc::alloc::Global> (entry.rs:373)
==104772==    by 0x14AAE6: insert<usize, std::sys::pal::unix::stack_overflow::thread_info::ThreadInfo, alloc::alloc::Global> (map.rs:1046)
==104772==    by 0x14AAE6: std::sys::pal::unix::stack_overflow::thread_info::set_current_info (thread_info.rs:118)
==104772==    by 0x14932B: init (stack_overflow.rs:166)
==104772==    by 0x14932B: init (mod.rs:48)
==104772==    by 0x14932B: init (rt.rs:114)
==104772==    by 0x14932B: {closure#0} (rt.rs:173)
==104772==    by 0x14932B: do_call<std::rt::lang_start_internal::{closure_env#0}, isize> (panicking.rs:590)
==104772==    by 0x14932B: catch_unwind<isize, std::rt::lang_start_internal::{closure_env#0}> (panicking.rs:553)
==104772==    by 0x14932B: catch_unwind<std::rt::lang_start_internal::{closure_env#0}, isize> (panic.rs:359)
==104772==    by 0x14932B: std::rt::lang_start_internal (rt.rs:171)
==104772==    by 0x138EE6: std::rt::lang_start (rt.rs:205)
==104772==    by 0x138F8D: main (in REDACTED)
==104772== 
==104772== LEAK SUMMARY:
==104772==    definitely lost: 0 bytes in 0 blocks
==104772==    indirectly lost: 0 bytes in 0 blocks
==104772==      possibly lost: 0 bytes in 0 blocks
==104772==    still reachable: 456 bytes in 1 blocks
==104772==         suppressed: 0 bytes in 0 blocks

I am aware that rust does not wish to fix full leak check findings for complicated situations.
However here the fix appears to be trivial so In my opinion it is reasonable to fix it here.

My fix:
I have changed the function delete_current_info in thread_info.rs
which looks like this in the release:

pub fn delete_current_info() {
    let this = errno_location().addr();
    let _lock_guard = LOCK.lock();
    let _spin_guard = spin_lock_in_setup(this);

    // SAFETY: we own the spin lock, so `THREAD_INFO` cannot not be aliased.
    let thread_info = unsafe { &mut *(&raw mut THREAD_INFO) };
    thread_info.remove(&this);
}

To this:

pub fn delete_current_info() {
    let this = errno_location().addr();
    let _lock_guard = LOCK.lock();
    let _spin_guard = spin_lock_in_setup(this);

    // SAFETY: we own the spin lock, so `THREAD_INFO` cannot not be aliased.
    let thread_info = unsafe { &mut *(&raw mut THREAD_INFO) };
    thread_info.remove(&this);
    if thread_info.len() == 0 {
        *thread_info = BTreeMap::new();
    }
}

This solves the problem and hello world no longer trips valgrind on a full leak check.

I think this fix is probably always safe?
Since for as long as any thread is alive thread_info.len() is never 0.
And once it reaches 0 it is never increased again, but even if that happens by something that I have missed,
then the allocation simply happens again, just like it would if the map were inserted into normally the first time
set_current_info is called when the program starts.

I don't see any downsides to this suggested fix and would therefore appreciate it if it or something similar could be implemented as I have to verify that my programs do not leak any memory including still reachable statics.
This would help me a lot.

Sincerely
Alexander Schütz

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in sys/pal/unix/stack_overflow/thread_info.rs, read delete_current_info and the related set_current_info path. Reproduce the reported Hello World case with a full Valgrind leak check, then verify that the empty THREAD_INFO map no longer leaves its allocation reachable without affecting active-thread cleanup.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
operating-systems
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.