jhpratt / num_threads

Obtain the number of threads in the current process
https://docs.rs/num_threads
Apache License 2.0
12 stars 7 forks source link

Memory leak in `num_threads::is_single_threaded()` on Apple platforms #16

Closed aw-cf closed 9 months ago

aw-cf commented 9 months ago

The implementation for num_threads::is_single_threaded() is leaking thread_count mach ports on every call.

This is because thread_state is ignored currently. The correct usage of this API requires calling mach_port_deallocate and vm_deallocate. One example of using this API without leaking memory would be can be found here.

This issue was introduced with #6 (https://github.com/jhpratt/num_threads/commit/b12a1c8cea0f5dbd4e83af41670c5dfaa3add0cf) and it could make sense to revert this change given fixing the current code would likely lead to more allocations/deallocations than the previous implementation.

jhpratt commented 9 months ago

I have no problem fixing a memory leak, but reverting #6 is not an option given that it would require going back to using non-public APIs.

thibault-ml commented 9 months ago

Fixing forward is definitely fine. It should be easy to fix. mach_port_deallocate needs to be called for each thread_state[x], and then vm_deallocate called on thread_state with thread_count * sizeof(mach_port_t)

Also, I believe we shouldn't need to manually declare the interface of task_threads or task_inspect_t, since they are already defined in Rust's libc: https://github.com/rust-lang/libc/blob/main/src/unix/bsd/apple/mod.rs#L6136-L6140

thibault-ml commented 9 months ago

@jhpratt We've got a working fix, we'll put up a PR today.

aw-cf commented 9 months ago

I have no problem fixing a memory leak, but reverting #6 is not an option given that it would require going back to using non-public APIs.

That's a valid point indeed