libs: add platform-specific APIs to extract file descriptors, SOCKETs, HANDLEs, etc from `std::io` #19169

Merged
merged 3 commits into from Nov 26, 2014

Conversation

Projects
None yet
8 participants
@aturon
Member

aturon commented Nov 21, 2014

This PR adds some internal infrastructure to allow the private std::sys module to access internal representation details of std::io.

It then exposes those details in two new, platform-specific API surfaces: std::os::unix and std::os::windows.

To start with, these will provide the ability to extract file descriptors, HANDLEs, SOCKETs, and so on from std::io types.

More functionality, and more specific platforms (e.g. std::os::linux) will be added over time.

Closes #18897

@aturon

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Nov 21, 2014

cc @cmr
cc #19116

@aturon

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Nov 21, 2014

@cmr

This comment has been minimized.

Show comment
Hide comment
@cmr

cmr Nov 21, 2014

Member

cc @xales

Looks good to me!

Member

cmr commented Nov 21, 2014

cc @xales

Looks good to me!

@aturon

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Nov 21, 2014

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 21, 2014

Contributor

Great!

As discuss in #15643, it would be interesting to expose a less raw object like FileDesc instead of directly exposing fd_t-like. This way, it's possible to use a more safe and abstract FileDesc wrapper to control some resources (e.g. keep the automatic file descriptor closing feature).

Additionally, a safer FileDesc could have this properties:

  • a safe read-only object with this available functions: fstat, fd, handle, tell
  • a writable object with this available functions: read, write, seek, dataseek, fsync, truncate
  • unsafe functions: new, unwrap
  • add a safe Clone trait using dup-like (e.g. libc::fcntl(self.fd, libc::F_DUPFD_CLOEXEC))
Contributor

l0kod commented Nov 21, 2014

Great!

As discuss in #15643, it would be interesting to expose a less raw object like FileDesc instead of directly exposing fd_t-like. This way, it's possible to use a more safe and abstract FileDesc wrapper to control some resources (e.g. keep the automatic file descriptor closing feature).

Additionally, a safer FileDesc could have this properties:

  • a safe read-only object with this available functions: fstat, fd, handle, tell
  • a writable object with this available functions: read, write, seek, dataseek, fsync, truncate
  • unsafe functions: new, unwrap
  • add a safe Clone trait using dup-like (e.g. libc::fcntl(self.fd, libc::F_DUPFD_CLOEXEC))
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 21, 2014

Member

@l0kod Definitely! I'm quite excited about these abstractions developing over time.

That said, I wanted to start conservatively by exposing the raw object and letting the community experiment with abstractions in external crates like the ones you describe, before we commit to anything in libstd.

Member

aturon commented Nov 21, 2014

@l0kod Definitely! I'm quite excited about these abstractions developing over time.

That said, I wanted to start conservatively by exposing the raw object and letting the community experiment with abstractions in external crates like the ones you describe, before we commit to anything in libstd.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 21, 2014

Contributor

FileDesc-like interface will also discourage the direct/only use of lifetime-less (and inherently unsafe) fd_t for FFI wrappers.

Contributor

l0kod commented Nov 21, 2014

FileDesc-like interface will also discourage the direct/only use of lifetime-less (and inherently unsafe) fd_t for FFI wrappers.

