From db0d74cce5d492d5f8381aadf27877fe8806d2b1 Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Mon, 12 Apr 2021 20:03:24 +0200 Subject: [PATCH] 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();