std: don't leak the thread closure if destroying the thread attributes fails #148026
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use |
| let mut attr = DropGuard::new(&mut attr, |attr| { | ||
| assert_eq!(libc::pthread_attr_destroy(attr.as_mut_ptr()), 0) | ||
| }); |
There was a problem hiding this comment.
Perhaps you could add a comment here to explain the relevant considerations of using DropGuard here?
There was a problem hiding this comment.
One thing I notice is that before, libc::pthread_attr_destroy(attr.as_mut_ptr()) is not called for #[cfg(any(target_os = "espidf", target_os = "nuttx"))], but with this change it will/could be. Is that intended?
There was a problem hiding this comment.
Yes, that is very much intended. The leakage of the current version is a bug. And do you really think that this needs documentation? The DropGuard just makes sure that the attribute structure is destroyed, which is pretty self-explanatory to me.
There was a problem hiding this comment.
The what is clear enough, but the why could use some expanding. If it was completely straightforward, then this bug wouldn't have needed fixing, so I do think explaining why the DropGuard is needed would be good.
There was a problem hiding this comment.
IMO, this is fine as-is. The bug existed precisely because the resources (Box and attr) weren't guarded appropriately with DropGuard or equivalent. That's fixed now.
| // Round up to the nearest page and try again. | ||
| let page_size = os::page_size(); | ||
| let stack_size = | ||
| (stack_size + page_size - 1) & (-(page_size as isize - 1) as usize - 1); |
There was a problem hiding this comment.
nit: I wonder if we can make this be stack_size.next_multiple_of(page_size)? I'm not sure why we need to handwrite the code... I guess maybe it's a bit slower since LLVM doesn't know page_size is a multiple of 2, but that seems unimportant in this context?
(Obviously pre-existing...)
|
@bors r+ |
bors
added
S-waiting-on-bors
…=Mark-Simulacrum std: don't leak the thread closure if destroying the thread attributes fails The comment about double-free is wrong – we can safely drop both the thread attributes and the thread closure. Here, I've used `DropGuard` for the attributes and moved the `Box::into_raw` to just before the `pthread_create`.
Rollup of 7 pull requests Successful merges: - #146573 (Constify Range functions) - #146699 (Add `is_ascii` function optimized for LoongArch64 for [u8]) - #148026 (std: don't leak the thread closure if destroying the thread attributes fails) - #148135 (Ignore unix socket related tests for VxWorks) - #148211 (clippy fixes and code simplification) - #148395 (Generalize branch references) - #148405 (Fix suggestion when there were a colon already in generics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148026 - joboet:dont-leak-thread-closure, r=Mark-Simulacrum std: don't leak the thread closure if destroying the thread attributes fails The comment about double-free is wrong – we can safely drop both the thread attributes and the thread closure. Here, I've used `DropGuard` for the attributes and moved the `Box::into_raw` to just before the `pthread_create`.
Rollup of 7 pull requests Successful merges: - rust-lang/rust#146573 (Constify Range functions) - rust-lang/rust#146699 (Add `is_ascii` function optimized for LoongArch64 for [u8]) - rust-lang/rust#148026 (std: don't leak the thread closure if destroying the thread attributes fails) - rust-lang/rust#148135 (Ignore unix socket related tests for VxWorks) - rust-lang/rust#148211 (clippy fixes and code simplification) - rust-lang/rust#148395 (Generalize branch references) - rust-lang/rust#148405 (Fix suggestion when there were a colon already in generics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - rust-lang/rust#146573 (Constify Range functions) - rust-lang/rust#146699 (Add `is_ascii` function optimized for LoongArch64 for [u8]) - rust-lang/rust#148026 (std: don't leak the thread closure if destroying the thread attributes fails) - rust-lang/rust#148135 (Ignore unix socket related tests for VxWorks) - rust-lang/rust#148211 (clippy fixes and code simplification) - rust-lang/rust#148395 (Generalize branch references) - rust-lang/rust#148405 (Fix suggestion when there were a colon already in generics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - rust-lang/rust#146573 (Constify Range functions) - rust-lang/rust#146699 (Add `is_ascii` function optimized for LoongArch64 for [u8]) - rust-lang/rust#148026 (std: don't leak the thread closure if destroying the thread attributes fails) - rust-lang/rust#148135 (Ignore unix socket related tests for VxWorks) - rust-lang/rust#148211 (clippy fixes and code simplification) - rust-lang/rust#148395 (Generalize branch references) - rust-lang/rust#148405 (Fix suggestion when there were a colon already in generics) r? `@ghost` `@rustbot` modify labels: rollup
5 participants
The comment about double-free is wrong – we can safely drop both the thread attributes and the thread closure. Here, I've used
DropGuardfor the attributes and moved theBox::into_rawto just before thepthread_create.