From 2364c268a019ced1802692c25c409ff0cebf74a1 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 7 Dec 2023 16:09:54 -0800 Subject: [PATCH] ffi: Prefer std::ffi::* over libc::* for c_void, c_int, etc And use usize for size_t: - They're always equivalent in practice. - When we're actually using it as a memory size, we're talking about the size of Rust objects, so usize is more accurate anyway. This eliminates the use of the libc crate in the bridge layer. We still use libc for time_t in attest and device_transfer, to interact with BoringSSL. --- Cargo.lock | 2 - rust/bridge/ffi/Cargo.toml | 1 - rust/bridge/ffi/src/lib.rs | 5 +-- rust/bridge/ffi/src/logging.rs | 3 +- rust/bridge/shared/Cargo.toml | 3 +- rust/bridge/shared/macros/src/ffi.rs | 2 +- rust/bridge/shared/src/ffi/convert.rs | 53 ++++++++++++------------- rust/bridge/shared/src/ffi/futures.rs | 8 ++-- rust/bridge/shared/src/ffi/io.rs | 2 +- rust/bridge/shared/src/ffi/storage.rs | 4 +- rust/bridge/shared/src/testing/types.rs | 12 +++--- swift/Sources/SignalFfi/signal_ffi.h | 2 +- 12 files changed, 46 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1aee4e85..7e029880 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1591,7 +1591,6 @@ dependencies = [ "hkdf", "hmac", "jni", - "libc", "libsignal-bridge-macros", "libsignal-net", "libsignal-protocol", @@ -1639,7 +1638,6 @@ dependencies = [ "cpufeatures", "device-transfer", "futures-util", - "libc", "libsignal-bridge", "libsignal-protocol", "log", diff --git a/rust/bridge/ffi/Cargo.toml b/rust/bridge/ffi/Cargo.toml index ef825b7f..df565757 100644 --- a/rust/bridge/ffi/Cargo.toml +++ b/rust/bridge/ffi/Cargo.toml @@ -32,7 +32,6 @@ libsignal-bridge = { path = "../shared", features = ["ffi"] } async-trait = "0.1.41" cpufeatures = "0.2.1" # Make sure iOS gets optimized crypto. futures-util = "0.3" -libc = "0.2" rand = "0.8" log = { version = "0.4", features = ["release_max_level_info"] } log-panics = { version = "2.1.0", features = ["with-backtrace"] } diff --git a/rust/bridge/ffi/src/lib.rs b/rust/bridge/ffi/src/lib.rs index 47765700..dfb601e0 100644 --- a/rust/bridge/ffi/src/lib.rs +++ b/rust/bridge/ffi/src/lib.rs @@ -7,11 +7,10 @@ #![warn(clippy::unwrap_used)] use futures_util::FutureExt; -use libc::{c_char, c_uchar, c_uint, size_t}; use libsignal_bridge::ffi::*; use libsignal_protocol::*; -use std::ffi::CString; +use std::ffi::{c_char, c_uchar, c_uint, CString}; use std::panic::AssertUnwindSafe; pub mod logging; @@ -33,7 +32,7 @@ pub unsafe extern "C" fn signal_free_string(buf: *const c_char) { } #[no_mangle] -pub unsafe extern "C" fn signal_free_buffer(buf: *const c_uchar, buf_len: size_t) { +pub unsafe extern "C" fn signal_free_buffer(buf: *const c_uchar, buf_len: usize) { if buf.is_null() { return; } diff --git a/rust/bridge/ffi/src/logging.rs b/rust/bridge/ffi/src/logging.rs index 6d056553..81969d49 100644 --- a/rust/bridge/ffi/src/logging.rs +++ b/rust/bridge/ffi/src/logging.rs @@ -3,8 +3,7 @@ // SPDX-License-Identifier: AGPL-3.0-only // -use libc::c_char; -use std::ffi::CString; +use std::ffi::{c_char, CString}; #[repr(C)] pub enum LogLevel { diff --git a/rust/bridge/shared/Cargo.toml b/rust/bridge/shared/Cargo.toml index a72a848f..924e7322 100644 --- a/rust/bridge/shared/Cargo.toml +++ b/rust/bridge/shared/Cargo.toml @@ -43,7 +43,6 @@ uuid = "1.1.2" subtle = { version = "2.5", features = ["core_hint_black_box"] } bytemuck = { version = "1.13.0", optional = true } -libc = { version = "0.2", optional = true } jni = { version = "0.21", package = "jni", optional = true } neon = { version = "0.10.0", optional = true, default-features = false, features = ["napi-6", "promise-api"] } linkme = { version = "0.3.9", optional = true } @@ -52,6 +51,6 @@ num_enum = "0.6.1" nonzero_ext = "0.3.0" [features] -ffi = ["libc"] +ffi = [] jni = ["dep:jni", "bytemuck"] node = ["neon", "linkme", "signal-neon-futures"] diff --git a/rust/bridge/shared/macros/src/ffi.rs b/rust/bridge/shared/macros/src/ffi.rs index c05aa52a..66a60ad0 100644 --- a/rust/bridge/shared/macros/src/ffi.rs +++ b/rust/bridge/shared/macros/src/ffi.rs @@ -46,7 +46,7 @@ pub(crate) fn bridge_fn( let output = result_type(&sig.output); quote!( promise: ffi::CPromise, - promise_context: *const libc::c_void, + promise_context: *const std::ffi::c_void, async_runtime: ffi_arg_type!(&#runtime), // note the trailing comma ) } diff --git a/rust/bridge/shared/src/ffi/convert.rs b/rust/bridge/shared/src/ffi/convert.rs index 357e0fef..e04b2a10 100644 --- a/rust/bridge/shared/src/ffi/convert.rs +++ b/rust/bridge/shared/src/ffi/convert.rs @@ -3,11 +3,10 @@ // SPDX-License-Identifier: AGPL-3.0-only // -use libc::{c_char, c_uchar}; use libsignal_protocol::*; use paste::paste; -use std::ffi::CStr; +use std::ffi::{c_char, c_uchar, CStr}; use std::ops::Deref; use crate::io::{InputStream, SyncInputStream}; @@ -41,7 +40,7 @@ use super::*; /// /// Implementers should also see the `ffi_arg_type` macro in `convert.rs`. pub trait ArgTypeInfo<'storage>: Sized { - /// The FFI form of the argument (e.g. `libc::c_uchar`). + /// The FFI form of the argument (e.g. `std::ffi::c_uchar`). type ArgType; /// Local storage for the argument (ideally borrowed rather than copied). type StoredType: 'storage; @@ -75,7 +74,7 @@ pub trait ArgTypeInfo<'storage>: Sized { /// /// However, some types do need the full flexibility of `ArgTypeInfo`. pub trait SimpleArgTypeInfo: Sized { - /// The FFI form of the argument (e.g. `libc::c_uchar`). + /// The FFI form of the argument (e.g. `std::ffi::c_uchar`). type ArgType; /// Converts the data in `foreign` to the Rust type. fn convert_from(foreign: Self::ArgType) -> SignalFfiResult; @@ -115,7 +114,7 @@ where /// /// Implementers should also see the `ffi_result_type` macro in `convert.rs`. pub trait ResultTypeInfo: Sized { - /// The FFI form of the result (e.g. `libc::c_uchar`). + /// The FFI form of the result (e.g. `std::ffi::c_uchar`). type ResultType; /// Converts the data in `self` to the FFI type, similar to `try_into()`. fn convert_into(self) -> SignalFfiResult; @@ -321,7 +320,7 @@ where /// Allocates and returns a new Rust-owned C string. impl ResultTypeInfo for String { - type ResultType = *const libc::c_char; + type ResultType = *const std::ffi::c_char; fn convert_into(self) -> SignalFfiResult { self.deref().convert_into() } @@ -329,7 +328,7 @@ impl ResultTypeInfo for String { /// Allocates and returns a new Rust-owned C string (or `NULL`). impl ResultTypeInfo for Option { - type ResultType = *const libc::c_char; + type ResultType = *const std::ffi::c_char; fn convert_into(self) -> SignalFfiResult { self.as_deref().convert_into() } @@ -337,7 +336,7 @@ impl ResultTypeInfo for Option { /// Allocates and returns a new Rust-owned C string. impl ResultTypeInfo for &str { - type ResultType = *const libc::c_char; + type ResultType = *const std::ffi::c_char; fn convert_into(self) -> SignalFfiResult { let cstr = CString::new(self).expect("No NULL characters in string being returned to C"); Ok(cstr.into_raw()) @@ -346,7 +345,7 @@ impl ResultTypeInfo for &str { /// Allocates and returns a new Rust-owned C string (or `NULL`). impl ResultTypeInfo for Option<&str> { - type ResultType = *const libc::c_char; + type ResultType = *const std::ffi::c_char; fn convert_into(self) -> SignalFfiResult { match self { Some(s) => s.convert_into(), @@ -356,14 +355,14 @@ impl ResultTypeInfo for Option<&str> { } impl ResultTypeInfo for Vec { - type ResultType = OwnedBufferOf; + type ResultType = OwnedBufferOf; fn convert_into(self) -> SignalFfiResult { Ok(OwnedBufferOf::from(self.into_boxed_slice())) } } impl ResultTypeInfo for &[u8] { - type ResultType = OwnedBufferOf; + type ResultType = OwnedBufferOf; fn convert_into(self) -> SignalFfiResult { self.to_vec().convert_into() } @@ -586,13 +585,13 @@ macro_rules! ffi_arg_type { (u32) => (u32); (u64) => (u64); (Option) => (u32); - (usize) => (libc::size_t); + (usize) => (usize); (bool) => (bool); - (&[u8]) => (ffi::BorrowedSliceOf); - (&mut [u8]) => (ffi::BorrowedMutableSliceOf); - (String) => (*const libc::c_char); - (Option) => (*const libc::c_char); - (Option<&str>) => (*const libc::c_char); + (&[u8]) => (ffi::BorrowedSliceOf); + (&mut [u8]) => (ffi::BorrowedMutableSliceOf); + (String) => (*const std::ffi::c_char); + (Option) => (*const std::ffi::c_char); + (Option<&str>) => (*const std::ffi::c_char); (Timestamp) => (u64); (Uuid) => (*const [u8; 16]); (ServiceId) => (*const libsignal_protocol::ServiceIdFixedWidthBinaryBytes); @@ -605,11 +604,11 @@ macro_rules! ffi_arg_type { (&mut $typ:ty) => (*mut $typ); (Option<& $typ:ty>) => (*const $typ); - (Ignored<$typ:ty>) => (*const libc::c_void); + (Ignored<$typ:ty>) => (*const std::ffi::c_void); // In order to provide a fixed-sized array of the correct length, // a serialized type FooBar must have a constant FOO_BAR_LEN that's in scope (and exposed to C). - (Serialized<$typ:ident>) => (*const [libc::c_uchar; paste!([<$typ:snake:upper _LEN>])]); + (Serialized<$typ:ident>) => (*const [std::ffi::c_uchar; paste!([<$typ:snake:upper _LEN>])]); } /// Syntactically translates `bridge_fn` result types to FFI types for `cbindgen`. @@ -635,10 +634,10 @@ macro_rules! ffi_result_type { (Option) => (u32); (u64) => (u64); (bool) => (bool); - (&str) => (*const libc::c_char); - (String) => (*const libc::c_char); - (Option) => (*const libc::c_char); - (Option<&str>) => (*const libc::c_char); + (&str) => (*const std::ffi::c_char); + (String) => (*const std::ffi::c_char); + (Option) => (*const std::ffi::c_char); + (Option<&str>) => (*const std::ffi::c_char); (Option<$typ:ty>) => (*mut $typ); (Timestamp) => (u64); (Uuid) => ([u8; 16]); @@ -646,14 +645,14 @@ macro_rules! ffi_result_type { (Aci) => (libsignal_protocol::ServiceIdFixedWidthBinaryBytes); (Pni) => (libsignal_protocol::ServiceIdFixedWidthBinaryBytes); ([u8; $len:expr]) => ([u8; $len]); - (&[u8]) => (ffi::OwnedBufferOf); - (Vec) => (ffi::OwnedBufferOf); + (&[u8]) => (ffi::OwnedBufferOf); + (Vec) => (ffi::OwnedBufferOf); // In order to provide a fixed-sized array of the correct length, // a serialized type FooBar must have a constant FOO_BAR_LEN that's in scope (and exposed to C). - (Serialized<$typ:ident>) => ([libc::c_uchar; paste!([<$typ:snake:upper _LEN>])]); + (Serialized<$typ:ident>) => ([std::ffi::c_uchar; paste!([<$typ:snake:upper _LEN>])]); - (Ignored<$typ:ty>) => (*const libc::c_void); + (Ignored<$typ:ty>) => (*const std::ffi::c_void); ( $typ:ty ) => (*mut $typ); } diff --git a/rust/bridge/shared/src/ffi/futures.rs b/rust/bridge/shared/src/ffi/futures.rs index bb3bfbb9..5a7a54ef 100644 --- a/rust/bridge/shared/src/ffi/futures.rs +++ b/rust/bridge/shared/src/ffi/futures.rs @@ -15,7 +15,7 @@ use std::future::Future; /// cbindgen will produce independent C types like `SignalCPromisei32` and /// `SignalCPromiseProtocolAddress`. pub type CPromise = - extern "C" fn(error: *mut SignalFfiError, result: *const T, context: *const libc::c_void); + extern "C" fn(error: *mut SignalFfiError, result: *const T, context: *const std::ffi::c_void); /// Keeps track of the information necessary to report a promise result back to C. /// @@ -24,7 +24,7 @@ pub type CPromise = /// will result in a panic in debug mode and an error log in release mode. pub struct PromiseCompleter { promise: CPromise, - promise_context: *const libc::c_void, + promise_context: *const std::ffi::c_void, } /// Pointers are not Send by default just in case, @@ -99,7 +99,7 @@ impl ResultReporter for FutureResult /// # where F::Output: ResultReporter { /// # fn run_future(&self, future: F, receiver: ::Receiver) { unimplemented!() } /// # } -/// # fn test(promise: CPromise, promise_context: *const libc::c_void, async_runtime: &ExampleAsyncRuntime) { +/// # fn test(promise: CPromise, promise_context: *const std::ffi::c_void, async_runtime: &ExampleAsyncRuntime) { /// run_future_on_runtime(async_runtime, promise, promise_context, async { /// let result: i32 = 1 + 2; /// // Do some complicated awaiting here. @@ -110,7 +110,7 @@ impl ResultReporter for FutureResult pub fn run_future_on_runtime( runtime: &impl AsyncRuntime, promise: CPromise, - promise_context: *const libc::c_void, + promise_context: *const std::ffi::c_void, future: F, ) where F: Future + std::panic::UnwindSafe + 'static, diff --git a/rust/bridge/shared/src/ffi/io.rs b/rust/bridge/shared/src/ffi/io.rs index cd44433c..497d96b4 100644 --- a/rust/bridge/shared/src/ffi/io.rs +++ b/rust/bridge/shared/src/ffi/io.rs @@ -3,10 +3,10 @@ // SPDX-License-Identifier: AGPL-3.0-only // +use std::ffi::{c_int, c_void}; use std::io; use async_trait::async_trait; -use libc::{c_int, c_void}; use crate::io::{InputStream, InputStreamRead, SyncInputStream}; diff --git a/rust/bridge/shared/src/ffi/storage.rs b/rust/bridge/shared/src/ffi/storage.rs index 1566b0c6..8ff07d61 100644 --- a/rust/bridge/shared/src/ffi/storage.rs +++ b/rust/bridge/shared/src/ffi/storage.rs @@ -4,10 +4,12 @@ // use super::*; + use async_trait::async_trait; -use libc::{c_int, c_uint, c_void}; use uuid::Uuid; +use std::ffi::{c_int, c_uint, c_void}; + type GetIdentityKeyPair = extern "C" fn(store_ctx: *mut c_void, keyp: *mut *mut PrivateKey) -> c_int; type GetLocalRegistrationId = extern "C" fn(store_ctx: *mut c_void, idp: *mut u32) -> c_int; diff --git a/rust/bridge/shared/src/testing/types.rs b/rust/bridge/shared/src/testing/types.rs index 470547a5..49d8705d 100644 --- a/rust/bridge/shared/src/testing/types.rs +++ b/rust/bridge/shared/src/testing/types.rs @@ -53,7 +53,7 @@ impl node::Finalize for NeedsCleanup { #[cfg(feature = "ffi")] impl ffi::SimpleArgTypeInfo for NeedsCleanup { - type ArgType = *const libc::c_void; + type ArgType = *const std::ffi::c_void; fn convert_from(_foreign: Self::ArgType) -> ffi::SignalFfiResult { // The plain FFI bridge does not have any context or environment it's executed in, @@ -126,7 +126,7 @@ pub struct ErrorOnBorrow; #[cfg(feature = "ffi")] impl ffi::SimpleArgTypeInfo for ErrorOnBorrow { - type ArgType = *const libc::c_void; + type ArgType = *const std::ffi::c_void; fn convert_from(_foreign: Self::ArgType) -> ffi::SignalFfiResult { Err(SignalProtocolError::InvalidArgument("deliberate error".to_string()).into()) @@ -163,7 +163,7 @@ pub struct PanicOnBorrow; #[cfg(feature = "ffi")] impl ffi::SimpleArgTypeInfo for PanicOnBorrow { - type ArgType = *const libc::c_void; + type ArgType = *const std::ffi::c_void; fn convert_from(_foreign: Self::ArgType) -> ffi::SignalFfiResult { panic!("deliberate panic") @@ -203,7 +203,7 @@ pub struct PanicOnLoad; #[cfg(feature = "ffi")] impl<'storage> ffi::ArgTypeInfo<'storage> for PanicOnLoad { - type ArgType = *const libc::c_void; + type ArgType = *const std::ffi::c_void; type StoredType = (); @@ -276,7 +276,7 @@ pub struct ErrorOnReturn; #[cfg(feature = "ffi")] impl ffi::ResultTypeInfo for ErrorOnReturn { - type ResultType = *const libc::c_void; + type ResultType = *const std::ffi::c_void; fn convert_into(self) -> ffi::SignalFfiResult { Err(SignalProtocolError::InvalidArgument("deliberate error".to_string()).into()) @@ -306,7 +306,7 @@ pub struct PanicOnReturn; #[cfg(feature = "ffi")] impl ffi::ResultTypeInfo for PanicOnReturn { - type ResultType = *const libc::c_void; + type ResultType = *const std::ffi::c_void; fn convert_into(self) -> ffi::SignalFfiResult { panic!("deliberate panic"); diff --git a/swift/Sources/SignalFfi/signal_ffi.h b/swift/Sources/SignalFfi/signal_ffi.h index 6f2bf572..3d2c1bf0 100644 --- a/swift/Sources/SignalFfi/signal_ffi.h +++ b/swift/Sources/SignalFfi/signal_ffi.h @@ -450,7 +450,7 @@ void signal_print_ptr(const void *p); void signal_free_string(const char *buf); -void signal_free_buffer(const unsigned char *buf, size_t buf_len); +void signal_free_buffer(const unsigned char *buf, uintptr_t buf_len); SignalFfiError *signal_error_get_message(const SignalFfiError *err, const char **out);