Fix potential panics in Drop implementations in rustc_errors::lock#152447
Fix potential panics in Drop implementations in rustc_errors::lock#152447nuxtreact wants to merge 1 commit intorust-lang:mainfrom
Conversation
Replace .unwrap() calls with error ignoring in Drop implementations for Handle and Guard structs to prevent panicking in destructors. Panicking in Drop can lead to process abortion if it occurs during stack unwinding. This change makes the code more robust by ignoring errors from CloseHandle and ReleaseMutex during cleanup. Fixes the FIXME comments about potential panics in drop handlers.
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| // Ignore errors during drop to avoid panicking in a destructor. | ||
| // If CloseHandle fails, the handle will be leaked, but this is | ||
| // preferable to aborting the process. | ||
| let _ = CloseHandle(self.0); |
There was a problem hiding this comment.
Is it even possible for a valid handle to return an error when closing?
| ReleaseMutex((self.0).0).unwrap(); | ||
| // Ignore errors during drop to avoid panicking in a destructor. | ||
| // If ReleaseMutex fails, the mutex will remain locked, but this is | ||
| // preferable to aborting the process. |
There was a problem hiding this comment.
I don't think keeping the mutex locked is preferable. Keeping the mutex locked would cause a deadlock if another rustc instance (or another thread of the same rustc instance in the case of parallel rustc) inside the same process attempts to print a diagnostic. A deadlock is much worse than a crash.
| //! Note that this is currently only needed for allowing only one 32-bit MSVC | ||
| //! linker to execute at once on MSVC hosts, so this is only implemented for | ||
| //! `cfg(windows)`. Also note that this may not always be used on Windows, | ||
| //! only when targeting 32-bit MSVC. |
There was a problem hiding this comment.
Presuming this comment is accurate, I'm not sure how much time it's worth investing in fixing up the implementation here. 32-bit MSVC Windows is already a niche target and all of the panic/abort seem like they'd at worst be a slightly worse error report for something that has gone genuinely wrong (seems acceptable).
|
@rustbot reroll |
Replace .unwrap() calls with error ignoring in Drop implementations for Handle and Guard structs to prevent panicking in destructors.
Panicking in Drop can lead to process abortion if it occurs during stack unwinding. This change makes the code more robust by ignoring errors from CloseHandle and ReleaseMutex during cleanup.
Fixes the FIXME comments about potential panics in drop handlers.