From 8da6b5a3f22a4bcf691270eacd2fb8be3a09cc2d Mon Sep 17 00:00:00 2001 From: Sander Maijers <3374183+sanmai-NL@users.noreply.github.com> Date: Mon, 17 Apr 2023 20:44:56 +0200 Subject: [PATCH 1/9] Differentiate `timex`: `gnu` & `musl` `target_env` --- ntp-os-clock/src/unix.rs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/ntp-os-clock/src/unix.rs b/ntp-os-clock/src/unix.rs index c0959f72c..385cb281c 100644 --- a/ntp-os-clock/src/unix.rs +++ b/ntp-os-clock/src/unix.rs @@ -15,7 +15,7 @@ use ntp_proto::{NtpClock, NtpDuration, NtpLeapIndicator, NtpTimestamp, PollInter // Libc has no good other way of obtaining this, so let's at least make our functions // more readable. -#[cfg(target_os = "linux")] +#[cfg(all(target_os = "linux", target_env = "gnu"))] pub(crate) const EMPTY_TIMEX: libc::timex = libc::timex { modes: 0, offset: 0, @@ -53,6 +53,34 @@ pub(crate) const EMPTY_TIMEX: libc::timex = libc::timex { __unused11: 0, }; +#[cfg(all(target_os = "linux", target_env = "musl"))] +pub(crate) const EMPTY_TIMEX: libc::timex = libc::timex { + modes: 0, + offset: 0, + freq: 0, + maxerror: 0, + esterror: 0, + status: 0, + constant: 0, + precision: 0, + tolerance: 0, + time: libc::timeval { + tv_sec: 0, + tv_usec: 0, + }, + tick: 0, + ppsfreq: 0, + jitter: 0, + shift: 0, + stabil: 0, + jitcnt: 0, + calcnt: 0, + errcnt: 0, + stbcnt: 0, + tai: 0, + __padding: [0; 11] +}; + #[cfg(any(target_os = "freebsd", target_os = "macos"))] pub(crate) const EMPTY_TIMEX: libc::timex = libc::timex { modes: 0, From 0dc46a891b020b455184935813a7e9200b0c26e6 Mon Sep 17 00:00:00 2001 From: Sander Maijers <3374183+sanmai-NL@users.noreply.github.com> Date: Mon, 17 Apr 2023 21:15:41 +0200 Subject: [PATCH 2/9] Only use `ntp_adjtime()` where supported --- ntp-os-clock/src/unix.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ntp-os-clock/src/unix.rs b/ntp-os-clock/src/unix.rs index 385cb281c..a87bda464 100644 --- a/ntp-os-clock/src/unix.rs +++ b/ntp-os-clock/src/unix.rs @@ -213,7 +213,11 @@ impl UnixNtpClock { // information in the return value of ntp_adjtime can be ignored. // The ntp_adjtime call is safe because the reference always // points to a valid libc::timex. - if unsafe { libc::ntp_adjtime(timex) } == -1 { + #[cfg(any(target_os = "freebsd", target_os = "macos", target_env = "gnu"))] + let errno = unsafe { libc::ntp_adjtime(timex) }; + #[cfg(all(target_os = "linux", target_env = "musl"))] + let errno = unsafe { libc::adjtimex(timex) }; + if errno == -1 { Err(convert_errno()) } else { Ok(()) From dff9ad5a36dba5d579a6f47b0122dc923d974121 Mon Sep 17 00:00:00 2001 From: Sander Maijers <3374183+sanmai-NL@users.noreply.github.com> Date: Mon, 17 Apr 2023 23:47:54 +0200 Subject: [PATCH 3/9] Deal with ioctl requests being signed or unsigned With musl and POSIX, they're signed. With glibc, they're unsigned. --- ntp-udp/src/hwtimestamp.rs | 4 ++-- ntp-udp/src/raw_socket.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ntp-udp/src/hwtimestamp.rs b/ntp-udp/src/hwtimestamp.rs index 1daa82827..be5f870e9 100644 --- a/ntp-udp/src/hwtimestamp.rs +++ b/ntp-udp/src/hwtimestamp.rs @@ -28,7 +28,7 @@ fn set_hardware_timestamp( }; let fd = udp_socket.as_raw_fd(); - cerr(unsafe { libc::ioctl(fd, libc::SIOCSHWTSTAMP as libc::c_ulong, &mut ifreq) })?; + cerr(unsafe { libc::ioctl(fd, libc::SIOCSHWTSTAMP as _, &mut ifreq) })?; Ok(()) } @@ -51,7 +51,7 @@ fn get_hardware_timestamp( }; let fd = udp_socket.as_raw_fd(); - cerr(unsafe { libc::ioctl(fd, libc::SIOCGHWTSTAMP as libc::c_ulong, &mut ifreq) })?; + cerr(unsafe { libc::ioctl(fd, libc::SIOCGHWTSTAMP as _, &mut ifreq) })?; Ok(tstamp_config) } diff --git a/ntp-udp/src/raw_socket.rs b/ntp-udp/src/raw_socket.rs index 08a810928..f8cf0779c 100644 --- a/ntp-udp/src/raw_socket.rs +++ b/ntp-udp/src/raw_socket.rs @@ -439,8 +439,8 @@ pub(crate) mod timestamping_config { }, }; - const SIOCETHTOOL: u64 = 0x8946; - cerr(unsafe { libc::ioctl(fd, SIOCETHTOOL as libc::c_ulong, &ifr) }).unwrap(); + // SIOCETHTOOL = 0x8946 (Ethtool interface) Linux ioctl request + cerr(unsafe { libc::ioctl(fd, 0x8946, &ifr) }).unwrap(); let support = EnableTimestamps { rx_software: tsi.so_timestamping & libc::SOF_TIMESTAMPING_RX_SOFTWARE != 0, From 427d82f6ee3535ab74d89427b35a8ecacb9ce8c0 Mon Sep 17 00:00:00 2001 From: Sander Maijers <3374183+sanmai-NL@users.noreply.github.com> Date: Mon, 17 Apr 2023 23:48:49 +0200 Subject: [PATCH 4/9] Init private struct fields without constructor --- ntp-udp/src/raw_socket.rs | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/ntp-udp/src/raw_socket.rs b/ntp-udp/src/raw_socket.rs index f8cf0779c..8d48579c3 100644 --- a/ntp-udp/src/raw_socket.rs +++ b/ntp-udp/src/raw_socket.rs @@ -163,7 +163,7 @@ mod set_timestamping_options { } mod recv_message { - use std::{io::IoSliceMut, marker::PhantomData, net::SocketAddr, os::unix::prelude::AsRawFd}; + use std::{io::IoSliceMut, marker::PhantomData, mem::MaybeUninit, net::SocketAddr, os::unix::prelude::AsRawFd, ptr::addr_of_mut}; use tracing::warn; @@ -190,14 +190,31 @@ mod recv_message { let mut buf_slice = IoSliceMut::new(packet_buf); let mut addr = zeroed_sockaddr_storage(); - let mut mhdr = libc::msghdr { - msg_control: control_buf.as_mut_ptr().cast::(), - msg_controllen: control_buf.len() as _, - msg_iov: (&mut buf_slice as *mut IoSliceMut).cast::(), - msg_iovlen: 1, - msg_flags: 0, - msg_name: (&mut addr as *mut libc::sockaddr_storage).cast::(), - msg_namelen: std::mem::size_of::() as u32, + let msg_iov = (&mut buf_slice as *mut IoSliceMut).cast::(); + let msg_name = (&mut addr as *mut libc::sockaddr_storage).cast::(); + let msg_control = control_buf.as_mut_ptr().cast::(); + let mut mhdr = { + // Safety: + // To initialize private padding fields (present on `target_env = "musl"`), we must use unsafe. + let mut mhdr = MaybeUninit::::zeroed(); + let msg_controllen = control_buf.len() as _; + let ptr_msg_control = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_control) }; + unsafe {ptr_msg_control.write(msg_control); } + let ptr_msg_controllen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_controllen) }; + unsafe { ptr_msg_controllen.write(msg_controllen); } + let ptr_msg_iov = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_iov) }; + unsafe { ptr_msg_iov.write(msg_iov); } + let ptr_msg_iovlen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_iovlen) }; + unsafe { ptr_msg_iovlen.write(1); } + let ptr_msg_flags = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_flags) }; + unsafe { ptr_msg_flags.write(0); } + let ptr_msg_name = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_name) }; + unsafe { ptr_msg_name.write(msg_name); } + let ptr_msg_namelen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_namelen) }; + unsafe { + ptr_msg_namelen.write(std::mem::size_of::() as u32); + mhdr.assume_init() + } }; let receive_flags = match queue { From 0ffbba62959dc58635bfa2d7fad6a53a227383c6 Mon Sep 17 00:00:00 2001 From: Sander Maijers <3374183+sanmai-NL@users.noreply.github.com> Date: Wed, 19 Apr 2023 11:42:41 +0200 Subject: [PATCH 5/9] Include musl target in Github Actions workflow --- .github/workflows/build.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 6648507f1..deb14f6b6 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -21,6 +21,9 @@ jobs: - stable - beta - 1.65.0 + target: + - "" + - x86_64-unknown-linux-musl os: [ubuntu-latest] features: - "" From 945772dff7f7ffc4cc04d0ba1a5b1f4f94cc9300 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 24 Apr 2023 10:16:29 +0200 Subject: [PATCH 6/9] clarify why padding fields are private; make code less verbose --- ntp-udp/src/raw_socket.rs | 51 ++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/ntp-udp/src/raw_socket.rs b/ntp-udp/src/raw_socket.rs index 8d48579c3..6ca2a4a90 100644 --- a/ntp-udp/src/raw_socket.rs +++ b/ntp-udp/src/raw_socket.rs @@ -163,7 +163,10 @@ mod set_timestamping_options { } mod recv_message { - use std::{io::IoSliceMut, marker::PhantomData, mem::MaybeUninit, net::SocketAddr, os::unix::prelude::AsRawFd, ptr::addr_of_mut}; + use std::{ + io::IoSliceMut, marker::PhantomData, mem::MaybeUninit, net::SocketAddr, + os::unix::prelude::AsRawFd, + }; use tracing::warn; @@ -177,6 +180,17 @@ mod recv_message { Error, } + fn empty_msghdr() -> libc::msghdr { + // On `target_env = "musl"`, there are several private padding fields. + // the position of these padding fields depends on the system endianness, + // so keeping making them public does not really help. + // + // Safety: + // + // all fields are either integer or pointer types. For those types, 0 is a valid value + unsafe { MaybeUninit::::zeroed().assume_init() } + } + pub(crate) fn receive_message<'a>( socket: &std::net::UdpSocket, packet_buf: &mut [u8], @@ -190,32 +204,15 @@ mod recv_message { let mut buf_slice = IoSliceMut::new(packet_buf); let mut addr = zeroed_sockaddr_storage(); - let msg_iov = (&mut buf_slice as *mut IoSliceMut).cast::(); - let msg_name = (&mut addr as *mut libc::sockaddr_storage).cast::(); - let msg_control = control_buf.as_mut_ptr().cast::(); - let mut mhdr = { - // Safety: - // To initialize private padding fields (present on `target_env = "musl"`), we must use unsafe. - let mut mhdr = MaybeUninit::::zeroed(); - let msg_controllen = control_buf.len() as _; - let ptr_msg_control = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_control) }; - unsafe {ptr_msg_control.write(msg_control); } - let ptr_msg_controllen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_controllen) }; - unsafe { ptr_msg_controllen.write(msg_controllen); } - let ptr_msg_iov = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_iov) }; - unsafe { ptr_msg_iov.write(msg_iov); } - let ptr_msg_iovlen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_iovlen) }; - unsafe { ptr_msg_iovlen.write(1); } - let ptr_msg_flags = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_flags) }; - unsafe { ptr_msg_flags.write(0); } - let ptr_msg_name = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_name) }; - unsafe { ptr_msg_name.write(msg_name); } - let ptr_msg_namelen = unsafe { addr_of_mut!((*mhdr.as_mut_ptr()).msg_namelen) }; - unsafe { - ptr_msg_namelen.write(std::mem::size_of::() as u32); - mhdr.assume_init() - } - }; + let mut mhdr = empty_msghdr(); + + mhdr.msg_control = control_buf.as_mut_ptr().cast::(); + mhdr.msg_controllen = control_buf.len() as _; + mhdr.msg_iov = (&mut buf_slice as *mut IoSliceMut).cast::(); + mhdr.msg_iovlen = 1; + mhdr.msg_flags = 0; + mhdr.msg_name = (&mut addr as *mut libc::sockaddr_storage).cast::(); + mhdr.msg_namelen = std::mem::size_of::() as u32; let receive_flags = match queue { MessageQueue::Normal => 0, From 288e8ab5afd8c6f7eb7353c5c1af063c34ec9955 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 24 Apr 2023 10:33:37 +0200 Subject: [PATCH 7/9] fix warnings on libc::time_t for musl --- ntp-os-clock/src/unix.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/ntp-os-clock/src/unix.rs b/ntp-os-clock/src/unix.rs index a87bda464..d611b5837 100644 --- a/ntp-os-clock/src/unix.rs +++ b/ntp-os-clock/src/unix.rs @@ -78,7 +78,7 @@ pub(crate) const EMPTY_TIMEX: libc::timex = libc::timex { errcnt: 0, stbcnt: 0, tai: 0, - __padding: [0; 11] + __padding: [0; 11], }; #[cfg(any(target_os = "freebsd", target_os = "macos"))] @@ -209,15 +209,20 @@ impl UnixNtpClock { } fn ntp_adjtime(timex: &mut libc::timex) -> Result<(), Error> { + #[cfg(any(target_os = "freebsd", target_os = "macos", target_env = "gnu"))] + use libc::ntp_adjtime as adjtime; + + // ntp_adjtime is equivalent to adjtimex for our purposes + // + // https://man7.org/linux/man-pages/man2/adjtimex.2.html + #[cfg(all(target_os = "linux", target_env = "musl"))] + use libc::adjtimex as adjtime; + // We don't care about the time status, so the non-error // information in the return value of ntp_adjtime can be ignored. // The ntp_adjtime call is safe because the reference always // points to a valid libc::timex. - #[cfg(any(target_os = "freebsd", target_os = "macos", target_env = "gnu"))] - let errno = unsafe { libc::ntp_adjtime(timex) }; - #[cfg(all(target_os = "linux", target_env = "musl"))] - let errno = unsafe { libc::adjtimex(timex) }; - if errno == -1 { + if unsafe { adjtime(timex) } == -1 { Err(convert_errno()) } else { Ok(()) @@ -238,7 +243,15 @@ impl UnixNtpClock { let mut timespec = self.clock_gettime()?; - timespec.tv_sec += offset_secs as libc::time_t; + timespec.tv_sec += { + // this looks a little strange. it is to work around the `libc::time_t` type being + // deprectated for musl in rust's libc. + if true { + offset_secs as _ + } else { + timespec.tv_sec + } + }; timespec.tv_nsec += offset_nanos as libc::c_long; self.clock_settime(timespec)?; @@ -253,7 +266,7 @@ impl UnixNtpClock { let mut timex = libc::timex { modes: libc::ADJ_SETOFFSET | libc::MOD_NANO, time: libc::timeval { - tv_sec: secs as libc::time_t, + tv_sec: secs as _, tv_usec: nanos as libc::suseconds_t, }, ..crate::unix::EMPTY_TIMEX From a8b85b59599d381da80f3aeff9c92e621b1cfa28 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 24 Apr 2023 11:04:18 +0200 Subject: [PATCH 8/9] more debug info in assert --- ntp-daemon/src/spawn/standard.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ntp-daemon/src/spawn/standard.rs b/ntp-daemon/src/spawn/standard.rs index 3d3cb01d0..874ee0625 100644 --- a/ntp-daemon/src/spawn/standard.rs +++ b/ntp-daemon/src/spawn/standard.rs @@ -273,7 +273,10 @@ mod tests { || addr2 != orig_addr || addr3 != orig_addr || addr4 != orig_addr - || addr5 != orig_addr + || addr5 != orig_addr, + "{:?} should not be in {:?}", + orig_addr, + [addr1, addr2, addr3, addr4, addr5], ); } From f941252cdda17ebd563e016256e38b2069733b05 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 24 Apr 2023 11:05:19 +0200 Subject: [PATCH 9/9] add separate CI actions for musl --- .github/workflows/build.yaml | 68 +++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index deb14f6b6..ac1962904 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -23,7 +23,6 @@ jobs: - 1.65.0 target: - "" - - x86_64-unknown-linux-musl os: [ubuntu-latest] features: - "" @@ -55,6 +54,39 @@ jobs: files: lcov.info fail_ci_if_error: false + build-musl: + name: Build+test-musl + runs-on: ${{ matrix.os }} + strategy: + matrix: + rust: + - stable + - beta + - 1.65.0 + target: + - "x86_64-unknown-linux-musl" + os: [ubuntu-latest] + features: + - "" + - "--features sentry" + - "--features rfc-algorithm" + steps: + - name: Checkout sources + uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab + with: + persist-credentials: false + - name: Install ${{ matrix.rust }} toolchain + uses: actions-rs/toolchain@16499b5e05bf2e26879000db0c1d13f7e13fa3af + with: + toolchain: ${{ matrix.rust }} + override: true + - name: cargo build + run: cargo build ${{ matrix.features }} + - name: cargo test + run: cargo test + env: + RUST_BACKTRACE: 1 + unused: name: Unused dependencies runs-on: ubuntu-latest @@ -188,6 +220,40 @@ jobs: command: clippy args: --target armv7-unknown-linux-gnueabihf --workspace --all-targets -- -D warnings + clippy-musl: + name: ClippyMusl + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab + with: + persist-credentials: false + - name: Install rust toolchain + uses: actions-rs/toolchain@16499b5e05bf2e26879000db0c1d13f7e13fa3af + with: + toolchain: stable + override: true + default: true + components: clippy + target: x86_64-unknown-linux-musl + # Use zig as our C compiler for convenient cross-compilation. We run into rustls having a dependency on `ring`. + # This crate uses C and assembly code, and because of its build scripts, `cargo clippy` needs to be able to compile + # that code for our target. + - uses: goto-bus-stop/setup-zig@869a4299cf8ac7db4ebffaec36ad82a682f88acb + with: + version: 0.9.0 + - name: Install cargo-zigbuild + uses: taiki-e/install-action@f0b89cda51dc27169e64a0dbc20182a1ec84d8ff + with: + tool: cargo-zigbuild + - name: Run clippy + uses: actions-rs/cargo@844f36862e911db73fe0815f00a4a2602c279505 + env: + TARGET_CC: "/home/runner/.cargo/bin/cargo-zigbuild zig cc -- -target x86_64-linux-musl" + with: + command: clippy + args: --target x86_64-unknown-linux-musl --workspace --all-targets -- -D warnings + fuzz: name: Smoke-test fuzzing targets runs-on: ubuntu-20.04