src/libstd/sys/windows/ext.rs
+/// Windows-specific extensions to `std::io::net::pipe::UnixListener`.
+pub trait UnixListenerExt {
+ /// Extract the raw handle, without taking any ownership.
+ fn as_raw_fd(&self) -> Handle;

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

s/fd/handle

@alexcrichton

alexcrichton Nov 21, 2014

Member

s/fd/handle

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

(a few below as well)

@alexcrichton

alexcrichton Nov 21, 2014

Member

(a few below as well)

src/libstd/sys/windows/ext.rs
+/// Windows-specific extensions to `std::io::net::tcp::TcpStream`.
+pub trait TcpStreamExt {
+ /// Extract the raw socket, without taking any ownership.
+ fn as_raw_fd(&self) -> Socket;

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

s/fd/socket/ ?

@alexcrichton

alexcrichton Nov 21, 2014

Member

s/fd/socket/ ?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 21, 2014

Member

This is an interesting design with respect to an extension-trait-per-object rather than an extension trait per piece of functionality, but I agree that use std::os::unix::prelude::* is pretty reasonable for getting around any ergonomic concern, and this improves discoverability as you know exactly what trait to look at to see what fancy methods are available.

Over time I think we'll need to figure out how to deal with the rustdoc problem to show windows extension traits as well as unix ones, but we can deal with that later. Overall I like the direction this is going in for providing platform-specific functionality.

cc @brson, I'm curious as to your opinion with this organization as well.

Member

alexcrichton commented Nov 21, 2014

This is an interesting design with respect to an extension-trait-per-object rather than an extension trait per piece of functionality, but I agree that use std::os::unix::prelude::* is pretty reasonable for getting around any ergonomic concern, and this improves discoverability as you know exactly what trait to look at to see what fancy methods are available.

Over time I think we'll need to figure out how to deal with the rustdoc problem to show windows extension traits as well as unix ones, but we can deal with that later. Overall I like the direction this is going in for providing platform-specific functionality.

cc @brson, I'm curious as to your opinion with this organization as well.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 21, 2014

Member

@alexcrichton

This is an interesting design with respect to an extension-trait-per-object rather than an extension trait per piece of functionality

So, my original thought there was that these extension traits would diverge over time, and I think in the long run we probably will want type-specific extensions like this.

That said, thinking more about the functionality being proposed here, there's actually a good use case for exposing this via more generic traits, so that the os::unix etc. API can eventually work generically over them. I will revise.

Member

aturon commented Nov 21, 2014

@alexcrichton

This is an interesting design with respect to an extension-trait-per-object rather than an extension trait per piece of functionality

So, my original thought there was that these extension traits would diverge over time, and I think in the long run we probably will want type-specific extensions like this.

That said, thinking more about the functionality being proposed here, there's actually a good use case for exposing this via more generic traits, so that the os::unix etc. API can eventually work generically over them. I will revise.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 21, 2014

Member

Updated with improved design.

Member

aturon commented Nov 21, 2014

Updated with improved design.

aturon added some commits Nov 21, 2014

sys: reveal std::io representation to sys module
This commit adds a `AsInner` trait to `sys_common` and provides
implementations on many `std::io` types. This is a building block for
exposing platform-specific APIs that hook into `std::io` types.
libs: add std::os::unix module
The new `std::os::unix` module exposes several extension traits
for extracting file descriptors from `std::io` types.
src/libstd/sys/windows/ext.rs
+ fn as_raw_fd(&self) -> Fd {
+ self.as_fd().fd()
+ }
+}

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

I wonder if we could mark these as "super extra experimental" because I'd love to reimplement File on top of just a HANDLE instead of relying on msvcrt for file descriptors... (fine for now of course)

@alexcrichton

alexcrichton Nov 21, 2014

Member

I wonder if we could mark these as "super extra experimental" because I'd love to reimplement File on top of just a HANDLE instead of relying on msvcrt for file descriptors... (fine for now of course)

src/libstd/sys/windows/ext.rs
+ fn as_raw_handle(&self) -> Fd {
+ self.as_fd().handle()
+ }
+}

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

Not sure this should be impl'd for File and PipeStream

@alexcrichton

alexcrichton Nov 21, 2014

Member

Not sure this should be impl'd for File and PipeStream

This comment has been minimized.

@aturon

aturon Nov 21, 2014

Member

I'm asking for some help from Windows experts on this one.

@aturon

aturon Nov 21, 2014

Member

I'm asking for some help from Windows experts on this one.

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

Whoa sorry I'm off my rocker here, these are fine.

@alexcrichton

alexcrichton Nov 21, 2014

Member

Whoa sorry I'm off my rocker here, these are fine.

This comment has been minimized.

@alexcrichton

alexcrichton Nov 21, 2014

Member

but I think the return type should be Handle, not Fd

@alexcrichton

alexcrichton Nov 21, 2014

Member

but I think the return type should be Handle, not Fd

This comment has been minimized.

@aturon

aturon Nov 21, 2014

Member

This is why type aliases are a bad idea 😨

@aturon

aturon Nov 21, 2014

Member

This is why type aliases are a bad idea 😨

libs: add std::os::windows module
The new `std::os::windows` module exposes several extension traits
for extracting file descriptors, sockets, and handles from `std::io`
types.
@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

I like this design as well, but I hope we will regain the FileDesc-like safety soon instead of the raw Fd, Handle and Socket.

