std: Make FromRawFd::from_raw_fd an unsafe method

[alexcrichton]

As pointed out in RFC issue 1043 it is quite useful to have the standard
I/O types to provide the contract that they are the sole owner of the underlying
object they represent. This guarantee enables writing safe interfaces like the
MemoryMap API sketched out in that issue.

As constructing objects from these raw handles may end up violating these
ownership gurantees, the functions for construction are now marked unsafe.

[breaking-change]
Closes rust-lang/rfcs#1043

[alexcrichton]

r? @aturon

[alexcrichton]

cc @Stebalien

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[Stebalien]

Thanks. lgtm (except I think you meant to close rust-lang/rfcs#1043).

[alexcrichton]

Oops I did indeed!

[aturon]

@bors: r+

[bors]

📌 Commit 2705051 has been approved by aturon

[bors]

⌛ Testing commit 2705051 with merge c5d347a...

[bors]

⛄ The build was interrupted to prioritize another pull request.

[bors]

⌛ Testing commit 2705051 with merge ef6f489...

[pnkfelix]

@bors r=aturon 2705051

[bors]

💔 Test failed - auto-win-32-nopt-t

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 2705051 with merge 3f8fed6...

[bors]

💔 Test failed - auto-win-32-nopt-t

[alexcrichton]

@bors: retry

On Mon, Apr 13, 2015 at 5:01 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-32-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/4158

—
Reply to this email directly or view it on GitHub
#24251 (comment).

[bors]

⌛ Testing commit 2705051 with merge e6a8124...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[l0kod]

I'm a bit late but this issue was already pointed out by #19169 some months ago.

Why not use the file descriptor's owner lifetime for the return object to guarantee a safe file descriptor use?

Replacing unsafe fn from_raw_fd(fd: RawFd) -> Self with fn from_raw_fd<'a, T>(fd: mut T) -> Self<'a> where T: AsRawFd, T: 'a.
This way, the file descriptor would be cleanly close by the AsRawFd implementation while being safe to use by the FromRawFd implementation.

It sounds reasonable to use a mutable AsRawFd implementation to get a unique file descriptor user (could be different than the owner).
The fn as_raw_fd(&self) -> RawFd could be changed to fn as_raw_fd(&mut self) -> RawFd.

[Stebalien]

1. How would I use that to have a type borrow a file descriptor. In your proposal, the type consumes the AsRawFd struct.
2. That doesn't guarantee that the file descriptor is the right kind of file descriptor.
3. What if I want the FromRawFd type to close the file descriptor when done. That is, what If I want to manually open a file and then create a File object from this manually opened file descriptor.
4. The FromRawFd function as described would either require that the implementing type be generic over T or store T in a box.

[l0kod]

How would I use that to have a type borrow a file descriptor. In your proposal, the type consumes the AsRawFd struct.

I though AsRawFd was implemented for &mut File as well but it's not for now. However it could do the same as AsMut does: impl<'a, T, U> AsRawFd<U> for &'a mut T where U: ?Sized, T: AsRawFd<U> + ?Sized

This way, the AsRawFd could be a struct or a reference, which allow to move an object or to pass a reference.

That doesn't guarantee that the file descriptor is the right kind of file descriptor.

The FromRawFd implementation functions (e.g. write()) will return an error if the OS doesn't allow I/O operations on the file descriptor anyway.
The from_raw_fd could also return Result<FromRawFd, FromRawFdError> but that's another issue.

What if I want the FromRawFd type to close the file descriptor when done. That is, what If I want to manually open a file and then create a File object from this manually opened file descriptor.

AsRawFd implemented for &mut File solve this issue. We could pass an object who will then be closed at the FromRawFd implementation drop, or we could safely pass a reference and reuse the AsRawFd implementation after the FromRawFd implementation will be dropped.

The FromRawFd function as described would either require that the implementing type be generic over T or store T in a box.

Yes, the object's inner file descriptor should go from RawFd to T where T: AsRawFd.

[l0kod]

The FromRawFd function as described would either require that the implementing type be generic over T or store T in a box.

Well, I think it should be enough to put a 'a lifetime to the FromRawFd implementation. This object could then use an inner RawFd and would not have to be generic.

[l0kod]

Lifetime fix from FromRawFd<'a> to FromRawFd<'b>:
fn from_raw_fd<'a, T>(fd: mut T) -> Self<'a> where T: AsRawFd, T: 'a to
fn from_raw_fd<'a, 'b, T>(fd: mut T) -> Self<'b> where T: AsRawFd, T: 'a, 'a: 'b

[Stebalien]

Lifetimes aren't the issue. Say I want to create a File object but I need fine control over how the file object is created. Currently, I can make a few system calls and then call unsafe { File::from_raw_fd(fd)}. Under your proposal File would have to become File<'a>. Worse, it would be impossible to create a file with a static lifetime from a file descriptor (without using static variables) because 'a is the lifetime of the reference to the AsRawFd object which presumably lives on the stack.

[l0kod]

Under your proposal File would have to become File<'a>.

Yes, that's the price for a safe FromRawFd.

it would be impossible to create a file with a static lifetime from a file descriptor (without using static variables) because 'a is the lifetime of the reference to the AsRawFd object which presumably lives on the stack.

I don't think it's currently possible to create a File with a 'static lifetime anyway, but the proposal doesn't change anything about that. If you want to really transform a FromRawFd object to a File, do not pass an object reference but move/pass the object to from_raw_fd. The new File object will get the lifetime of the AsRawFd object.
If both objects live on the stack, nothing to think/worry about, otherwise the compiler will tell you that the code is unsafe, which is what we want. We can box the AsRawFd object as we already do for similar lifetime issues.

[Stebalien]

The type File has a 'static lifetime.

If you want to really transform a FromRawFd (sic: assuming you meant AsRawFd) object to a File, do not pass an object reference but move/pass the object to from_raw_fd. The new File object will get the lifetime of the AsRawFd object.

Which means the new File would have to contain the AsRawFd which means either File would have to be generic or the AsRawFd struct would have to be boxed. Additionally, this means File would have to have two variants, one that wraps a raw file descriptor, and another that wraps an AsRawFd (in some form).

You're missing the entire point of FromRawFd. It's not for constructing types from other types that implement AsRawFd, it's for constructing types from raw file descriptors as the name indicates:

fn open_custom_file() -> File {
    unsafe {
        let my_fd = libc::open(...);
        File::from_raw_fd(my_fd)
    }
}
fn my_c_api_wrapper() -> File {
    unsafe {
        let fd = my_c_api::something_that_returns_an_fd();
        File::from_raw_fd(fd)
    }
}

In other words, it's designed to be paired with unsafe code.

[l0kod]

The type File has a 'static lifetime.

It depend of the File instance/object, so no, File doesn't automatically have a 'static lifetime.

Which means the new File would have to contain the AsRawFd which means either File would have to be generic or the AsRawFd struct would have to be boxed.

Not necessarily, the from_raw_fd can take an AsRawFd but the Self<'b> only need to keep track of the lifetime and doesn't need to keep the AsRawFd object, only the inner RawFd, so no need for generic.

You're missing the entire point of FromRawFd. It's not for constructing types from other types that implement AsRawFd, it's for constructing types from raw file descriptors as the name indicates

I understand quite well the current status of FromRawFd and AsRawFd but I'm suggesting to tweak it a little to be able to handle file descriptor safely between objects. Rust has the features do it, so we should try to get a safe API for handling file descriptor as well.

In other words, it's designed to be paired with unsafe code.

For now, yes, but I think we could manage to remove the need for the code to be unsafe .

Your example is legitimate, but do not automatically handle the file descriptor closing, which is why it's unsafe. Instead, it rely on the File implementation to take care for closing the FD.

A more generic way to handle file descriptor is to wrap it with a dedicated object like FileDesc/IoHandle<T>/IoDesc/Io (cf. #19169 (comment) like does MIO). Even if it's not in the std it's trivial to implement (only need an AsRawFd and a Drop implementation for auto-closing the FD).

My proposal would allow to continue using file descriptors for FFI, but moreover, to safely temporally pass the file descriptor to another object and get it back without closing it (but remain safe), if needed, which is not possible right now because the FromRawFd objets "have the contract that they are the sole owner of the file descriptor they are wrapping".
To sum up: always one unique file descriptor owner (correctly handling closing, from the beginning to the end) but multiple exclusive file descriptor users (one at a time).
This will help to write safe code for interoperability between I/O libraries (like std and MIO) while being flexible (for future use).

cc @carllerche, @reem

[Stebalien]

It depend of the File instance/object, so no, File doesn't automatically have a 'static lifetime.

No. File as currently defined in the rust source has a static lifetime. &'a File and &'a mut File have non-static lifetimes ('a) but those are different types. Lifetimes are defined at the type level. There are no lifetimes in the type File so File (the type) must have a static lifetime.

Your example is legitimate, but do not automatically handle the file descriptor closing, which is why it's unsafe. Instead, it rely on the File implementation to take care for closing the FD.

1. As stated in the FromRawFd description:

   This function consumes ownership of the specified file descriptor. The returned object will take responsibility for closing it when the object goes out of scope.

2. That's not the only reason it's unsafe. Going back to the motivating example, if MemoryMap were to implement AsRawFd, the following would be unsafe:

let mm: MemoryMap<MyStructs> = MemoryMap::with_capacity(20) // Create a file and allocate 20 MyStructs worth of space.
// Use mm...
{
   let file = File::from_raw_fd(mm)
   // write to the file
}
// Use mm...
// SEGFAULT because writing to mm is equivalent to transmuting `[u8]` to `MyStruct`

It's perfectly reasonable to have MemoryMap implement AsRawFd (for example, you might want to use it for locking, you might need to stat it, etc...) however, it's up to the user of the file descriptor to ensure that they are using it in a memory safe manner.

[l0kod]

As stated in the FromRawFd description: This function consumes ownership of the specified file descriptor. The returned object will take responsibility for closing it when the object goes out of scope.

I known, I quoted it. It's a contract, it's not enforced by the compiler.

This example does not demonstrate the type safety problem because the mm object is moved to a File object. There is an issue if &mm is passed to the File object because mm can be corrupted after that.
So, I agree that passing a reference (like &AsRawFd) can be unsafe but moving an AsRawFd object should be safe because the FromRawFd object handle the "conversion".

The issue in this example could be to force the mm object to not close his file descriptor because the new File object handle it now. I do not see any way than using mem::forget, which is unsafe, again.
I don't think this can scale well to all types (i.e. to bypass the Drop implementation), except if the original owner (the AsRawFd object) live until the end of the FromRawFd object, who then, will not need to take care of the file descriptor closing.

[l0kod]

Another way to make it simple (without explicit lifetime), is to use a minimal (inner) object handling the file descriptor lifetime (e.g. FileDesc/Io).

[Stebalien]

I known, I quoted it. It's a contract, it's not enforced by the compiler.

Neither is "don't close the file descriptor" (the contract you'd need to make your proposal work).

This example does not demonstrate the type safety problem because the mm object is moved to a File object. There is an issue if &mm is passed to the File object because mm can be corrupted after that.
So, I agree that passing a reference (like &AsRawFd) can be unsafe but moving an AsRawFd object should be safe because the FromRawFd object handle the "conversion".

Sorry, I meant &mut mm. Furthermore, you don't even need to explicitly use mm to be memory unsafe, you just need to drop it (calling drop on its now mangled members).

[Stebalien]

Anyways, this discussion should probably go in an RFC.

[alexcrichton]

@l0kod

I think in general there's a bit of a disconnect about where you want the standard library to go with these sorts of primitives and the vision that we've set for for the I/O modules currently. The current design is intentionally conservative in these respects, providing the bare bones primitives necessary to accomplish these sorts of tasks instead of trying to provide a comprehensive solution to interoperating with system primitives.

We expected nicer utilities for dealing with these kinds of applications to grow naturally in the crates.io ecosystem, but not in the standard library. If you'd like to pursue a much more featureful suite of primitives in the standard library I would recommend writing an RFC.