regression: RefCell<LineWriter<std::io::stdio::StdoutRaw>> cannot be shared between threads safely

[Mark-Simulacrum]

The error seems probably correct, but it's not clear why this would have started failing now.

• https://crater-reports.s3.amazonaws.com/beta-1.80-4/beta-2024-06-22/reg/clap-io-0.1.0/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.80-4/beta-2024-06-22/reg/fakelogs-0.1.10-75501d4/log.txt

[INFO] [stdout] error[E0277]: `RefCell<LineWriter<std::io::stdio::StdoutRaw>>` cannot be shared between threads safely
[INFO] [stdout]    --> src/lib.rs:205:20
[INFO] [stdout]     |
[INFO] [stdout] 205 |                 Ok(Box::new(stdout))
[INFO] [stdout]     |                    ^^^^^^^^^^^^^^^^ `RefCell<LineWriter<std::io::stdio::StdoutRaw>>` cannot be shared between threads safely
[INFO] [stdout]     |
[INFO] [stdout]     = help: the trait `Sync` is not implemented for `RefCell<LineWriter<std::io::stdio::StdoutRaw>>`, which is required by `StdoutLock<'_>: Sync`
[INFO] [stdout]     = note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` instead
[INFO] [stdout]     = note: required for `ReentrantLockGuard<'_, RefCell<LineWriter<std::io::stdio::StdoutRaw>>>` to implement `Sync`
[INFO] [stdout] note: required because it appears within the type `StdoutLock<'_>`
[INFO] [stdout]    --> /rustc/64a1fe67112931359c7c9a222f08fd206255c2b5/library/std/src/io/stdio.rs:614:12
[INFO] [stdout]     = note: required for the cast from `Box<StdoutLock<'_>>` to `Box<dyn std::io::Write + Sync>`

[workingjubilee]

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))
}

[workingjubilee]

cargo-bisect-rustc informs us it is rust-lang-ci@fd225fc

This is collateral damage of

• ReentrantLockGuard's Sync impl is unsound #125526
• Add manual Sync impl for ReentrantLockGuard #125527

...apparently, std said it can have a little bit of unsoundness, as a treat? It seems like exposing #121440 is proving to be a good idea because otherwise we would not have had this actually audited.

cc @joboet @programmerjake

[workingjubilee]

We have the options of

1. accepting this regression
   • pros: we don't have to do anything
   • cons: ~2 crates relying on an unsound impl deep inside std are broken
2. making the internals of stdout() a custom internal reentrant lock type with appropriate internal mutability and stuff (and hoping we get it right)
   • pros: the crates build and we can do whatever since this is internal impl details
   • cons: odds are good we get it wrong again
   • small variation: unsafe impl Sync for StdoutLock, has the same "we got it right... right?" problem
3. adjusting ReentrantLockGuard until the original impl was correct
   • pros: probably makes the type more useful?
   • cons: maybe impossible¹, idk, haven't thought about it deeply
4. slapping a Mutex into this bad boy
   • pros: known correct
   • cons: deadlocks ahoy? kinda silly

@rustbot label: I-libs-nominated

Footnotes

1. https://github.com/rust-lang/rust/issues/127340#issuecomment-2210632093 ↩

[workingjubilee]

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.

[Ciel-MC]

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

[workingjubilee]

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.

[joboet]

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.

[joshtriplett]

Having it not be Sync doesn't seem like a problem. Having it not be Send seems sad, but given the crater results, probably acceptable.

[joshtriplett]

We should relnote this as a soundness fix.

[workingjubilee]

Wait, did T-libs ever actually discuss this?

[workingjubilee]

Ah, yes they did on the July 10 meeting, people were like "eh, acceptable".

[workingjubilee]

This problem applies to StderrLock too, it's just no one noticed:

[Mark-Simulacrum]

This was mentioned in the release notes (currently last line of compat notes):

std::sync::ReentrantLockGuard<T> is no longer Sync if T: !Sync which means std::io::StdoutLock and std::io::StderrLock are no longer Sync

Closing.

[workingjubilee]

Fixed in clap-io 0.1.1