Moreover, having different function names for unix and windows seems to make OS-independent code more cumbersome to write. Not sure if it's a good idea but, what if we had an OS-independent file descriptor type? Maybe something like IoHandle<T>, with T implemented for Fd, Handle and Socket? This trait could manage the file descriptor lifetime with Drop, a safe duplication with Clone and expose the raw value with Deref<T>.

Replacing as_raw_fd(&self) -> Fd with as_iohandle(&self) -> &IoHandle<T> would allow safe and OS-independent APIs. This should also replace fd(&self) and handle(&self).
Handling mutability safely is still a problem.

Contributor

l0kod commented Nov 22, 2014

I like this design as well, but I hope we will regain the FileDesc-like safety soon instead of the raw Fd, Handle and Socket.

Moreover, having different function names for unix and windows seems to make OS-independent code more cumbersome to write. Not sure if it's a good idea but, what if we had an OS-independent file descriptor type? Maybe something like IoHandle<T>, with T implemented for Fd, Handle and Socket? This trait could manage the file descriptor lifetime with Drop, a safe duplication with Clone and expose the raw value with Deref<T>.

Replacing as_raw_fd(&self) -> Fd with as_iohandle(&self) -> &IoHandle<T> would allow safe and OS-independent APIs. This should also replace fd(&self) and handle(&self).
Handling mutability safely is still a problem.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

Deref<Mutex<T>> for IoHandle<T> should solve the mutability problem :)

Contributor

l0kod commented Nov 22, 2014

Deref<Mutex<T>> for IoHandle<T> should solve the mutability problem :)

@mahkoh

This comment has been minimized.

Show comment
Hide comment
@mahkoh

mahkoh Nov 22, 2014

Contributor

There is no mutability safety problem.

Contributor

mahkoh commented Nov 22, 2014

There is no mutability safety problem.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

There is no mutability safety problem.

Look at #15643 for background information. Basically it could be unsafe to write or even read from a file descriptor because of concurrent access or file descriptor wrappers state.

Contributor

l0kod commented Nov 22, 2014

There is no mutability safety problem.

Look at #15643 for background information. Basically it could be unsafe to write or even read from a file descriptor because of concurrent access or file descriptor wrappers state.

@mahkoh

This comment has been minimized.

Show comment
Hide comment
@mahkoh

mahkoh Nov 22, 2014

Contributor

Basically it could be unsafe to write or even read from a file descriptor because of concurrent access or file descriptor wrappers state.

There is nothing unsafe about this. See http://doc.rust-lang.org/reference.html#behavior-considered-undefined

Contributor

mahkoh commented Nov 22, 2014

Basically it could be unsafe to write or even read from a file descriptor because of concurrent access or file descriptor wrappers state.

There is nothing unsafe about this. See http://doc.rust-lang.org/reference.html#behavior-considered-undefined

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

This is low level FFI unsafe behavior. Again, read #15643 for more details.

Contributor

l0kod commented Nov 22, 2014

This is low level FFI unsafe behavior. Again, read #15643 for more details.

@mahkoh

This comment has been minimized.

Show comment
Hide comment
@mahkoh

mahkoh Nov 22, 2014

Contributor

This is low level FFI unsafe behavior.

You should first define what this even means.

Contributor

mahkoh commented Nov 22, 2014

This is low level FFI unsafe behavior.

You should first define what this even means.

@pythonesque

This comment has been minimized.

Show comment
Hide comment
@pythonesque

pythonesque Nov 22, 2014

Contributor

@l0kod I don't think this is unsafe. Can you give me an example of a situation where sharing a file descriptor could cause memory unsafety? AFAIK, the kernel already synchronizes updates, refcounts, etc. In the mmap case, trying to access truncated memory will result in a SIGBUS, you can't cause memory unsafety that way. If as I suspect, there is no way to cause memory unsafety this way, then it is not in the scope of things that should be protected with an unsafe block. If someone wants to write a higher level library that makes additional guarantees, that's fine, but Rust is under no obligation to protect that library.

Contributor

pythonesque commented Nov 22, 2014

@l0kod I don't think this is unsafe. Can you give me an example of a situation where sharing a file descriptor could cause memory unsafety? AFAIK, the kernel already synchronizes updates, refcounts, etc. In the mmap case, trying to access truncated memory will result in a SIGBUS, you can't cause memory unsafety that way. If as I suspect, there is no way to cause memory unsafety this way, then it is not in the scope of things that should be protected with an unsafe block. If someone wants to write a higher level library that makes additional guarantees, that's fine, but Rust is under no obligation to protect that library.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

