`RawFdContainer` seems to be unsound

[eduardosm]

RawFdContainer allows to wrap and close arbitrary file descriptors, which does not seem very safe. In fact, the standard library has this, where the from_raw_fd method is unsafe.

The solution would be to make RawFdContainer::new unsafe, remove impl<T: IntoRawFd> From<T> for RawFdContainer, and possibly implement std::os::unix::io::FromRawFd for RawFdContainer.

[psychon]

RawFdContainer allows to wrap and close arbitrary file descriptors, which does not seem very safe.

Why? How can this lead to memory unsafety? This sounds weird and I have no idea on how an example could look like.

From the docs:

This function is also unsafe as the primitives currently returned have the contract that they are the sole owner of the file descriptor they are wrapping. Usage of this function could accidentally allow violating this contract which can cause memory unsafety in code that relies on it being true.

Well, RawFdContainer does not allow violating the "sole owner" stuff...

[eduardosm]

Well, RawFdContainer does not allow violating the "sole owner" stuff...

A RawFdContainer will close the FD when dropped, that's "taking ownership".

Why? How can this lead to memory unsafety? This sounds weird and I have no idea on how an example could look like.

Something like this:

• A std::fs::File (we'll call it file) is created, allocating a FD number.
• A RawFdContainer is created with RawFdContainer::new(file.as_raw_fd())
• The RawFdContainer is dropped, closing the FD, but file is still living with a dangling FD.

At this point, trying to do something with file will cause an EBADF, which is not "that bad" in terms of safety. However, if a new resource that allocates a FD is open, it might reuse the FD number. In that case, operations on file will unexpectedly affect that new resource.

[psychon]

Random shower thought: A combination of AsRawFd and libc::dup instead.. That way, RawFdContainer would create its own copy of the FD. Any unsafety that stems from concurrent use of the same file description would stay, but at least any unsafety related to same file descripors would go away.

This could also lead to a possibly slightly nicer API. Instead of requiring arguments of type impl Into<RawFdContainer>, the code could use &impl AsRawFd for the arguments.
Alternatively: Is is possible to impl<T> Into<RawFdContainer> for &T? Would that be better? (and another impl for the T instead of &T case).

[eduardosm]

Is is possible to impl<T> Into<RawFdContainer> for &T?

dup can fail, so it would need to be a TryInto.

I think it would be safe to provide traits to convert from individual types (such as impl From<std::fs::File>).

So far, I created #386, which is still WIP

[psychon]

Random data point: nix::unistd::close is safe: https://docs.rs/nix/0.17.0/nix/unistd/fn.close.html

The scenario from #363 (comment) is possible without RawFdContainer. Thus, I feel like either nix::unistd::close is unsound or the current RawFdContainer is sound.

[eduardosm]

Random data point: nix::unistd::close is safe: https://docs.rs/nix/0.17.0/nix/unistd/fn.close.html

The scenario from #363 (comment) is possible without RawFdContainer. Thus, I feel like either nix::unistd::close is unsound or the current RawFdContainer is sound.

I would say that nix::unistd::close is unsound. They also have nix::unistd::write, which feels like something that allows writing into a mmaped FD without much control...

[eduardosm]

I'm closing this given the reasoning in nix-rust/nix#1225

[psychon]

Ah, thanks for opening that issue. That's indeed a helpful description.

[psychon]

Possibly related: rust-lang/rust#72175