From 5cd564c7561f01382f8158b1afdf091fb5c97060 Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Mon, 12 Apr 2021 20:03:24 +0200 Subject: [PATCH 1/6] Remove unsound offset_of macro And replace it with constants that define the offsets to the fields. It's not a pretty solution, but it's one without UB. --- src/sys/windows/named_pipe.rs | 66 ++++++++++++++++++++--------------- tests/regressions.rs | 3 +- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/sys/windows/named_pipe.rs b/src/sys/windows/named_pipe.rs index 5f643596b..49064317b 100644 --- a/src/sys/windows/named_pipe.rs +++ b/src/sys/windows/named_pipe.rs @@ -1,41 +1,22 @@ -use crate::event::Source; -use crate::sys::windows::{Event, Overlapped}; -use crate::{poll, Registry}; -use winapi::um::minwinbase::OVERLAPPED_ENTRY; - use std::ffi::OsStr; -use std::fmt; use std::io::{self, Read, Write}; -use std::mem; +use std::mem::{self, size_of}; use std::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, RawHandle}; -use std::slice; use std::sync::atomic::Ordering::{Relaxed, SeqCst}; use std::sync::atomic::{AtomicBool, AtomicUsize}; use std::sync::{Arc, Mutex}; +use std::{fmt, slice}; -use crate::{Interest, Token}; use miow::iocp::{CompletionPort, CompletionStatus}; use miow::pipe; use winapi::shared::winerror::{ERROR_BROKEN_PIPE, ERROR_PIPE_LISTENING}; use winapi::um::ioapiset::CancelIoEx; +use winapi::um::minwinbase::OVERLAPPED_ENTRY; -/// # Safety -/// -/// Only valid if the strict is annotated with `#[repr(C)]`. This is only used -/// with `Overlapped` and `Inner`, which are correctly annotated. -macro_rules! offset_of { - ($t:ty, $($field:ident).+) => ( - &(*(0 as *const $t)).$($field).+ as *const _ as usize - ) -} - -macro_rules! overlapped2arc { - ($e:expr, $t:ty, $($field:ident).+) => ({ - let offset = offset_of!($t, $($field).+); - debug_assert!(offset < mem::size_of::<$t>()); - Arc::from_raw(($e as usize - offset) as *mut $t) - }) -} +use crate::event::Source; +use crate::sys::windows::{Event, Overlapped}; +use crate::{poll, Registry}; +use crate::{Interest, Token}; /// Non-blocking windows named pipe. /// @@ -83,6 +64,10 @@ pub struct NamedPipe { inner: Arc, } +/// # Notes +/// +/// The memory layout of this structure must be fixed as the `*_OFFSET` +/// constants depend on it, see the `offset_constants` test. #[repr(C)] struct Inner { handle: pipe::NamedPipe, @@ -98,6 +83,28 @@ struct Inner { pool: Mutex, } +/// Offsets from a pointer to `Inner` to the `connect`, `read` and `write` +/// fields. +const CONNECT_OFFSET: usize = size_of::(); // `handle` field. +const READ_OFFSET: usize = CONNECT_OFFSET + size_of::() + size_of::(); // `connect`, `connecting` (`bool` + padding) fields. +const WRITE_OFFSET: usize = READ_OFFSET + size_of::(); // `read` fields. + +#[test] +fn offset_constants() { + use std::mem::ManuallyDrop; + use std::ptr; + + let pipe = unsafe { ManuallyDrop::new(NamedPipe::from_raw_handle(ptr::null_mut())) }; + let inner: &Inner = &pipe.inner; + let base = inner as *const Inner as usize; + let connect = &inner.connect as *const Overlapped as usize; + assert_eq!(base + CONNECT_OFFSET, connect); + let read = &inner.read as *const Overlapped as usize; + assert_eq!(base + READ_OFFSET, read); + let write = &inner.write as *const Overlapped as usize; + assert_eq!(base + WRITE_OFFSET, write); +} + struct Io { // Uniquely identifies the selector associated with this named pipe cp: Option>, @@ -587,7 +594,8 @@ fn connect_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `Arc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `connect` above. - let me = unsafe { overlapped2arc!(status.overlapped(), Inner, connect) }; + let me = + unsafe { Arc::from_raw((status.overlapped() as usize - CONNECT_OFFSET) as *mut Inner) }; // Flag ourselves as no longer using the `connect` overlapped instances. let prev = me.connecting.swap(false, SeqCst); @@ -613,7 +621,7 @@ fn read_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `FromRawArc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `schedule_read` above. - let me = unsafe { overlapped2arc!(status.overlapped(), Inner, read) }; + let me = unsafe { Arc::from_raw((status.overlapped() as usize - READ_OFFSET) as *mut Inner) }; // Move from the `Pending` to `Ok` state. let mut io = me.io.lock().unwrap(); @@ -645,7 +653,7 @@ fn write_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `Arc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `schedule_write` above. - let me = unsafe { overlapped2arc!(status.overlapped(), Inner, write) }; + let me = unsafe { Arc::from_raw((status.overlapped() as usize - WRITE_OFFSET) as *mut Inner) }; // Make the state change out of `Pending`. If we wrote the entire buffer // then we're writable again and otherwise we schedule another write. diff --git a/tests/regressions.rs b/tests/regressions.rs index 2e2bacd00..f41c6cae5 100644 --- a/tests/regressions.rs +++ b/tests/regressions.rs @@ -9,7 +9,7 @@ use mio::net::{TcpListener, TcpStream}; use mio::{Events, Interest, Poll, Token, Waker}; mod util; -use util::{any_local_address, init, init_with_poll, temp_file}; +use util::{any_local_address, init, init_with_poll}; const ID1: Token = Token(1); const WAKE_TOKEN: Token = Token(10); @@ -109,6 +109,7 @@ fn issue_1205() { #[cfg(unix)] fn issue_1403() { use mio::net::UnixDatagram; + use util::temp_file; init(); From ba944bb56e97638c6352502d9f5ac1a117ff13cd Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Mon, 12 Apr 2021 20:26:53 +0200 Subject: [PATCH 2/6] Reorder NamedPipe fields Moving the Overlapped fields to the start to make it easier to determine the offsets and hopefully incur less breakage once external fields change size. Note that the Overlapped fields internally uses miow::Overlapped, which in turn is a OVERLAPPED struct as found in the winapi crate and has a stable layout (as defined by the Windows API). --- src/sys/windows/named_pipe.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/sys/windows/named_pipe.rs b/src/sys/windows/named_pipe.rs index 49064317b..5ac025a8b 100644 --- a/src/sys/windows/named_pipe.rs +++ b/src/sys/windows/named_pipe.rs @@ -70,23 +70,19 @@ pub struct NamedPipe { /// constants depend on it, see the `offset_constants` test. #[repr(C)] struct Inner { - handle: pipe::NamedPipe, - connect: Overlapped, - connecting: AtomicBool, - read: Overlapped, write: Overlapped, - + handle: pipe::NamedPipe, + connecting: AtomicBool, io: Mutex, - pool: Mutex, } /// Offsets from a pointer to `Inner` to the `connect`, `read` and `write` /// fields. -const CONNECT_OFFSET: usize = size_of::(); // `handle` field. -const READ_OFFSET: usize = CONNECT_OFFSET + size_of::() + size_of::(); // `connect`, `connecting` (`bool` + padding) fields. +const CONNECT_OFFSET: usize = 0; +const READ_OFFSET: usize = CONNECT_OFFSET + size_of::(); // `connect` fields. const WRITE_OFFSET: usize = READ_OFFSET + size_of::(); // `read` fields. #[test] From 556d1971350966a51893969e0ea6b9ad1ea5fdba Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Wed, 5 May 2021 12:13:47 +0200 Subject: [PATCH 3/6] Replace offset constants with methods in Windows NamedPipe --- src/sys/windows/named_pipe.rs | 64 +++++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/src/sys/windows/named_pipe.rs b/src/sys/windows/named_pipe.rs index 5ac025a8b..d8c38f03e 100644 --- a/src/sys/windows/named_pipe.rs +++ b/src/sys/windows/named_pipe.rs @@ -11,7 +11,7 @@ use miow::iocp::{CompletionPort, CompletionStatus}; use miow::pipe; use winapi::shared::winerror::{ERROR_BROKEN_PIPE, ERROR_PIPE_LISTENING}; use winapi::um::ioapiset::CancelIoEx; -use winapi::um::minwinbase::OVERLAPPED_ENTRY; +use winapi::um::minwinbase::{OVERLAPPED, OVERLAPPED_ENTRY}; use crate::event::Source; use crate::sys::windows::{Event, Overlapped}; @@ -70,35 +70,64 @@ pub struct NamedPipe { /// constants depend on it, see the `offset_constants` test. #[repr(C)] struct Inner { + // NOTE: careful modifying the order of these three fields, the `ptr_from_*` + // methods depend on the layout! connect: Overlapped, read: Overlapped, write: Overlapped, + // END NOTE. handle: pipe::NamedPipe, connecting: AtomicBool, io: Mutex, pool: Mutex, } -/// Offsets from a pointer to `Inner` to the `connect`, `read` and `write` -/// fields. -const CONNECT_OFFSET: usize = 0; -const READ_OFFSET: usize = CONNECT_OFFSET + size_of::(); // `connect` fields. -const WRITE_OFFSET: usize = READ_OFFSET + size_of::(); // `read` fields. +impl Inner { + /// Converts a pointer to `Inner.connect` to a pointer to `Inner`. + /// + /// # Unsafety + /// + /// Caller must ensure `ptr` is pointing to `Inner.connect`. + unsafe fn ptr_from_conn_overlapped(ptr: *mut OVERLAPPED) -> *const Inner { + // `connect` is the first field, so the pointer are the same. + ptr.cast() + } + + /// Same as [`ptr_from_conn_overlapped`] but for `Inner.read`. + unsafe fn ptr_from_read_overlapped(ptr: *mut OVERLAPPED) -> *const Inner { + // `read` is after `connect: Overlapped`. + (ptr as usize + size_of::()) as *const Inner + } + + /// Same as [`ptr_from_conn_overlapped`] but for `Inner.write`. + unsafe fn ptr_from_write_overlapped(ptr: *mut OVERLAPPED) -> *const Inner { + // `read` is after `connect: Overlapped` and `read: Overlapped`. + (ptr as usize + (2 * size_of::())) as *const Inner + } +} #[test] -fn offset_constants() { +fn ptr_from() { use std::mem::ManuallyDrop; use std::ptr; let pipe = unsafe { ManuallyDrop::new(NamedPipe::from_raw_handle(ptr::null_mut())) }; let inner: &Inner = &pipe.inner; - let base = inner as *const Inner as usize; - let connect = &inner.connect as *const Overlapped as usize; - assert_eq!(base + CONNECT_OFFSET, connect); - let read = &inner.read as *const Overlapped as usize; - assert_eq!(base + READ_OFFSET, read); - let write = &inner.write as *const Overlapped as usize; - assert_eq!(base + WRITE_OFFSET, write); + assert_eq!( + inner as *const Inner, + unsafe { Inner::ptr_from_conn_overlapped(&inner.connect as *const _ as *mut OVERLAPPED) }, + "`ptr_from_conn_overlapped` incorrect" + ); + assert_eq!( + inner as *const Inner, + unsafe { Inner::ptr_from_read_overlapped(&inner.read as *const _ as *mut OVERLAPPED) }, + "`ptr_from_read_overlapped` incorrect" + ); + assert_eq!( + inner as *const Inner, + unsafe { Inner::ptr_from_write_overlapped(&inner.write as *const _ as *mut OVERLAPPED) }, + "`ptr_from_write_overlapped` incorrect" + ); } struct Io { @@ -590,8 +619,7 @@ fn connect_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `Arc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `connect` above. - let me = - unsafe { Arc::from_raw((status.overlapped() as usize - CONNECT_OFFSET) as *mut Inner) }; + let me = unsafe { Arc::from_raw(Inner::ptr_from_conn_overlapped(status.overlapped())) }; // Flag ourselves as no longer using the `connect` overlapped instances. let prev = me.connecting.swap(false, SeqCst); @@ -617,7 +645,7 @@ fn read_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `FromRawArc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `schedule_read` above. - let me = unsafe { Arc::from_raw((status.overlapped() as usize - READ_OFFSET) as *mut Inner) }; + let me = unsafe { Arc::from_raw(Inner::ptr_from_read_overlapped(status.overlapped())) }; // Move from the `Pending` to `Ok` state. let mut io = me.io.lock().unwrap(); @@ -649,7 +677,7 @@ fn write_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec>) { // Acquire the `Arc`. Note that we should be guaranteed that // the refcount is available to us due to the `mem::forget` in // `schedule_write` above. - let me = unsafe { Arc::from_raw((status.overlapped() as usize - WRITE_OFFSET) as *mut Inner) }; + let me = unsafe { Arc::from_raw(Inner::ptr_from_write_overlapped(status.overlapped())) }; // Make the state change out of `Pending`. If we wrote the entire buffer // then we're writable again and otherwise we schedule another write. From 095e2c579dff8e69dfd007cbe5eee8eed9b48dad Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Mon, 10 May 2021 11:56:41 +0200 Subject: [PATCH 4/6] Avoid cast pointers to usize in windows::NamedPipe Changes the Inner::ptr_from_* methods to use ptr::wrapping_sub rather then casting to usize. --- src/sys/windows/named_pipe.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/sys/windows/named_pipe.rs b/src/sys/windows/named_pipe.rs index d8c38f03e..d8fe619ce 100644 --- a/src/sys/windows/named_pipe.rs +++ b/src/sys/windows/named_pipe.rs @@ -1,11 +1,10 @@ use std::ffi::OsStr; use std::io::{self, Read, Write}; -use std::mem::{self, size_of}; use std::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, RawHandle}; use std::sync::atomic::Ordering::{Relaxed, SeqCst}; use std::sync::atomic::{AtomicBool, AtomicUsize}; use std::sync::{Arc, Mutex}; -use std::{fmt, slice}; +use std::{fmt, mem, slice}; use miow::iocp::{CompletionPort, CompletionStatus}; use miow::pipe; @@ -96,13 +95,13 @@ impl Inner { /// Same as [`ptr_from_conn_overlapped`] but for `Inner.read`. unsafe fn ptr_from_read_overlapped(ptr: *mut OVERLAPPED) -> *const Inner { // `read` is after `connect: Overlapped`. - (ptr as usize + size_of::()) as *const Inner + (ptr as *mut Overlapped).wrapping_sub(1) as *const Inner } /// Same as [`ptr_from_conn_overlapped`] but for `Inner.write`. unsafe fn ptr_from_write_overlapped(ptr: *mut OVERLAPPED) -> *const Inner { // `read` is after `connect: Overlapped` and `read: Overlapped`. - (ptr as usize + (2 * size_of::())) as *const Inner + (ptr as *mut Overlapped).wrapping_sub(2) as *const Inner } } From b21a5ee39b4c061774b572e406be34f9297e53c8 Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Mon, 10 May 2021 12:10:28 +0200 Subject: [PATCH 5/6] Replace x86_64-sun-solaris with x86_64-pc-solaris https://github.com/rust-lang/rust/pull/82216 removed the x86_64-sun-solaris target from rustup, changing it to use x86_64-pc-solaris instead. Related issues: * https://github.com/rust-lang/rust/issues/85098 --- Cargo.toml | 2 +- Makefile | 2 +- ci/azure-cross-compile.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bf9f3e147..cd1ccaf5c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,7 +65,7 @@ targets = [ "aarch64-linux-android", "x86_64-apple-darwin", "x86_64-pc-windows-msvc", - "x86_64-sun-solaris", + "x86_64-pc-solaris", "x86_64-unknown-dragonfly", "x86_64-unknown-freebsd", "x86_64-unknown-linux-gnu", diff --git a/Makefile b/Makefile index fb4453826..8f797612c 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ # Targets available via Rustup that are supported. -TARGETS ?= "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-linux-gnu" "x86_64-unknown-netbsd" +TARGETS ?= "aarch64-apple-ios" "aarch64-linux-android" "x86_64-apple-darwin" "x86_64-pc-windows-msvc" "x86_64-pc-solaris" "x86_64-unknown-freebsd" "x86_64-unknown-linux-gnu" "x86_64-unknown-netbsd" test: cargo test --all-features diff --git a/ci/azure-cross-compile.yml b/ci/azure-cross-compile.yml index 763828a43..73225df37 100644 --- a/ci/azure-cross-compile.yml +++ b/ci/azure-cross-compile.yml @@ -32,7 +32,7 @@ jobs: Solaris: vmImage: ubuntu-16.04 - target: x86_64-sun-solaris + target: x86_64-pc-solaris pool: vmImage: $(vmImage) From 881b343180a46d25f0900acc9015547f57261178 Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Thu, 13 May 2021 17:09:57 +0200 Subject: [PATCH 6/6] Update outdated comment --- src/sys/windows/named_pipe.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sys/windows/named_pipe.rs b/src/sys/windows/named_pipe.rs index d8fe619ce..c51df3907 100644 --- a/src/sys/windows/named_pipe.rs +++ b/src/sys/windows/named_pipe.rs @@ -65,8 +65,8 @@ pub struct NamedPipe { /// # Notes /// -/// The memory layout of this structure must be fixed as the `*_OFFSET` -/// constants depend on it, see the `offset_constants` test. +/// The memory layout of this structure must be fixed as the +/// `ptr_from_*_overlapped` methods depend on it, see the `ptr_from` test. #[repr(C)] struct Inner { // NOTE: careful modifying the order of these three fields, the `ptr_from_*`