AsFd impl for Stdout/StdoutLock can violate locking guarantees of those types.

[the8472]

View all comments

The stdout/err APIs guarantee that they're globally synchronized:

Each handle returned is a reference to a shared global buffer whose access is synchronized via a mutex. If you need more explicit control over locking, see the Stdout::lock method.

This can be relied on to write whole lines at a time, e.g. for json-lines style logging where multiple threads shouldn't interleave their output. It can be ok if some random other library or thread outputs non-JSON output, but it's important that it's not emitted in the middle of a line that's currently being emitted.

The combination of impl AsFd for Stdout, try_clone_to_owned() and impl From<OwnedFd> for File can then be used to break that guarantee. AsRawFd is somewhat less of an issue because any use of the created RawFd should be unsafe.

Possible solutions:

• weaken the guarantees
• deprecate or remove the impls
• add "don't do that" warnings

Related: rust-lang/libs-team#148 (comment) and following.

[ChrisDenton]

I think try_clone_to_owned is the weak link in the chain.

Converting back and forth between OwnedFd and File should be fine.

[the8472]

No that's not enough. You can get multiple Stdout instances. They all share the underlying lock, but they have separate lifetimes so you can get another BorrowedFd even when the lock is taken.

Well, I suppose we could acquire the lock before returning the borrowedfd... but we'd have to tell libraries to not use dup() on borrowed descriptors.
But that's still weird because at least part of the motivation of the BorrowedFd lifetime is to prevent double-close. Which the try_clone_to_owned does properly solve. Removing it because Stdout made additional promises that are incompatible with cloning seems like shooting the messenger.

[ChrisDenton]

Fair. But using the fd returned by AsRawFd is unsafe and OwnedFd -> File should be a no-op. Only try_clone_to_owned actually does something. And tbh, there has been some discussion recently about how safe dup actually is.

However, I do see your point.

[sunfishcode]

a shared global buffer whose access is synchronized via a mutex

The mutex protects the buffer.

AsRawFd is somewhat less of an issue because any use of the created RawFd should be unsafe.

Lots of existing Rust code does things like unsafe { libc::ioctl(stdout.as_raw_fd(), libc::TCSETS, ptr) };. This use of an unsafe block means code like this is assuming it knows the safety condition needed to call libc like this. Consequently, we need to be careful before claiming that we can make the unsafety condition stronger.

The existing long-standing situation in Rust is that the raw fd safety condition does not include "you must respect the Stdout lock".

I recommend just adding documentation to warn users of the potential hazards.

[the8472]

Stdout says

Each handle shares a global buffer of data to be written to the standard output stream. Access is also synchronized via a lock and explicit control over locking is available via the lock method.

i.e. access is synchronized, not just the buffer.

[SUPERCILEX]

My vote is for weakening the guarantees, but I also agree that @the8472 is reading too much into the existing guarantees and that they don't say anything about the stdout stream.

Each handle shares a global buffer of data to be written to the standard output stream.

When I read this line I hear "There's a global buffer where everyone can play nice and synchronize their writes to stdout, but that says nothing about others writing to stdout elsewhere".

[the8472]

but that says nothing about others writing to stdout elsewhere".

Nominally that doesn't exist inside Rust. We assume exclusive ownership of file descriptors. This is part of IO Safety.

[SUPERCILEX]

I need to reread the io safety docs, but then doesn't that mean any method of getting a file descriptor through unsafe to 0,1,2 is disallowed? I would be quite annoyed if a systems programming language said you can't mess with those FDs.