From 5f53d871aeb3bbb4ee033ff488ecaaf9c22f6972 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Thu, 11 Jul 2024 22:05:20 +0200 Subject: [PATCH 1/7] fix various code quality issues, including clippy warnings including: - replace `str::from_utf8(cstr.to_bytes())` with `cstr.to_str()` - remove unnecessary `as` casts (numeric and pointer) - remove unused import `std::str`, not detected by rustc because of a bug --- src/lib.rs | 11 +++++------ src/message.rs | 2 +- src/sockopt.rs | 5 ++--- tests/message_from_boxed_slice.rs | 2 +- tests/monitor.rs | 1 - tests/test.rs | 2 +- tests/unix/connection.rs | 2 +- tests/z85.rs | 6 +++--- 8 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 17dc0ccd3..e62e74456 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,10 +13,10 @@ use std::os::raw::c_void; use std::os::unix::io::{AsRawFd, RawFd as UnixRawFd}; #[cfg(windows)] use std::os::windows::io::{AsRawSocket, RawSocket}; +use std::ptr; use std::result; use std::string::FromUtf8Error; use std::sync::Arc; -use std::{mem, ptr, str}; use zmq_sys::{errno, RawFd}; @@ -298,7 +298,7 @@ impl Error { panic!( "unknown error [{}]: {}", x, - str::from_utf8(ffi::CStr::from_ptr(s).to_bytes()).unwrap() + ffi::CStr::from_ptr(s).to_str().unwrap() ) }, } @@ -308,8 +308,7 @@ impl Error { pub fn message(self) -> &'static str { unsafe { let s = zmq_sys::zmq_strerror(self.to_raw()); - let v: &'static [u8] = mem::transmute(ffi::CStr::from_ptr(s).to_bytes()); - str::from_utf8(v).unwrap() + ffi::CStr::from_ptr(s).to_str().unwrap() } } } @@ -371,7 +370,7 @@ pub fn version() -> (i32, i32, i32) { zmq_sys::zmq_version(&mut major, &mut minor, &mut patch); } - (major as i32, minor as i32, patch as i32) + (major, minor, patch) } struct RawContext { @@ -442,7 +441,7 @@ impl Context { /// Set the size of the ØMQ thread pool to handle I/O operations. pub fn set_io_threads(&self, value: i32) -> Result<()> { zmq_try!(unsafe { - zmq_sys::zmq_ctx_set(self.raw.ctx, zmq_sys::ZMQ_IO_THREADS as _, value as i32) + zmq_sys::zmq_ctx_set(self.raw.ctx, zmq_sys::ZMQ_IO_THREADS as _, value) }); Ok(()) } diff --git a/src/message.rs b/src/message.rs index 0350084d5..32b457bc1 100644 --- a/src/message.rs +++ b/src/message.rs @@ -152,7 +152,7 @@ impl Message { if value.is_null() { None } else { - str::from_utf8(unsafe { ffi::CStr::from_ptr(value) }.to_bytes()).ok() + unsafe { ffi::CStr::from_ptr(value) }.to_str().ok() } } } diff --git a/src/sockopt.rs b/src/sockopt.rs index 77c3c848f..a1b724616 100644 --- a/src/sockopt.rs +++ b/src/sockopt.rs @@ -2,7 +2,7 @@ use libc::{c_int, c_uint, size_t}; use std::os::raw::c_void; use std::result; use std::string::FromUtf8Error; -use std::{mem, ptr, str}; +use std::{mem, ptr}; use super::{PollEvents, Result}; @@ -47,8 +47,7 @@ getsockopt_num!(c_uint, u32); getsockopt_num!(i64, i64); getsockopt_num!(u64, u64); -pub fn get_bytes(sock: *mut c_void, opt: c_int, size: size_t) -> Result> { - let mut size = size; +pub fn get_bytes(sock: *mut c_void, opt: c_int, mut size: size_t) -> Result> { let mut value = vec![0u8; size]; zmq_try!(unsafe { diff --git a/tests/message_from_boxed_slice.rs b/tests/message_from_boxed_slice.rs index a0e09938e..4754278bf 100644 --- a/tests/message_from_boxed_slice.rs +++ b/tests/message_from_boxed_slice.rs @@ -26,7 +26,7 @@ static A: Allocator = Allocator; #[test] fn message_from_boxed_slice() { let mut b: Box<[u8]> = Box::new([0u8; 42]); - CHECK_PTR.store(b.as_mut_ptr() as *mut u8, Ordering::SeqCst); + CHECK_PTR.store(b.as_mut_ptr(), Ordering::SeqCst); let _ = zmq::Message::from(b); assert_eq!(CHECK_PTR.load(Ordering::SeqCst), ptr::null_mut()); } diff --git a/tests/monitor.rs b/tests/monitor.rs index a227ef5e0..664d134a6 100644 --- a/tests/monitor.rs +++ b/tests/monitor.rs @@ -2,7 +2,6 @@ mod common; use std::str; -use std::u16; fn version_ge_4_3() -> bool { let (major, minor, _) = zmq::version(); diff --git a/tests/test.rs b/tests/test.rs index 57104dcd7..02a209d59 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -71,7 +71,7 @@ test!(test_exchanging_multipart, { let (sender, receiver) = create_socketpair(); // convenience API - sender.send_multipart(&["foo", "bar"], 0).unwrap(); + sender.send_multipart(["foo", "bar"], 0).unwrap(); assert_eq!(receiver.recv_multipart(0).unwrap(), vec![b"foo", b"bar"]); // manually diff --git a/tests/unix/connection.rs b/tests/unix/connection.rs index 9cf8aabe9..f6f2c3ef0 100644 --- a/tests/unix/connection.rs +++ b/tests/unix/connection.rs @@ -103,7 +103,7 @@ fn poll_worker(_ctx: &zmq::Context, socket: &zmq::Socket) { } Some(msg) => { state.wait(zmq::POLLOUT); - let done = msg.len() == 0; + let done = msg.is_empty(); socket.send(msg, zmq::DONTWAIT).unwrap(); if done { break; diff --git a/tests/z85.rs b/tests/z85.rs index 55048b175..6867faacf 100644 --- a/tests/z85.rs +++ b/tests/z85.rs @@ -33,14 +33,14 @@ fn test_decode_errors() { } } +/* +// Disabled because quickcheck doesn't expose gen_range and gen anymore + // Valid input for z85 encoding (i.e. a slice of bytes with its length // being a multiple of 4) #[derive(Clone, Debug)] struct Input(Vec); -/* -// Disabled because quickcheck doesn't expose gen_range and gen anymore - impl Arbitrary for Input { fn arbitrary(g: &mut Gen) -> Self { let len = g.gen_range(0..256) * 4; From c341b7e1ef53b94a41cd7fb344854f70c606897f Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Thu, 11 Jul 2024 22:10:12 +0200 Subject: [PATCH 2/7] implement `std::error::Error::source` for `EncodeError` and `DecodeError` --- src/lib.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e62e74456..a8fed1710 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1271,7 +1271,14 @@ impl fmt::Display for EncodeError { } } -impl std::error::Error for EncodeError {} +impl std::error::Error for EncodeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::BadLength => None, + Self::FromUtf8Error(err) => Some(err), + } + } +} /// Encode a binary key as Z85 printable text. /// @@ -1323,7 +1330,14 @@ impl fmt::Display for DecodeError { } } -impl std::error::Error for DecodeError {} +impl std::error::Error for DecodeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::BadLength => None, + Self::NulError(err) => Some(err), + } + } +} /// Decode a binary key from Z85-encoded text. /// From 4e9394ce368a6748a1da7739324879abad932f5d Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Thu, 11 Jul 2024 22:11:25 +0200 Subject: [PATCH 3/7] prevent possible overflow or crashes by dividing first, then multiplying --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a8fed1710..f0675a199 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1291,7 +1291,7 @@ pub fn z85_encode(data: &[u8]) -> result::Result { return Err(EncodeError::BadLength); } - let len = data.len() * 5 / 4 + 1; + let len = data.len() / 4 * 5 + 1; let mut dest = vec![0u8; len]; unsafe { @@ -1350,7 +1350,7 @@ pub fn z85_decode(data: &str) -> result::Result, DecodeError> { return Err(DecodeError::BadLength); } - let len = data.len() * 4 / 5; + let len = data.len() / 5 * 4; let mut dest = vec![0u8; len]; let c_str = ffi::CString::new(data)?; From bcfdeb1781297acb013eed0b205994f4111dac81 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Thu, 11 Jul 2024 22:13:00 +0200 Subject: [PATCH 4/7] implement standard library traits for `Message` - added PartialOrd, Ord, Clone, Default and Hash - moved PartialEq and Eq from between Deref and DerefMut --- src/message.rs | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/src/message.rs b/src/message.rs index 32b457bc1..a9a834db3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1,7 +1,9 @@ use libc::size_t; +use std::cmp::Ordering; use std::ffi; use std::fmt; +use std::hash::{Hash, Hasher}; use std::ops::{Deref, DerefMut}; use std::os::raw::c_void; use std::{ptr, slice, str}; @@ -157,6 +159,44 @@ impl Message { } } +impl PartialEq for Message { + fn eq(&self, other: &Message) -> bool { + self[..] == other[..] + } +} + +impl Eq for Message {} + +impl PartialOrd for Message { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Message { + fn cmp(&self, other: &Self) -> Ordering { + self[..].cmp(&other[..]) + } +} + +impl Clone for Message { + fn clone(&self) -> Self { + self[..].into() + } +} + +impl Default for Message { + fn default() -> Self { + Self::new() + } +} + +impl Hash for Message { + fn hash(&self, state: &mut H) { + self[..].hash(state); + } +} + impl Deref for Message { type Target = [u8]; @@ -172,14 +212,6 @@ impl Deref for Message { } } -impl PartialEq for Message { - fn eq(&self, other: &Message) -> bool { - self[..] == other[..] - } -} - -impl Eq for Message {} - impl DerefMut for Message { fn deref_mut(&mut self) -> &mut [u8] { // This is safe because we're constraining the slice to the lifetime of From 85c8ea1405f72aabb01c9ce40a9375ba40038bf2 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Thu, 11 Jul 2024 22:15:20 +0200 Subject: [PATCH 5/7] fix creation of mutable reference to uninitialized data (ub) caused by as_mut_ptr through DerefMut - add `as_ptr` and `as_mut_ptr`, these methods create no intermediate reference, previously these methods from `&mut [u8]` (or `&[u8]`) would be used, which is ub if the data is uninitialized. - use `ptr::slice_from_raw_parts_mut` when dropping a contained Box in a message to not create an intermediate reference to its contents. if this is ub, this has now been fixed. also, explicitly dropping here better expressed the goal of this function. - add methods to get the raw pointer of a Message, and get the length of the bytes without creating an intermediate reference. --- src/message.rs | 50 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/src/message.rs b/src/message.rs index a9a834db3..fbe27802d 100644 --- a/src/message.rs +++ b/src/message.rs @@ -40,7 +40,10 @@ impl fmt::Debug for Message { } unsafe extern "C" fn drop_msg_data_box(data: *mut c_void, hint: *mut c_void) { - let _ = Box::from_raw(slice::from_raw_parts_mut(data as *mut u8, hint as usize)); + drop(Box::from_raw(ptr::slice_from_raw_parts_mut( + data as *mut u8, + hint as usize, + ))); } impl Message { @@ -122,6 +125,38 @@ impl Message { Self::from(data) } + /// Returns a raw pointer to the message to be used with ffi calls. + pub fn as_message_ptr(&self) -> *const zmq_sys::zmq_msg_t { + &self.msg + } + + /// Returns a raw mutable pointer to the message to be used with ffi calls. + pub fn as_mut_message_ptr(&mut self) -> *mut zmq_sys::zmq_msg_t { + &mut self.msg + } + + /// Returns the amount of bytes that are stored in this `Message`. + pub fn len(&self) -> usize { + unsafe { zmq_sys::zmq_msg_size(self.as_message_ptr()) } + } + + /// Return `true` is there is at least one byte stored in this `Message`. + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Returns a raw pointer to the buffer of this message. + pub fn as_ptr(&self) -> *const u8 { + let ptr = unsafe { zmq_sys::zmq_msg_data(self.as_message_ptr().cast_mut()) }; + ptr as *const u8 + } + + /// Returns a raw mutable pointer to the buffer of this message. + pub fn as_mut_ptr(&mut self) -> *mut u8 { + let ptr = unsafe { zmq_sys::zmq_msg_data(self.as_mut_message_ptr()) }; + ptr as *mut u8 + } + /// Return the message content as a string slice if it is valid UTF-8. pub fn as_str(&self) -> Option<&str> { str::from_utf8(self).ok() @@ -203,12 +238,7 @@ impl Deref for Message { fn deref(&self) -> &[u8] { // This is safe because we're constraining the slice to the lifetime of // this message. - unsafe { - let ptr = &self.msg as *const _ as *mut _; - let data = zmq_sys::zmq_msg_data(ptr); - let len = zmq_sys::zmq_msg_size(ptr) as usize; - slice::from_raw_parts(data as *mut u8, len) - } + unsafe { slice::from_raw_parts(self.as_ptr(), self.len()) } } } @@ -216,11 +246,7 @@ impl DerefMut for Message { fn deref_mut(&mut self) -> &mut [u8] { // This is safe because we're constraining the slice to the lifetime of // this message. - unsafe { - let data = zmq_sys::zmq_msg_data(&mut self.msg); - let len = zmq_sys::zmq_msg_size(&self.msg) as usize; - slice::from_raw_parts_mut(data as *mut u8, len) - } + unsafe { slice::from_raw_parts_mut(self.as_mut_ptr(), self.len()) } } } From 0aae80faf93774967bc67e4388c7ad5a02c61fbe Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 17 Jul 2024 18:12:25 +0200 Subject: [PATCH 6/7] use const instead of static for DONTWAIT and SNDMORE --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f0675a199..2a4beb42c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,10 +149,10 @@ impl SocketEvent { } /// Flag for socket `send` methods that specifies non-blocking mode. -pub static DONTWAIT: i32 = zmq_sys::ZMQ_DONTWAIT as i32; +pub const DONTWAIT: i32 = zmq_sys::ZMQ_DONTWAIT as i32; /// Flag for socket `send` methods that specifies that more frames of a /// multipart message will follow. -pub static SNDMORE: i32 = zmq_sys::ZMQ_SNDMORE as i32; +pub const SNDMORE: i32 = zmq_sys::ZMQ_SNDMORE as i32; /// Security Mechanism #[allow(non_camel_case_types)] From c6566fad325dfaeb6eb8260bf7bd76656088133c Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 23 Sep 2024 17:34:06 +0200 Subject: [PATCH 7/7] fix documentation of Message::is_empty --- src/message.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/message.rs b/src/message.rs index fbe27802d..fcf2ec0a3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -140,7 +140,7 @@ impl Message { unsafe { zmq_sys::zmq_msg_size(self.as_message_ptr()) } } - /// Return `true` is there is at least one byte stored in this `Message`. + /// Return `true` if the `Message` stores no bytes. pub fn is_empty(&self) -> bool { self.len() == 0 }