Skip to content

Implement the I/O-safety traits for Socket and SockRef #325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 13 additions & 37 deletions src/sockref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use std::marker::PhantomData;
use std::mem::ManuallyDrop;
use std::ops::Deref;
#[cfg(unix)]
use std::os::unix::io::{AsRawFd, FromRawFd};
use std::os::unix::io::{AsFd, AsRawFd, FromRawFd};
#[cfg(windows)]
use std::os::windows::io::{AsRawSocket, FromRawSocket};
use std::os::windows::io::{AsRawSocket, AsSocket, FromRawSocket};

use crate::Socket;

Expand All @@ -15,14 +15,13 @@ use crate::Socket;
/// This allows for example a [`TcpStream`], found in the standard library, to
/// be configured using all the additional methods found in the [`Socket`] API.
///
/// `SockRef` can be created from any socket type that implements [`AsRawFd`]
/// (Unix) or [`AsRawSocket`] (Windows) using the [`From`] implementation, but
/// the caller must ensure the file descriptor/socket is a valid.
/// `SockRef` can be created from any socket type that implements [`AsFd`]
/// (Unix) or [`AsSocket`] (Windows) using the [`From`] implementation.
///
/// [`TcpStream`]: std::net::TcpStream
// Don't use intra-doc links because they won't build on every platform.
/// [`AsRawFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsRawFd.html
/// [`AsRawSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsRawSocket.html
/// [`AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html
/// [`AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html
///
/// # Examples
///
Expand Down Expand Up @@ -59,29 +58,6 @@ use crate::Socket;
/// # Ok(())
/// # }
/// ```
///
/// Below is an example of **incorrect usage** of `SockRef::from`, which is
/// currently possible (but not intended and will be fixed in future versions).
///
/// ```compile_fail
/// use socket2::SockRef;
///
/// # fn main() -> Result<(), Box<dyn std::error::Error>> {
/// /// THIS USAGE IS NOT VALID!
/// let socket_ref = SockRef::from(&123);
/// // The above line is overseen possibility when using `SockRef::from`, it
/// // uses the `RawFd` (on Unix), which is a type alias for `c_int`/`i32`,
/// // which implements `AsRawFd`. However it may be clear that this usage is
/// // invalid as it doesn't guarantee that `123` is a valid file descriptor.
///
/// // Using `Socket::set_nodelay` now will call it on a file descriptor we
/// // don't own! We don't even not if the file descriptor is valid or a socket.
/// socket_ref.set_nodelay(true)?;
/// drop(socket_ref);
/// # Ok(())
/// # }
/// # DO_NOT_COMPILE
/// ```
pub struct SockRef<'s> {
/// Because this is a reference we don't own the `Socket`, however `Socket`
/// closes itself when dropped, so we use `ManuallyDrop` to prevent it from
Expand All @@ -100,16 +76,16 @@ impl<'s> Deref for SockRef<'s> {
}
}

/// On Windows, a corresponding `From<&impl AsRawSocket>` implementation exists.
/// On Windows, a corresponding `From<&impl AsSocket>` implementation exists.
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
impl<'s, S> From<&'s S> for SockRef<'s>
where
S: AsRawFd,
S: AsFd,
{
/// The caller must ensure `S` is actually a socket.
fn from(socket: &'s S) -> Self {
let fd = socket.as_raw_fd();
let fd = socket.as_fd().as_raw_fd();
assert!(fd >= 0);
SockRef {
socket: ManuallyDrop::new(unsafe { Socket::from_raw_fd(fd) }),
Expand All @@ -118,16 +94,16 @@ where
}
}

/// On Unix, a corresponding `From<&impl AsRawFd>` implementation exists.
/// On Unix, a corresponding `From<&impl AsFd>` implementation exists.
#[cfg(windows)]
#[cfg_attr(docsrs, doc(cfg(windows)))]
impl<'s, S> From<&'s S> for SockRef<'s>
where
S: AsRawSocket,
S: AsSocket,
{
Comment on lines +97 to 103
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be careful with this kind of blanket impl. This will prevent you from implementing From<&T> for any other type T even if T does not implement AsSocket. For example, the previous impl for S: AsRawSocket would prevent you from adding the S: AsSocket impl without the breaking change you're currently going through.

In particular, this prevents you from having From<&T> for any type that is AsRawSocket but isn't AsSocket.

The impl might still be a good idea — as long as you've thought about whether this is acceptable, then that sounds good to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two justifications:

  • The T: AsRawSocket bound is unsound. Replacing it with T: AsSocket makes it sound. It is okay to make breading changes if they are to fix unsoundness
  • The bound mirrors the From<OwnedSocket> bound on Socket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be careful with this kind of blanket impl. This will prevent you from implementing From<&T> for any other type T even if T does not implement AsSocket. For example, the previous impl for S: AsRawSocket would prevent you from adding the S: AsSocket impl without the breaking change you're currently going through.

In particular, this prevents you from having From<&T> for any type that is AsRawSocket but isn't AsSocket.

I know, that's why I removed the original implementation.

The impl might still be a good idea — as long as you've thought about whether this is acceptable, then that sounds good to me.

The concern above is acceptable because this is the only sound way to create SockRef, AsRaw/AsSocket was exactly designed for this reason. SockRef is essentially a type BorrowedFd similar to what TcpStream is to a OwnedFd.

/// See the `From<&impl AsRawFd>` implementation.
/// See the `From<&impl AsFd>` implementation.
fn from(socket: &'s S) -> Self {
let socket = socket.as_raw_socket();
let socket = socket.as_socket().as_raw_socket();
assert!(socket != windows_sys::Win32::Networking::WinSock::INVALID_SOCKET as _);
SockRef {
socket: ManuallyDrop::new(unsafe { Socket::from_raw_socket(socket) }),
Expand Down
26 changes: 25 additions & 1 deletion src/sys/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use std::os::unix::ffi::OsStrExt;
)
))]
use std::os::unix::io::RawFd;
use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd};
#[cfg(feature = "all")]
use std::os::unix::net::{UnixDatagram, UnixListener, UnixStream};
#[cfg(feature = "all")]
Expand Down Expand Up @@ -2012,20 +2012,44 @@ impl crate::Socket {
}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl AsFd for crate::Socket {
fn as_fd(&self) -> BorrowedFd<'_> {
// SAFETY: lifetime is bound by self.
unsafe { BorrowedFd::borrow_raw(self.as_raw()) }
}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl AsRawFd for crate::Socket {
fn as_raw_fd(&self) -> c_int {
self.as_raw()
}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl From<crate::Socket> for OwnedFd {
fn from(sock: crate::Socket) -> OwnedFd {
// SAFETY: sock.into_raw() always returns a valid fd.
unsafe { OwnedFd::from_raw_fd(sock.into_raw()) }
Comment on lines +2033 to +2034
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently your Socket::from_raw constructor does not guarantee this as it is still not marked unsafe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be ameliorated by adding an unsafe bound on from_raw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently your Socket::from_raw constructor does not guarantee this as it is still not marked unsafe.

As per the comment on the function:

socket2/src/socket.rs

Lines 80 to 85 in 057f6c2

/// # Safety
///
/// The caller must ensure `raw` is a valid file descriptor/socket. NOTE:
/// this should really be marked `unsafe`, but this being an internal
/// function, often passed as mapping function, it's makes it very
/// inconvenient to mark it as `unsafe`.

It should be marked as unsafe, but then it can be passed (easily) as a closure any more. So it should be an unsafe function, it's only not because it's an internal function.

But to be clear crate::Socket always holds a valid fd/socket, with the assumption that the OS will not return an invalid fd/socket (outside of errors of course). I also place to switch to std::os::unix::io::OwnedFd to take advantage of the layout optimisation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't actually notice that it was an internal function.

}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl IntoRawFd for crate::Socket {
fn into_raw_fd(self) -> c_int {
self.into_raw()
}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl From<OwnedFd> for crate::Socket {
fn from(fd: OwnedFd) -> crate::Socket {
// SAFETY: `OwnedFd` ensures the fd is valid.
unsafe { crate::Socket::from_raw_fd(fd.into_raw_fd()) }
}
}

#[cfg_attr(docsrs, doc(cfg(unix)))]
impl FromRawFd for crate::Socket {
unsafe fn from_raw_fd(fd: c_int) -> crate::Socket {
Expand Down
31 changes: 30 additions & 1 deletion src/sys/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use std::io::{self, IoSlice};
use std::marker::PhantomData;
use std::mem::{self, size_of, MaybeUninit};
use std::net::{self, Ipv4Addr, Ipv6Addr, Shutdown};
use std::os::windows::io::{AsRawSocket, FromRawSocket, IntoRawSocket, RawSocket};
use std::os::windows::io::{
AsRawSocket, AsSocket, BorrowedSocket, FromRawSocket, IntoRawSocket, OwnedSocket, RawSocket,
};
use std::sync::Once;
use std::time::{Duration, Instant};
use std::{process, ptr, slice};
Expand Down Expand Up @@ -800,18 +802,45 @@ impl crate::Socket {
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl AsSocket for crate::Socket {
fn as_socket(&self) -> BorrowedSocket<'_> {
// SAFETY: lifetime is bound by self.
unsafe { BorrowedSocket::borrow_raw(self.as_raw() as RawSocket) }
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl AsRawSocket for crate::Socket {
fn as_raw_socket(&self) -> RawSocket {
self.as_raw() as RawSocket
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl From<crate::Socket> for OwnedSocket {
fn from(sock: crate::Socket) -> OwnedSocket {
// SAFETY: sock.into_raw() always returns a valid fd.
unsafe { OwnedSocket::from_raw_socket(sock.into_raw() as RawSocket) }
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl IntoRawSocket for crate::Socket {
fn into_raw_socket(self) -> RawSocket {
self.into_raw() as RawSocket
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl From<OwnedSocket> for crate::Socket {
fn from(fd: OwnedSocket) -> crate::Socket {
// SAFETY: `OwnedFd` ensures the fd is valid.
unsafe { crate::Socket::from_raw_socket(fd.into_raw_socket()) }
}
}

#[cfg_attr(docsrs, doc(cfg(windows)))]
impl FromRawSocket for crate::Socket {
unsafe fn from_raw_socket(socket: RawSocket) -> crate::Socket {
crate::Socket::from_raw(socket as Socket)
Expand Down