Skip to content
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

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

Merged
merged 2 commits into from
Aug 14, 2022

Conversation

Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw commented Aug 13, 2022

I've merged the commits from #324 into this one as is was missing the implementations for SockRef, the most important bit.

This implements:

  • The AsFd and AsSocket traits for Socket.
  • From and From for Socket.
  • From for OwnedFd and OwnedSocket.

Replacing AsRawFd with AsFd and AsRawSocket with AsSocket for SockRef.

Closed #218.

notgull and others added 2 commits August 13, 2022 15:27
This implements:
 * The AsFd and AsSocket traits for Socket.
 * From<OwnedFd> and From<OwnedSocket> for Socket.
 * From<Socket> for OwnedFd and OwnedSocket.

Co-authored-by: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Replacing AsRawFd with AsFd and AsRawSocket with AsSocket.
Copy link
Contributor

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Looks good to me! A more sound SockRef will be a boon to the ecosystem as a whole.

@Thomasdezeeuw
Copy link
Collaborator Author

Looks good to me! A more sound SockRef will be a boon to the ecosystem as a whole.

That (#218) certainly got me interested in the I/O safety traits.

@Thomasdezeeuw Thomasdezeeuw requested a review from Darksonn August 13, 2022 13:37
Comment on lines +97 to 103
/// 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,
{
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.

Comment on lines +2033 to +2034
// SAFETY: sock.into_raw() always returns a valid fd.
unsafe { OwnedFd::from_raw_fd(sock.into_raw()) }
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.

@Thomasdezeeuw
Copy link
Collaborator Author

@Darksonn you OK with the changes then?

Copy link
Collaborator

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants