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

Work around missing libc constants on Fuchsia #184

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/main.yml
Expand Up @@ -57,9 +57,8 @@ jobs:
strategy:
matrix:
# TODO: add the following targets, currently broken.
# "x86_64-fuchsia"
# "x86_64-unknown-redox"
target: ["aarch64-apple-ios", "aarch64-linux-android", "x86_64-apple-darwin", "x86_64-pc-windows-msvc", "x86_64-sun-solaris", "x86_64-unknown-freebsd", "x86_64-unknown-illumos", "x86_64-unknown-linux-gnu", "x86_64-unknown-netbsd"]
target: ["aarch64-apple-ios", "aarch64-linux-android", "x86_64-apple-darwin", "x86_64-fuchsia", "x86_64-pc-windows-msvc", "x86_64-sun-solaris", "x86_64-unknown-freebsd", "x86_64-unknown-illumos", "x86_64-unknown-linux-gnu", "x86_64-unknown-netbsd"]
steps:
- uses: actions/checkout@master
- name: Install Rust
Expand Down
65 changes: 33 additions & 32 deletions src/sys/unix.rs
Expand Up @@ -7,11 +7,6 @@
// except according to those terms.

use std::cmp::min;
#[cfg(all(
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
use std::ffi::{CStr, CString};
#[cfg(not(target_os = "redox"))]
use std::io::IoSlice;
use std::marker::PhantomData;
Expand Down Expand Up @@ -942,11 +937,19 @@ impl crate::Socket {
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
pub fn device(&self) -> io::Result<Option<CString>> {
pub fn device(&self) -> io::Result<Option<Vec<u8>>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change this to Vec<u8>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the interface name is not guaranteed to be nul terminated if its length is IF_NAMSIZ.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a fair point, but couldn't we use IFNAMSIZ + 1 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but why would you want to? It's easiest to work with a vector than with a CString.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can call CString::into_bytes to get a vector. The main benefit of CString/CStr is that they guarantee C-string compatibility, where Vec<u8> doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, but why is that a benefit? Why do we want ffi types in the public API? What will the caller do with this return value that requires a C string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

b"hel\0lo" is a perfectly valid vector of bytes, however as C string is likely invalid because it stops after "hel". The CString type ensure the caller knows the string can't contain null bytes. This is mainly useful in set_device, so the caller doesn't pass half names of devices. For consistency it's nicer if device also returns C string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the perspective of this API (setsockopt SO_BINDTODEVICE), b"hel\0lo" is a perfectly valid name. There's nothing in the API documentation that indicates that the nul byte is interpreted at all, since the caller always supplies the length. I haven't been able to create a local device with a name containing a nul byte, but I don't see any reason that such a thing would be illegal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking through the Linux source I think you're right. There is only a special case for "\0", which it considers an empty string, same as passing a zero length. I'll see to review #186.

use std::convert::TryInto as _;

// TODO(https://github.com/rust-lang/libc/pull/2010): Remove this.
#[cfg(target_os = "fuchsia")]
const IFNAMSIZ: libc::size_t = 16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update libc (after rust-lang/libc#2013 is merged) and use those constants.

Socket2 used to be filled with defined constants from libc, I don't want to go back to that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pr to update libc: rust-lang/libc#2014.


#[cfg(not(target_os = "fuchsia"))]
const IFNAMSIZ: libc::size_t = libc::IFNAMSIZ;

// TODO: replace with `MaybeUninit::uninit_array` once stable.
let mut buf: [MaybeUninit<u8>; libc::IFNAMSIZ] =
unsafe { MaybeUninit::<[MaybeUninit<u8>; libc::IFNAMSIZ]>::uninit().assume_init() };
let mut len = buf.len() as libc::socklen_t;
let mut buf: MaybeUninit<[u8; IFNAMSIZ]> = MaybeUninit::uninit();
let mut len = IFNAMSIZ.try_into().unwrap();
unsafe {
syscall!(getsockopt(
self.inner,
Expand All @@ -959,21 +962,13 @@ impl crate::Socket {
if len == 0 {
Ok(None)
} else {
// Allocate a buffer for `CString` with the length including the
// null terminator.
let len = len as usize;
let mut name = Vec::with_capacity(len);

// TODO: use `MaybeUninit::slice_assume_init_ref` once stable.
// Safety: `len` bytes are writen by the OS, this includes a null
// terminator. However we don't copy the null terminator because
// `CString::from_vec_unchecked` adds its own null terminator.
let buf = unsafe { slice::from_raw_parts(buf.as_ptr().cast(), len - 1) };
name.extend_from_slice(buf);

// Safety: the OS initialised the string for us, which shouldn't
// include any null bytes.
Ok(Some(unsafe { CString::from_vec_unchecked(name) }))
Ok(Some(
unsafe { buf.assume_init() }[..len.try_into().unwrap()].into(),
))
}
}

Expand All @@ -990,9 +985,9 @@ impl crate::Socket {
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
pub fn bind_device(&self, interface: Option<&CStr>) -> io::Result<()> {
pub fn bind_device(&self, interface: Option<&[u8]>) -> io::Result<()> {
let (value, len) = if let Some(interface) = interface {
(interface.as_ptr(), interface.to_bytes_with_nul().len())
(interface.as_ptr(), interface.len())
} else {
(ptr::null(), 0)
};
Expand Down Expand Up @@ -1058,9 +1053,15 @@ impl crate::Socket {
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
pub fn freebind(&self) -> io::Result<bool> {
// TODO(https://github.com/rust-lang/libc/pull/2011): Remove this.
#[cfg(target_os = "fuchsia")]
const IP_FREEBIND: c_int = 15;

#[cfg(not(target_os = "fuchsia"))]
const IP_FREEBIND: c_int = libc::IP_FREEBIND;

unsafe {
getsockopt::<c_int>(self.inner, libc::SOL_SOCKET, libc::IP_FREEBIND)
.map(|reuse| reuse != 0)
getsockopt::<c_int>(self.inner, libc::SOL_SOCKET, IP_FREEBIND).map(|reuse| reuse != 0)
}
}

Expand All @@ -1078,14 +1079,14 @@ impl crate::Socket {
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
pub fn set_freebind(&self, reuse: bool) -> io::Result<()> {
unsafe {
setsockopt(
self.inner,
libc::SOL_SOCKET,
libc::IP_FREEBIND,
reuse as c_int,
)
}
// TODO(https://github.com/rust-lang/libc/pull/2011): Remove this.
#[cfg(target_os = "fuchsia")]
const IP_FREEBIND: c_int = 15;

#[cfg(not(target_os = "fuchsia"))]
const IP_FREEBIND: c_int = libc::IP_FREEBIND;

unsafe { setsockopt(self.inner, libc::SOL_SOCKET, IP_FREEBIND, reuse as c_int) }
}
}

Expand Down
6 changes: 2 additions & 4 deletions tests/socket.rs
@@ -1,5 +1,3 @@
#[cfg(all(feature = "all", any(target_os = "fuchsia", target_os = "linux")))]
use std::ffi::CStr;
#[cfg(any(windows, target_vendor = "apple"))]
use std::io;
#[cfg(not(target_os = "redox"))]
Expand Down Expand Up @@ -633,13 +631,13 @@ fn tcp_keepalive() {
#[test]
fn device() {
// Some common network interface on Linux.
const INTERFACES: &[&str] = &["lo\0", "lo0\0", "eth0\0", "wlan0\0"];
const INTERFACES: &[&str] = &["lo", "lo0", "eth0", "wlan0"];

let socket = Socket::new(Domain::IPV4, Type::STREAM, None).unwrap();
assert_eq!(socket.device().unwrap(), None);

for interface in INTERFACES.iter() {
let interface = CStr::from_bytes_with_nul(interface.as_bytes()).unwrap();
let interface = interface.as_bytes();
if let Err(err) = socket.bind_device(Some(interface)) {
// Network interface is not available try another.
if matches!(err.raw_os_error(), Some(libc::ENODEV) | Some(libc::EPERM)) {
Expand Down