One main goal of Rust is safety and safety is not only about memory but about behavior as well, especially for file descriptor (e.g. Drop trait goal). Take a look at my first comment or other, comments, describing, the, unsafe, behaviors.

Please read the entire #15643 thread before other questions/comments.

Contributor

l0kod commented Nov 22, 2014

One main goal of Rust is safety and safety is not only about memory but about behavior as well, especially for file descriptor (e.g. Drop trait goal). Take a look at my first comment or other, comments, describing, the, unsafe, behaviors.

Please read the entire #15643 thread before other questions/comments.

@pythonesque

This comment has been minimized.

Show comment
Hide comment
@pythonesque

pythonesque Nov 22, 2014

Contributor

Sorry, I don't buy that.

First off, while safety in general may be a desirable goal, Rust's "unsafe" feature is very much only about memory safety. It is not for things other than memory safety. For example, deadlock is a much more common problem than close-then-open, and everyone agrees that code that can deadlock is incorrect, but Rust does not require that all code that can potentially deadlock be marked as unsafe, nor does it require libraries to protect against deadlock. Same with leaking memory--it's entirely legal in Rust. You're encouraged to avoid it, but it is not unsafe. It is not considered unsafe to perform an overlong bit shift, or cause a stack overflow, or have races where they don't impact memory safety (it's easy to screw up by overusing Relaxed atomics). What is different about file descriptors that warrants the use of unsafe?

Secondly, saying a function might be written to rely on an assumption for memory safety, so don't violate that assumption in safe code, doesn't make sense. Unless you make the assumption explicitly part of the definition of unsafe in the document above, other code can expose the file descriptors safely, and then your code will still not work. This is not dogmatism, this is fundamentally the whole reason unsafe works--that all code is required to enforce the invariants. Functions cannot rely on file descriptors not being used by other threads, etc., for memory safety, unless that is explicitly stated in the definition of unsafe. The unsafe traits system is not here yet, but is designed to help provide additional invariants, maybe you can figure out a way to do something with that (but I doubt it).

Thirdly, none of the behaviors you listed (close, then open, execve) are memory unsafe. So I have no idea why this matters. If you want protection from this kind of abuse, you have to do it through the kernel anyway.

I read all your comments. None of them explain why this is memory unsafe or why your proposal will help make other code safe. Please stop recommending this, it doesn't make sense.

Contributor

pythonesque commented Nov 22, 2014

Sorry, I don't buy that.

First off, while safety in general may be a desirable goal, Rust's "unsafe" feature is very much only about memory safety. It is not for things other than memory safety. For example, deadlock is a much more common problem than close-then-open, and everyone agrees that code that can deadlock is incorrect, but Rust does not require that all code that can potentially deadlock be marked as unsafe, nor does it require libraries to protect against deadlock. Same with leaking memory--it's entirely legal in Rust. You're encouraged to avoid it, but it is not unsafe. It is not considered unsafe to perform an overlong bit shift, or cause a stack overflow, or have races where they don't impact memory safety (it's easy to screw up by overusing Relaxed atomics). What is different about file descriptors that warrants the use of unsafe?

Secondly, saying a function might be written to rely on an assumption for memory safety, so don't violate that assumption in safe code, doesn't make sense. Unless you make the assumption explicitly part of the definition of unsafe in the document above, other code can expose the file descriptors safely, and then your code will still not work. This is not dogmatism, this is fundamentally the whole reason unsafe works--that all code is required to enforce the invariants. Functions cannot rely on file descriptors not being used by other threads, etc., for memory safety, unless that is explicitly stated in the definition of unsafe. The unsafe traits system is not here yet, but is designed to help provide additional invariants, maybe you can figure out a way to do something with that (but I doubt it).

Thirdly, none of the behaviors you listed (close, then open, execve) are memory unsafe. So I have no idea why this matters. If you want protection from this kind of abuse, you have to do it through the kernel anyway.

I read all your comments. None of them explain why this is memory unsafe or why your proposal will help make other code safe. Please stop recommending this, it doesn't make sense.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

Again, as discussed in #15643, it's not (directly) about memory safety but about safe file descriptor handling. Some examples:

  • file descriptor leak (can quickly fulfill the file descriptor pool per process)
  • Rust should provide features to cleanly manage file descriptor already opened at execution as well
  • concurrent inconsistent write(2), e.g. multiple tasks write at the same time in the same FD
  • concurrent lseek(2), e.g. with read(2), which lead to corrupt data
  • file descriptor closing, e.g. store raw FD1, close FD1, write to FD1 (which is close)
  • file descriptor replacing, e.g. store raw FD1, close FD1, open a new file (FD2), write to FD1 (which is now FD2)
  • some file descriptor (e.g. open /proc/self/mem, mmap or memfd) can be a representation of the process memory, which would then impact the memory safety
  • keep in mind that FFI can use file descriptor and manage them in your back (safe wrapper could prevent bugs)

Using a raw file descriptor is unsafe. This errors can be security bugs as well. We can prevent most of them with a good file descriptor interface for functions using them (e.g. FFI). FileDesc already have some safety with Drop.

Contributor

l0kod commented Nov 22, 2014

Again, as discussed in #15643, it's not (directly) about memory safety but about safe file descriptor handling. Some examples:

  • file descriptor leak (can quickly fulfill the file descriptor pool per process)
  • Rust should provide features to cleanly manage file descriptor already opened at execution as well
  • concurrent inconsistent write(2), e.g. multiple tasks write at the same time in the same FD
  • concurrent lseek(2), e.g. with read(2), which lead to corrupt data
  • file descriptor closing, e.g. store raw FD1, close FD1, write to FD1 (which is close)
  • file descriptor replacing, e.g. store raw FD1, close FD1, open a new file (FD2), write to FD1 (which is now FD2)
  • some file descriptor (e.g. open /proc/self/mem, mmap or memfd) can be a representation of the process memory, which would then impact the memory safety
  • keep in mind that FFI can use file descriptor and manage them in your back (safe wrapper could prevent bugs)

Using a raw file descriptor is unsafe. This errors can be security bugs as well. We can prevent most of them with a good file descriptor interface for functions using them (e.g. FFI). FileDesc already have some safety with Drop.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

Here are some sketchy thoughts:

struct IoHandle<T> {
    handle: Mutex<T>,
}

impl IoHandle<T> {
    pub unsafe fn new(handle: T) {
        IoHandle{ handle: Mutex::new(handle) }
    }

    // Use select! with the file descriptor
    pub fn event(&self) -> Receiver { … }
}

impl Deref<Mutex<Fd>> for IoHandle<Fd> {
    fn deref(&self) -> &Mutex<Fd> {
        &self.handle
    }
}

impl Drop for IoHandle<Fd> {
    fn drop(&mut self) {
        let _ = unsafe { libc::close(*self.lock()) };
    }
}

impl Clone for IoHandle<Fd> {
    fn clone(&self) -> IoHandle<Fd> {
        let fd = unsafe { libc::fcntl(*self.lock(), libc::F_DUPFD_CLOEXEC) };
        if fd == -1 {
            panic!(last_error());
        }
        unsafe { IoHandle::new(fd) }
    }
}

trait AsIoHandle<T, W> {
    fn as_iohandle(&self) -> &IoHandle<T>;
    fn from_iohandle(IoHandle<T>) -> W;
}

impl File {
    fn path(&'a self) -> Option<&'a Path> { … }
    fn open_at(&self, path: &Path) -> IoResult<File> { … }
}

impl AsIoHandle for File {
    #[cfg(windows)]
    fn from_iohandle(&self, IoHandle<Handle>) -> IoResult<File> { … }
    #[cfg(unix)]
    fn from_iohandle(&self, IoHandle<Fd>) -> IoResult<File> { … }

    #[cfg(windows)]
    fn as_iohandle(&self) -> &IoHandle<Handle> { … }
    #[cfg(unix)]
    fn as_iohandle(&self) -> &IoHandle<Fd> { … }
}
Contributor

l0kod commented Nov 22, 2014

Here are some sketchy thoughts:

struct IoHandle<T> {
    handle: Mutex<T>,
}

impl IoHandle<T> {
    pub unsafe fn new(handle: T) {
        IoHandle{ handle: Mutex::new(handle) }
    }

    // Use select! with the file descriptor
    pub fn event(&self) -> Receiver { … }
}

impl Deref<Mutex<Fd>> for IoHandle<Fd> {
    fn deref(&self) -> &Mutex<Fd> {
        &self.handle
    }
}

impl Drop for IoHandle<Fd> {
    fn drop(&mut self) {
        let _ = unsafe { libc::close(*self.lock()) };
    }
}

impl Clone for IoHandle<Fd> {
    fn clone(&self) -> IoHandle<Fd> {
        let fd = unsafe { libc::fcntl(*self.lock(), libc::F_DUPFD_CLOEXEC) };
        if fd == -1 {
            panic!(last_error());
        }
        unsafe { IoHandle::new(fd) }
    }
}

trait AsIoHandle<T, W> {
    fn as_iohandle(&self) -> &IoHandle<T>;
    fn from_iohandle(IoHandle<T>) -> W;
}

impl File {
    fn path(&'a self) -> Option<&'a Path> { … }
    fn open_at(&self, path: &Path) -> IoResult<File> { … }
}

impl AsIoHandle for File {
    #[cfg(windows)]
    fn from_iohandle(&self, IoHandle<Handle>) -> IoResult<File> { … }
    #[cfg(unix)]
    fn from_iohandle(&self, IoHandle<Fd>) -> IoResult<File> { … }

    #[cfg(windows)]
    fn as_iohandle(&self) -> &IoHandle<Handle> { … }
    #[cfg(unix)]
    fn as_iohandle(&self) -> &IoHandle<Fd> { … }
}
@pythonesque

This comment has been minimized.

Show comment
Hide comment
@pythonesque

pythonesque Nov 22, 2014

Contributor

Please reread my previous comment. It doesn't matter whether these things are unsafe by some other definition of safety, Rust's unsafe is only about memory safety and furthermore cannot work for anything other than memory safety. If you want to make raw file descriptors to be unsafe in general, please file an RFC to change the definition of unsafe.

Contributor

pythonesque commented Nov 22, 2014

Please reread my previous comment. It doesn't matter whether these things are unsafe by some other definition of safety, Rust's unsafe is only about memory safety and furthermore cannot work for anything other than memory safety. If you want to make raw file descriptors to be unsafe in general, please file an RFC to change the definition of unsafe.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 22, 2014

Member

@l0kod

Replacing as_raw_fd(&self) -> Fd with as_iohandle(&self) -> &IoHandle<T> would allow safe and OS-independent APIs.

I may be missing something, but I don't think this would really change things: the T would still vary by platform.

Fundamentally, these os::platform modules are intended for platform-specific APIs, and should be made as idiomatic for each platform as possible. Platform-independent APIs, on the other hand, can live in std::io.

@l0kod @mahkoh @pythonesque

I think there are a lot of interesting questions about what kind of safer/higher-level, but platform-specific abstractions we can provide here. But as I said above, the intent of this PR is just to expose the lowest-level hooks into std::io so that the broader Cargo ecosystem can explore abstractions on top of them; I'd rather that experimentation happen externally for now (that's the general migration path into std in any case).

It'd be good if we can turn the conversation back to issues about this PR in particular, and handle questions about abstractions on top elsewhere.

Member

aturon commented Nov 22, 2014

@l0kod

Replacing as_raw_fd(&self) -> Fd with as_iohandle(&self) -> &IoHandle<T> would allow safe and OS-independent APIs.

I may be missing something, but I don't think this would really change things: the T would still vary by platform.

Fundamentally, these os::platform modules are intended for platform-specific APIs, and should be made as idiomatic for each platform as possible. Platform-independent APIs, on the other hand, can live in std::io.

@l0kod @mahkoh @pythonesque

I think there are a lot of interesting questions about what kind of safer/higher-level, but platform-specific abstractions we can provide here. But as I said above, the intent of this PR is just to expose the lowest-level hooks into std::io so that the broader Cargo ecosystem can explore abstractions on top of them; I'd rather that experimentation happen externally for now (that's the general migration path into std in any case).

It'd be good if we can turn the conversation back to issues about this PR in particular, and handle questions about abstractions on top elsewhere.

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 22, 2014

Contributor

I may be missing something, but I don't think this would really change things: the T would still vary by platform.

You're right about the specific T by platform but the IoHandle<T> add a generic wrapper interface common to all platform that should help to add more (safety) features to this type (or trait) in the future (e.g. Drop implementation). In my example, the as_iohandle(), from_iohandle() or event() could benefit from this.
The return by reference is important for a (future) clean ownership too.

Contributor

l0kod commented Nov 22, 2014

I may be missing something, but I don't think this would really change things: the T would still vary by platform.

You're right about the specific T by platform but the IoHandle<T> add a generic wrapper interface common to all platform that should help to add more (safety) features to this type (or trait) in the future (e.g. Drop implementation). In my example, the as_iohandle(), from_iohandle() or event() could benefit from this.
The return by reference is important for a (future) clean ownership too.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 26, 2014

Member

ping @alexcrichton -- I still feel that we should land this initial, unopinonated version that just exposes the raw types, and then bring in better abstractions after some external experimentation.

Member

aturon commented Nov 26, 2014

ping @alexcrichton -- I still feel that we should land this initial, unopinonated version that just exposes the raw types, and then bring in better abstractions after some external experimentation.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment

@bors: retry

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

saw approval from alexcrichton
at aturon@1e66164

Contributor

bors commented on 1e66164 Nov 26, 2014

saw approval from alexcrichton
at aturon@1e66164

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

merging aturon/rust/fds = 1e66164 into auto

Contributor

bors replied Nov 26, 2014

merging aturon/rust/fds = 1e66164 into auto

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

aturon/rust/fds = 1e66164 merged ok, testing candidate = d55e2e6

Contributor

bors replied Nov 26, 2014

aturon/rust/fds = 1e66164 merged ok, testing candidate = d55e2e6

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

saw approval from alexcrichton
at aturon@1e66164

Contributor

bors replied Nov 26, 2014

saw approval from alexcrichton
at aturon@1e66164

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

merging aturon/rust/fds = 1e66164 into auto

Contributor

bors replied Nov 26, 2014

merging aturon/rust/fds = 1e66164 into auto

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

aturon/rust/fds = 1e66164 merged ok, testing candidate = 61af402

Contributor

bors replied Nov 26, 2014

aturon/rust/fds = 1e66164 merged ok, testing candidate = 61af402

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2014

Contributor

fast-forwarding master to auto = 61af402

Contributor

bors replied Nov 26, 2014

fast-forwarding master to auto = 61af402

bors added a commit that referenced this pull request Nov 26, 2014

auto merge of #19169 : aturon/rust/fds, r=alexcrichton
This PR adds some internal infrastructure to allow the private `std::sys` module to access internal representation details of `std::io`.

It then exposes those details in two new, platform-specific API surfaces: `std::os::unix` and `std::os::windows`.

To start with, these will provide the ability to extract file descriptors, HANDLEs, SOCKETs, and so on from `std::io` types.

More functionality, and more specific platforms (e.g. `std::os::linux`) will be added over time.

Closes #18897

bors added a commit that referenced this pull request Nov 26, 2014

auto merge of #19169 : aturon/rust/fds, r=alexcrichton
This PR adds some internal infrastructure to allow the private `std::sys` module to access internal representation details of `std::io`.

It then exposes those details in two new, platform-specific API surfaces: `std::os::unix` and `std::os::windows`.

To start with, these will provide the ability to extract file descriptors, HANDLEs, SOCKETs, and so on from `std::io` types.

More functionality, and more specific platforms (e.g. `std::os::linux`) will be added over time.

Closes #18897

@bors bors closed this Nov 26, 2014

@bors bors merged commit 1e66164 into rust-lang:master Nov 26, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default all tests passed
@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Nov 26, 2014

Contributor

@aturon, could it be possible to expose the FileDesc struct alone (temporary) to be able to use it?

Contributor

l0kod commented Nov 26, 2014

@aturon, could it be possible to expose the FileDesc struct alone (temporary) to be able to use it?

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Dec 14, 2014

Contributor

The MIO library from @carllerche use something similar to FileDesc with an IoDesc for auto-closing.

Contributor

l0kod commented Dec 14, 2014

The MIO library from @carllerche use something similar to FileDesc with an IoDesc for auto-closing.

@l0kod l0kod referenced this pull request in rust-lang/rfcs Dec 14, 2014

Merged

RFC: io and os reform: initial skeleton #517

@l0kod

This comment has been minimized.

Show comment
Hide comment
@l0kod

l0kod Jan 10, 2015

Contributor

To conclude, using Fd could be safe as long as the AsRawFd (implementation) is not dropped before the end of the Fd use (i.e. function taking AsRawFd) except if the AsRawFd implementation need a consistent/known inner Fd state…

Contributor

l0kod commented Jan 10, 2015

To conclude, using Fd could be safe as long as the AsRawFd (implementation) is not dropped before the end of the Fd use (i.e. function taking AsRawFd) except if the AsRawFd implementation need a consistent/known inner Fd state…

@l0kod

This comment has been minimized.

Show comment
Hide comment
Contributor

l0kod commented Apr 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment