Open Mark-Simulacrum opened 4 days ago
Minimized:
use std::io::{self, Write};
pub fn open_stdout() -> io::Result<Box<dyn Write + Sync + 'static>> {
let stdout = Box::leak(Box::new(io::stdout()));
let stdout = stdout.lock();
Ok(Box::new(stdout))
}
cargo-bisect-rustc informs us it is https://github.com/rust-lang-ci/rust/commit/fd225fc8ab992fbb00eb7bb188b3333bfb57dcc0
This is collateral damage of
...apparently, std said it can have a little bit of unsoundness, as a treat? It seems like exposing https://github.com/rust-lang/rust/issues/121440 is proving to be a good idea because otherwise we would not have had this actually audited.
cc @joboet @programmerjake
We have the options of
stdout()
a custom internal reentrant lock type with appropriate internal mutability and stuff (and hoping we get it right)
unsafe impl Sync for StdoutLock
, has the same "we got it right... right?" problem@rustbot label: I-libs-nominated
The other one minimizes to this:
use std::io;
fn chunk() -> Result<(), io::Error> {
let stdout = io::stdout();
let mut handle = stdout.lock();
write_csv(&mut handle)
}
pub fn write_csv(_w: &mut (dyn io::Write + Sync)) -> Result<(), io::Error> {
Ok(())
}
In both cases, we can see that the problem is only caused if someone deliberately erases the type of the StdoutLock and instead makes it a &mut dyn io::Write + Sync
, a transition it can no longer fulfill.
Can we try and see how much of crates break if we do 1? 2 feels like too much effort for little gain and 3 is not very pretty.
Disclaimer: Absolute stdlib noob, just offering my first thoughts because it’s brought into my attention
Based on @Mark-Simulacrum's original post, it seems only 2 crates with no dependents on crates.io, one of which seems to be an abandoned toy project. That strongly suggests people generally aren't doing this. There's lots to recommend taking the "shrug and do nothing" option, I just wanted to be thorough.
If 3 ("fix the Guard to be actually-Sync") is doable without sacrificing large parts of ReentrantLock's functionality (or performance?) I would say it's the hands-down winner. I just have given it literally zero thought, though it sounds more like wishful thinking than plausible.
Changing ReentrantLockGuard
is out of the question: you can get a &T
from a &ReentrantLockGuard<T>
, so T
must be Sync
for ReentrantLockGuard
to be.
Fixing StdoutLock
doesn't really make sense, as it obviates the point of locking: If we can get a &StdoutLock
on multiple threads, we have to make the inner state a Mutex
(this won't cause any deadlocks by the way, as the Mutex
is only locked while executing std
-internal code). So why have a lock
method at all, then?
Therefore, I strongly prefer option 1: this was a soundness bug in std
, so a breaking change is entirely justified.
The error seems probably correct, but it's not clear why this would have started failing now.