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

Fix Broken SockFlag::SOCK_NONBLOCK and SockFlag::SOCK_CLOEXEC on MacOS/iOS #863

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
66 changes: 40 additions & 26 deletions src/sys/socket/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,32 +86,44 @@ pub enum SockProtocol {
KextControl = libc::SYSPROTO_CONTROL,
}

libc_bitflags!{
/// Additional socket options
pub struct SockFlag: c_int {
/// Set non-blocking mode on the new socket
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"))]
SOCK_NONBLOCK;
/// Set close-on-exec on the new descriptor
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"))]
SOCK_CLOEXEC;
/// Return `EPIPE` instead of raising `SIGPIPE`
#[cfg(target_os = "netbsd")]
SOCK_NOSIGPIPE;
/// For domains `AF_INET(6)`, only allow `connect(2)`, `sendto(2)`, or `sendmsg(2)`
/// to the DNS port (typically 53)
#[cfg(target_os = "openbsd")]
SOCK_DNS;
cfg_if! {
if #[cfg(any(target_os = "macos",
Copy link
Author

@kristate kristate Feb 22, 2018

Choose a reason for hiding this comment

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

Note here: nix::sys::socket::socket fn API requires the linux specific SockFlag::SOCK_NONBLOCK and SockFlag::SOCK_CLOEXEC which is not included in macOS' libc -- the work around is to provide a bitflag that mimics such support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this be fixed in libc? Do these constants not exist on OS X and iOS?

Copy link
Author

Choose a reason for hiding this comment

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

This is fundamentally an implementation concern.

It's because the nix::sys::socket::socket fn is not only assigning a socket, but also setting flags on the socket in the same function to match parity with Linux (which is an improvement). The normal API is defined in socket(2) as:

int socket(int domain, int type, int protocol);

linux diverts from POSIX and BSD by supplying a SOCK_NONBLOCK flag as is defined in linux man socket(2) as:

SOCK_NONBLOCK
Set the O_NONBLOCK file status flag on the new open
file description.  Using this flag saves extra calls
to fcntl(2) to achieve the same result.

SOCK_CLOEXEC
Set the close-on-exec (FD_CLOEXEC) flag on the new
file descriptor.  See the description of the
O_CLOEXEC flag in open(2) for reasons why this may be
useful.

In summery:

  • SOCK_NONBLOCK and SOCK_CLOEXEC are in linux since 2.6.27
  • Some BSDs have conformed to this new API and such have a define in libc.
  • macOS is the exception and sticks to directly calling fcntl(2)
  • nix takes the high road and also only calls fcntl(2) with a new fourth parameter on the function call

Because nix provides a new API that does not conform to POSIX or BSD (or directly Linux), this patch creates virtual definitions for macOS and iOS that are backed by their equivalent fcntl flags.

I think this is the best course of action for two reasons:

  1. Redefining the function at this stage would break compatibility
  2. Defining virtual definitions for SOCK_NONBLOCK and SOCK_CLOEXEC are only used inside of nix and only for the socket fn.

Copy link
Author

Choose a reason for hiding this comment

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

For the sake of conversation, the alternative is:

Removing pub struct SockFlag and having the API user call fcntl directly.

Thus, these function defs need to be changed (breaking downstream code):

pub fn socket<T: Into<Option<SockProtocol>>>(domain: AddressFamily, ty: SockType, flags: SockFlag, protocol: T) -> Result<RawFd> {
/// Create a pair of connected sockets
///
/// [Further reading](http://pubs.opengroup.org/onlinepubs/9699919799/functions/socketpair.html)
pub fn socketpair<T: Into<Option<SockProtocol>>>(domain: AddressFamily, ty: SockType, protocol: T, flags: SockFlag) -> Result<(RawFd, RawFd)> {
/// Accept a connection on a socket
///
/// [Further reading](http://man7.org/linux/man-pages/man2/accept.2.html)
pub fn accept4(sockfd: RawFd, flags: SockFlag) -> Result<RawFd> {
fn accept4_polyfill(sockfd: RawFd, flags: SockFlag) -> Result<RawFd> {

target_os = "ios"))] {
bitflags! {
pub struct SockFlag: c_int {
const SOCK_NONBLOCK = libc::O_NONBLOCK;
const SOCK_CLOEXEC = libc::O_CLOEXEC;
}
}
} else {
libc_bitflags!{
/// Additional socket options
pub struct SockFlag: c_int {
/// Set non-blocking mode on the new socket
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"))]
SOCK_NONBLOCK;
/// Set close-on-exec on the new descriptor
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"))]
SOCK_CLOEXEC;
/// Return `EPIPE` instead of raising `SIGPIPE`
#[cfg(target_os = "netbsd")]
SOCK_NOSIGPIPE;
/// For domains `AF_INET(6)`, only allow `connect(2)`, `sendto(2)`, or `sendmsg(2)`
/// to the DNS port (typically 53)
#[cfg(target_os = "openbsd")]
SOCK_DNS;
}
}
}
}

Expand Down Expand Up @@ -709,7 +721,9 @@ pub fn socket<T: Into<Option<SockProtocol>>>(domain: AddressFamily, ty: SockType
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "ios",
target_os = "linux",
target_os = "macos",
target_os = "netbsd",
target_os = "openbsd"))]
{
Expand Down
58 changes: 58 additions & 0 deletions test/sys/test_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,61 @@ pub fn test_syscontrol() {
// requires root privileges
// connect(fd, &sockaddr).expect("connect failed");
}

/// Test non-blocking mode on new sockets via SockFlag::O_NONBLOCK
#[cfg(any(target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "ios",
target_os = "linux",
target_os = "macos",
target_os = "netbsd",
target_os = "openbsd"))]
#[test]
pub fn test_sockflag_nonblock() {
use libc;
use nix::fcntl::{fcntl};
use nix::fcntl::FcntlArg::{F_GETFL};
use nix::sys::socket::{socket, AddressFamily, SockType, SockFlag};

/* first, try without SockFlag::SOCK_NONBLOCK */
let sock = socket(AddressFamily::Unix, SockType::Stream, SockFlag::empty(), None)
.expect("socket failed");

let fcntl_res = fcntl(sock, F_GETFL).expect("fcntl failed");

assert!(fcntl_res & libc::O_NONBLOCK == 0);

/* next, try with SockFlag::SOCK_NONBLOCK */
let sock = socket(AddressFamily::Unix, SockType::Stream, SockFlag::SOCK_NONBLOCK, None)
.expect("socket failed");

let fcntl_res = fcntl(sock, F_GETFL).expect("fcntl failed");

assert!(fcntl_res & libc::O_NONBLOCK == libc::O_NONBLOCK);
}

/// Test close-on-exec on new sockets via SockFlag::SOCK_CLOEXEC
#[test]
pub fn test_sockflag_cloexec() {
use libc;
use nix::fcntl::{fcntl};
use nix::fcntl::FcntlArg::{F_GETFD};
use nix::sys::socket::{socket, AddressFamily, SockType, SockFlag};

/* first, test without SockFlag::SOCK_CLOEXEC */
let sock = socket(AddressFamily::Unix, SockType::Stream, SockFlag::empty(), None)
.expect("socket failed");

let fcntl_res = fcntl(sock, F_GETFD).expect("fcntl failed");

assert!(fcntl_res & libc::FD_CLOEXEC == 0);

/* next, test without SockFlag::SOCK_CLOEXEC */
let sock = socket(AddressFamily::Unix, SockType::Stream, SockFlag::SOCK_CLOEXEC, None)
.expect("socket failed");

let fcntl_res = fcntl(sock, F_GETFD).expect("fcntl failed");

assert!(fcntl_res & libc::FD_CLOEXEC == libc::FD_CLOEXEC);
}