FromRawFd should be unsafe

[Stebalien]

Currently, AFAIK, this is the only way to manipulate a file using a file descriptor directly without writing an unsafe block. If this trait remains safe, the following MemoryMap interface (for example) can't be safe:

struct MemoryMap {
  //...
}
impl MemoryMap {
    fn new(file: File) -> io::Result<MemoryMap> {
        // ...
    }
    // ...
}
fn main() {
    let f = File::create(...);
    let f2 = File::from_raw_fd(f.as_raw_fd());
    let m = MemoryMap::new(f).unwrap();
    drop(f2);
    // Use m and segfault...
}

[aturon]

Great catch. I agree that from_raw_fd should be unsafe, and we should make a clear contract about unsafe code and the ownership expectations for std's IO types.

[sfackler]

One thing we could also possibly do here in addition to making it unsafe is to have it return a Result, which would allow it to check if the file descriptor corresponds to the appropriate socket type. Not sure if the added required complexity on implementations is worth it though.

[alexcrichton]

The current intention for this trait is to be a cheap-as-possible raising operation from a lower-level representation into what the standard library is expecting, so I wouldn't personally expect any checks to be made about the actual value itself (e.g. that's up to the job of the application to work correctly).

That's my personal feeling at least :)

[Stebalien]

Closed by rust-lang/rust#24251

[l0kod]

cc rust-lang/rust#19169 and rust-lang/rust#24251 (comment)

[tomjakubowski]

I'm not sure I quite follow here.

struct MemoryMap {
  //...
}
impl MemoryMap {
    fn new(file: File) -> io::Result<MemoryMap> {
        // ...
    }
    // ...
}
fn main() {
    let f = File::create(...);
    let f2 = File::from_raw_fd(f.as_raw_fd());
    let m = MemoryMap::new(f).unwrap();
    drop(f2);
    // Use m and segfault...
}

Is the problem here supposed to be that dropping f2 will call close() on the descriptor (which also underlies f), thus invalidating m? According to SUSv2, an mmapped region can be used even after the file descriptor passed to mmap has been closed:

The mmap() function adds an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference is removed when there are no more mappings to the file.

edit:

The same applies to Windows file mappings and file handles:

Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order.

[Stebalien]

Good point, a better example would be writing arbitrary data to the file.

[elichai]

Related question. is there a "safe" trait to get a raw descriptor?
i.e. can this somehow be safe? or does write_wrapper must be unsafe?

use libc::*;
use std::os::unix::io::*;

pub fn write_wrapper<F: AsRawFd>(fd: &mut F, msg: &[u8]) -> Result<usize, ()> {
    let res = unsafe { write(fd.as_raw_fd(), msg.as_ptr() as _, msg.len()) };
    if res < 0 {
        Err(())
    } else {
        Ok(res as usize)
    }
}

[sfackler]

as_raw_fd is a safe function.

[elichai]

@sfackler My point is a "safe" way to get a fd, meaning that the implementor of the trait will need to use "unsafe" in order to return a fd, but the whoever calls as_raw_fd can assume the fd is valid(in a no segfaults valid)

look at my example right now you can do:

struct A;
    impl AsRawFd for A {
       fn as_raw_fd(&self) -> RawFd {
           -1
       }
}

write(&mut A, b"ABC");

Which will obviously won't be fine

[Stebalien]

Not in the standard library. However, you can implement this yourself: IntoSafeFd (not as, into) and FromSafeFd.