From 291ac5fb4c7a9ab2a8d616becb97e36b0f29e9ea Mon Sep 17 00:00:00 2001 From: Ian Douglas Scott Date: Mon, 20 Nov 2023 14:35:35 -0800 Subject: [PATCH] backend: Use `Message<_, BorrowedFd>` rather than `RawFd` This is a breaking change to `wayland-backend`, but not to `wayland-client` or `wayland-server`. I believe that should be fine. This avoids an unsafe conversion in the `rs` backend. --- wayland-backend/src/client_api.rs | 4 +- wayland-backend/src/rs/client_impl/mod.rs | 4 +- wayland-backend/src/rs/server_impl/client.rs | 4 +- wayland-backend/src/rs/server_impl/handle.rs | 10 ++--- wayland-backend/src/rs/socket.rs | 41 +++++++++++--------- wayland-backend/src/rs/wire.rs | 11 ++---- wayland-backend/src/server_api.rs | 4 +- wayland-backend/src/sys/client_impl/mod.rs | 6 +-- wayland-backend/src/sys/server_impl/mod.rs | 10 ++--- wayland-backend/src/test/many_args.rs | 8 +++- wayland-client/src/conn.rs | 3 +- wayland-server/src/display.rs | 3 +- 12 files changed, 56 insertions(+), 52 deletions(-) diff --git a/wayland-backend/src/client_api.rs b/wayland-backend/src/client_api.rs index a8633657e5c..b489a420c05 100644 --- a/wayland-backend/src/client_api.rs +++ b/wayland-backend/src/client_api.rs @@ -2,7 +2,7 @@ use std::{ any::Any, fmt, os::unix::io::{BorrowedFd, OwnedFd}, - os::unix::{io::RawFd, net::UnixStream}, + os::unix::net::UnixStream, sync::Arc, }; @@ -227,7 +227,7 @@ impl Backend { /// is `wl_registry.bind`), the `child_spec` must be provided. pub fn send_request( &self, - msg: Message, + msg: Message, data: Option>, child_spec: Option<(&'static Interface, u32)>, ) -> Result { diff --git a/wayland-backend/src/rs/client_impl/mod.rs b/wayland-backend/src/rs/client_impl/mod.rs index 8001fa3e623..066242924ba 100644 --- a/wayland-backend/src/rs/client_impl/mod.rs +++ b/wayland-backend/src/rs/client_impl/mod.rs @@ -4,7 +4,7 @@ use std::{ fmt, os::unix::io::{BorrowedFd, OwnedFd}, os::unix::{ - io::{AsRawFd, RawFd}, + io::AsRawFd, net::UnixStream, }, sync::{Arc, Condvar, Mutex, MutexGuard, Weak}, @@ -303,7 +303,7 @@ impl InnerBackend { pub fn send_request( &self, - Message { sender_id: ObjectId { id }, opcode, args }: Message, + Message { sender_id: ObjectId { id }, opcode, args }: Message, data: Option>, child_spec: Option<(&'static Interface, u32)>, ) -> Result { diff --git a/wayland-backend/src/rs/server_impl/client.rs b/wayland-backend/src/rs/server_impl/client.rs index 44c216db6de..48109036397 100644 --- a/wayland-backend/src/rs/server_impl/client.rs +++ b/wayland-backend/src/rs/server_impl/client.rs @@ -1,7 +1,7 @@ use std::{ ffi::CString, os::unix::io::{AsFd, BorrowedFd, OwnedFd}, - os::unix::{io::RawFd, net::UnixStream}, + os::unix::net::UnixStream, sync::Arc, }; @@ -106,7 +106,7 @@ impl Client { pub(crate) fn send_event( &mut self, - Message { sender_id: object_id, opcode, args }: Message, + Message { sender_id: object_id, opcode, args }: Message, pending_destructors: Option<&mut Vec>>, ) -> Result<(), InvalidId> { if self.killed { diff --git a/wayland-backend/src/rs/server_impl/handle.rs b/wayland-backend/src/rs/server_impl/handle.rs index 5030819b279..07d7a84e99f 100644 --- a/wayland-backend/src/rs/server_impl/handle.rs +++ b/wayland-backend/src/rs/server_impl/handle.rs @@ -1,7 +1,7 @@ use std::{ ffi::CString, - os::unix::io::OwnedFd, - os::unix::{io::RawFd, net::UnixStream}, + os::unix::io::{BorrowedFd, OwnedFd}, + os::unix::net::UnixStream, sync::{Arc, Mutex, Weak}, }; @@ -173,7 +173,7 @@ impl InnerHandle { } } - pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { + pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { self.state.lock().unwrap().send_event(msg) } @@ -292,7 +292,7 @@ pub(crate) trait ErasedState: downcast_rs::Downcast { &self, id: InnerObjectId, ) -> Result, InvalidId>; - fn send_event(&mut self, msg: Message) -> Result<(), InvalidId>; + fn send_event(&mut self, msg: Message) -> Result<(), InvalidId>; fn post_error(&mut self, object_id: InnerObjectId, error_code: u32, message: CString); fn kill_client(&mut self, client_id: InnerClientId, reason: DisconnectReason); fn global_info(&self, id: InnerGlobalId) -> Result; @@ -416,7 +416,7 @@ impl ErasedState for State { .map(|arc| arc.into_any_arc()) } - fn send_event(&mut self, msg: Message) -> Result<(), InvalidId> { + fn send_event(&mut self, msg: Message) -> Result<(), InvalidId> { self.clients .get_client_mut(msg.sender_id.id.client_id.clone())? .send_event(msg, Some(&mut self.pending_destructors)) diff --git a/wayland-backend/src/rs/socket.rs b/wayland-backend/src/rs/socket.rs index c9f790af2f1..ec45aba4002 100644 --- a/wayland-backend/src/rs/socket.rs +++ b/wayland-backend/src/rs/socket.rs @@ -155,7 +155,7 @@ impl BufferedSocket { // // if false is returned, it means there is not enough space // in the buffer - fn attempt_write_message(&mut self, msg: &Message) -> IoResult { + fn attempt_write_message(&mut self, msg: &Message) -> IoResult { match write_to_buffers(msg, self.out_data.get_writable_storage(), &mut self.out_fds) { Ok(bytes_out) => { self.out_data.advance(bytes_out); @@ -172,7 +172,7 @@ impl BufferedSocket { /// /// If the message is too big to fit in the buffer, the error `Error::Sys(E2BIG)` /// will be returned. - pub fn write_message(&mut self, msg: &Message) -> IoResult<()> { + pub fn write_message(&mut self, msg: &Message) -> IoResult<()> { if !self.attempt_write_message(msg)? { // the attempt failed, there is not enough space in the buffer // we need to flush it @@ -320,8 +320,8 @@ mod tests { use crate::protocol::{AllowNull, Argument, ArgumentType, Message}; use std::ffi::CString; + use std::io; use std::os::unix::io::BorrowedFd; - use std::os::unix::prelude::IntoRawFd; use smallvec::smallvec; @@ -335,18 +335,18 @@ mod tests { // // if arguments contain FDs, check that the fd point to // the same file, rather than are the same number. - fn assert_eq_msgs( - msg1: &Message, - msg2: &Message, + fn assert_eq_msgs( + msg1: Message, + msg2: Message, ) { + let msg1 = msg1.map_fd(|fd| fd.as_fd().try_clone_to_owned().unwrap()); + let msg2 = msg2.map_fd(|fd| fd.as_fd().try_clone_to_owned().unwrap()); assert_eq!(msg1.sender_id, msg2.sender_id); assert_eq!(msg1.opcode, msg2.opcode); assert_eq!(msg1.args.len(), msg2.args.len()); for (arg1, arg2) in msg1.args.iter().zip(msg2.args.iter()) { if let (Argument::Fd(fd1), Argument::Fd(fd2)) = (arg1, arg2) { - let fd1 = unsafe { BorrowedFd::borrow_raw(fd1.as_raw_fd()) }; - let fd2 = unsafe { BorrowedFd::borrow_raw(fd2.as_raw_fd()) }; - assert!(same_file(fd1, fd2)); + assert!(same_file(fd1.as_fd(), fd2.as_fd())); } else { assert_eq!(arg1, arg2); } @@ -399,17 +399,19 @@ mod tests { }) .unwrap(); - assert_eq_msgs(&msg.map_fd(|fd| fd.as_raw_fd()), &ret_msg.map_fd(IntoRawFd::into_raw_fd)); + assert_eq_msgs(msg, ret_msg); } #[test] fn write_read_cycle_fd() { + let stdin = io::stdin().lock(); + let stdout = io::stdout().lock(); let msg = Message { sender_id: 42, opcode: 7, args: smallvec![ - Argument::Fd(1), // stdin - Argument::Fd(0), // stdout + Argument::Fd(stdin.as_fd()), + Argument::Fd(stdout.as_fd()), ], }; @@ -434,11 +436,14 @@ mod tests { } }) .unwrap(); - assert_eq_msgs(&msg.map_fd(|fd| fd.as_raw_fd()), &ret_msg.map_fd(IntoRawFd::into_raw_fd)); + assert_eq_msgs(msg, ret_msg); } #[test] fn write_read_cycle_multiple() { + let stdin = io::stderr().lock(); + let stdout = io::stderr().lock(); + let stderr = io::stderr().lock(); let messages = vec![ Message { sender_id: 42, @@ -452,8 +457,8 @@ mod tests { sender_id: 42, opcode: 1, args: smallvec![ - Argument::Fd(1), // stdin - Argument::Fd(0), // stdout + Argument::Fd(stdin.as_fd()), + Argument::Fd(stdout.as_fd()), ], }, Message { @@ -461,7 +466,7 @@ mod tests { opcode: 2, args: smallvec![ Argument::Uint(3), - Argument::Fd(2), // stderr + Argument::Fd(stderr.as_fd()), ], }, ]; @@ -495,7 +500,7 @@ mod tests { } assert_eq!(recv_msgs.len(), 3); for (msg1, msg2) in messages.into_iter().zip(recv_msgs.into_iter()) { - assert_eq_msgs(&msg1.map_fd(|fd| fd.as_raw_fd()), &msg2.map_fd(IntoRawFd::into_raw_fd)); + assert_eq_msgs(msg1, msg2); } } @@ -534,6 +539,6 @@ mod tests { }) .unwrap(); - assert_eq_msgs(&msg.map_fd(|fd| fd.as_raw_fd()), &ret_msg.map_fd(IntoRawFd::into_raw_fd)); + assert_eq_msgs(msg, ret_msg); } } diff --git a/wayland-backend/src/rs/wire.rs b/wayland-backend/src/rs/wire.rs index e2922439a59..a60f0cd5a4b 100644 --- a/wayland-backend/src/rs/wire.rs +++ b/wayland-backend/src/rs/wire.rs @@ -3,7 +3,6 @@ use std::collections::VecDeque; use std::convert::TryInto; use std::ffi::CStr; -use std::os::unix::io::RawFd; use std::os::unix::io::{BorrowedFd, OwnedFd}; use crate::protocol::{Argument, ArgumentType, Message}; @@ -71,7 +70,7 @@ impl std::fmt::Display for MessageParseError { /// /// Any serialized Fd will be `dup()`-ed in the process pub fn write_to_buffers( - msg: &Message, + msg: &Message, payload: &mut [u8], fds: &mut Vec, ) -> Result { @@ -126,9 +125,7 @@ pub fn write_to_buffers( Argument::NewId(n) => write_buf(n, payload)?, Argument::Array(ref a) => write_array_to_payload(a, payload)?, Argument::Fd(fd) => { - let dup_fd = unsafe { BorrowedFd::borrow_raw(fd) } - .try_clone_to_owned() - .map_err(MessageWriteError::DupFdFailed)?; + let dup_fd = fd.try_clone_to_owned().map_err(MessageWriteError::DupFdFailed)?; fds.push(dup_fd); payload } @@ -247,7 +244,7 @@ mod tests { use super::*; use crate::protocol::AllowNull; use smallvec::smallvec; - use std::{ffi::CString, os::unix::io::IntoRawFd}; + use std::ffi::CString; #[test] fn into_from_raw_cycle() { @@ -285,7 +282,7 @@ mod tests { &mut fd_buffer, ) .unwrap(); - assert_eq!(rebuilt.map_fd(IntoRawFd::into_raw_fd), msg); + assert_eq!(rebuilt, msg.map_fd(|fd| fd.try_clone_to_owned().unwrap())); } } diff --git a/wayland-backend/src/server_api.rs b/wayland-backend/src/server_api.rs index 6e3565db1c0..cfbf9f4f2b8 100644 --- a/wayland-backend/src/server_api.rs +++ b/wayland-backend/src/server_api.rs @@ -2,7 +2,7 @@ use std::{ ffi::CString, fmt, os::unix::io::{BorrowedFd, OwnedFd}, - os::unix::{io::RawFd, net::UnixStream}, + os::unix::net::UnixStream, sync::Arc, }; @@ -367,7 +367,7 @@ impl Handle { /// - the message opcode must be valid for the sender interface /// - the argument list must match the prototype for the message associated with this opcode #[inline] - pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { + pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { self.handle.send_event(msg) } diff --git a/wayland-backend/src/sys/client_impl/mod.rs b/wayland-backend/src/sys/client_impl/mod.rs index 946b5918201..be557356255 100644 --- a/wayland-backend/src/sys/client_impl/mod.rs +++ b/wayland-backend/src/sys/client_impl/mod.rs @@ -6,7 +6,7 @@ use std::{ os::raw::{c_int, c_void}, os::unix::io::{BorrowedFd, OwnedFd}, os::unix::{ - io::{FromRawFd, IntoRawFd, RawFd}, + io::{AsRawFd, FromRawFd, IntoRawFd}, net::UnixStream, }, sync::{ @@ -520,7 +520,7 @@ impl InnerBackend { pub fn send_request( &self, - Message { sender_id: ObjectId { id }, opcode, args }: Message, + Message { sender_id: ObjectId { id }, opcode, args }: Message, data: Option>, child_spec: Option<(&'static Interface, u32)>, ) -> Result { @@ -615,7 +615,7 @@ impl InnerBackend { Argument::Uint(u) => argument_list.push(wl_argument { u }), Argument::Int(i) => argument_list.push(wl_argument { i }), Argument::Fixed(f) => argument_list.push(wl_argument { f }), - Argument::Fd(h) => argument_list.push(wl_argument { h }), + Argument::Fd(h) => argument_list.push(wl_argument { h: h.as_raw_fd() }), Argument::Array(ref a) => { let a = Box::new(wl_array { size: a.len(), diff --git a/wayland-backend/src/sys/server_impl/mod.rs b/wayland-backend/src/sys/server_impl/mod.rs index e2b1b34d8b4..bbf73659f01 100644 --- a/wayland-backend/src/sys/server_impl/mod.rs +++ b/wayland-backend/src/sys/server_impl/mod.rs @@ -5,7 +5,7 @@ use std::{ os::raw::{c_int, c_void}, os::unix::io::{BorrowedFd, OwnedFd}, os::unix::{ - io::{FromRawFd, IntoRawFd, RawFd}, + io::{AsRawFd, FromRawFd, IntoRawFd}, net::UnixStream, }, sync::{ @@ -575,7 +575,7 @@ impl InnerHandle { } } - pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { + pub fn send_event(&self, msg: Message) -> Result<(), InvalidId> { self.state.lock().unwrap().send_event(msg) } @@ -848,7 +848,7 @@ pub(crate) trait ErasedState: downcast_rs::Downcast { &self, id: InnerObjectId, ) -> Result, InvalidId>; - fn send_event(&mut self, msg: Message) -> Result<(), InvalidId>; + fn send_event(&mut self, msg: Message) -> Result<(), InvalidId>; fn post_error(&mut self, object_id: InnerObjectId, error_code: u32, message: CString); fn kill_client(&mut self, client_id: InnerClientId, reason: DisconnectReason); fn global_info(&self, id: InnerGlobalId) -> Result; @@ -1048,7 +1048,7 @@ impl ErasedState for State { fn send_event( &mut self, - Message { sender_id: ObjectId { id }, opcode, args }: Message, + Message { sender_id: ObjectId { id }, opcode, args }: Message, ) -> Result<(), InvalidId> { if !id.alive.load(Ordering::Acquire) || id.ptr.is_null() { return Err(InvalidId); @@ -1075,7 +1075,7 @@ impl ErasedState for State { Argument::Uint(u) => argument_list.push(wl_argument { u }), Argument::Int(i) => argument_list.push(wl_argument { i }), Argument::Fixed(f) => argument_list.push(wl_argument { f }), - Argument::Fd(h) => argument_list.push(wl_argument { h }), + Argument::Fd(h) => argument_list.push(wl_argument { h: h.as_raw_fd() }), Argument::Array(ref a) => { let a = Box::new(wl_array { size: a.len(), diff --git a/wayland-backend/src/test/many_args.rs b/wayland-backend/src/test/many_args.rs index 565011a80a4..1abad6e2f8e 100644 --- a/wayland-backend/src/test/many_args.rs +++ b/wayland-backend/src/test/many_args.rs @@ -1,5 +1,7 @@ use std::{ ffi::{CStr, CString}, + io, + os::unix::io::AsFd, sync::atomic::{AtomicBool, Ordering}, }; @@ -61,6 +63,7 @@ macro_rules! serverdata_impls { _: $server_backend::GlobalId, object_id: $server_backend::ObjectId, ) -> Arc> { + let stdout = io::stdout().lock(); handle .send_event(message!( object_id, @@ -71,7 +74,7 @@ macro_rules! serverdata_impls { Argument::Fixed(9823), Argument::Array(Box::new(vec![10, 20, 30, 40, 50, 60, 70, 80, 90])), Argument::Str(Some(Box::new(CString::new("I want cake".as_bytes()).unwrap()))), - Argument::Fd(1), // stdout + Argument::Fd(stdout.as_fd()), ], )) .unwrap(); @@ -171,6 +174,7 @@ expand_test!(many_args, { assert!(client_data.0.load(Ordering::SeqCst)); // send the many_args request + let stdin = io::stdin().lock(); client .send_request( message!( @@ -184,7 +188,7 @@ expand_test!(many_args, { Argument::Str(Some(Box::new( CString::new("I like trains".as_bytes()).unwrap() ))), - Argument::Fd(0), // stdin + Argument::Fd(stdin.as_fd()), ], ), None, diff --git a/wayland-client/src/conn.rs b/wayland-client/src/conn.rs index 5900ac54c29..b56a25d8d11 100644 --- a/wayland-client/src/conn.rs +++ b/wayland-client/src/conn.rs @@ -1,7 +1,7 @@ use std::{ env, fmt, io::ErrorKind, - os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd}, + os::unix::io::{AsFd, BorrowedFd, FromRawFd, OwnedFd}, os::unix::net::UnixStream, path::PathBuf, sync::{ @@ -197,7 +197,6 @@ impl Connection { data: Option>, ) -> Result { let (msg, child_spec) = proxy.write_request(self, request)?; - let msg = msg.map_fd(|fd| fd.as_raw_fd()); self.backend.send_request(msg, data, child_spec) } diff --git a/wayland-server/src/display.rs b/wayland-server/src/display.rs index fb1fcaa2329..79bd64d06d4 100644 --- a/wayland-server/src/display.rs +++ b/wayland-server/src/display.rs @@ -1,5 +1,5 @@ use std::{ - os::unix::io::{AsFd, AsRawFd, BorrowedFd}, + os::unix::io::{AsFd, BorrowedFd}, os::unix::net::UnixStream, sync::Arc, }; @@ -178,7 +178,6 @@ impl DisplayHandle { event: I::Event<'_>, ) -> Result<(), InvalidId> { let msg = resource.write_event(self, event)?; - let msg = msg.map_fd(|fd| fd.as_raw_fd()); self.handle.send_event(msg) }