Fix #95126: io::cleanup() can panic in unusual circumstances#95181
Fix #95126: io::cleanup() can panic in unusual circumstances#95181jswrenn wants to merge 2 commits intorust-lang:masterfrom
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #95180) made this pull request unmergeable. Please resolve the merge conflicts. |
| // leak the old, buffered LineWriter to avoid deallocations this | ||
| // ensures that writes to stdout *during* cleanup (e.g., writes | ||
| // emitted by a custom global allocator) won't deadlock (#95126) | ||
| crate::mem::forget(buffered); |
There was a problem hiding this comment.
I'm not sure I follow why this needs to be leaked -- it seems like what we need to do is make sure that the lock has been released before we drop it, but that's something that we can do easily enough (just moving the drop outside the lock.try_borrow_mut).
| // otherwise cause a deadlock here. | ||
| if let Some(lock) = Pin::static_ref(instance).try_lock() { | ||
| *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw()); | ||
| if let Ok(mut refmut) = lock.try_borrow_mut() { |
There was a problem hiding this comment.
Can you help me understand this try_borrow_mut? the try_lock above is justified by leaking a StdoutLock, so presumably the panic on borrow_mut() here would occur when someone leaks a locked RefCell -- but that RefCell is never exposed, and std shouldn't be leaking it (right?)...
I think this could also happen if cleanup() is invoked while the RefCell is locked on the stack above us, but I wouldn't expect that to happen either.
| // replace the current LineWriter with the unbuffered one | ||
| let mut buffered = crate::mem::replace(&mut *refmut, unbuffered); | ||
| // flush the old, buffered LineWriter | ||
| let _ = buffered.flush(); |
There was a problem hiding this comment.
I would loosely expect that we should move this flushing out of the STDOUT lock.
Mark-Simulacrum
added
S-waiting-on-author
|
@jswrenn any updates on this? |
|
Closing this as the pr is inactive. Feel free to open a new PR if you wish to continue working on this |
Dylan-DPC
added
S-inactive
6 participants
Fixes #95126 by modifying
io::cleanup()to flush stdout without dropping itsLineWriter.