From 68c73850d88cd56f1ace2e00ea148a7063bde7a7 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 7 Nov 2022 12:09:57 +1100 Subject: [PATCH] Minimise FFI in the public API Normal users should never need to directly interact with the FFI layer. Audit and reduce the use of `ffi` types in the public API of various types. Leave only the implementation of `CPtr`, and document this clearly as not required by normal users. Done for: - PublicKey - XOnlyPublicKey - KeyPair - ecdsa::Signature - ecdsa::RecoverableSignature --- secp256k1-sys/src/lib.rs | 8 +++--- src/ecdh.rs | 2 +- src/ecdsa/mod.rs | 12 +++++---- src/ecdsa/recovery.rs | 10 +++++--- src/key.rs | 54 +++++++++++++++++++++++++++++----------- src/schnorr.rs | 2 +- 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index de13cfbc8..52f11b419 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -711,10 +711,12 @@ unsafe fn strlen(mut str_ptr: *const c_char) -> usize { } -/// A trait for producing pointers that will always be valid in C. (assuming NULL pointer is a valid no-op) -/// Rust doesn't promise what pointers does it give to ZST () -/// In case the type is empty this trait will give a NULL pointer, which should be handled in C. +/// A trait for producing pointers that will always be valid in C (assuming NULL pointer is a valid +/// no-op). /// +/// Rust does not guarantee pointers to Zero Sized Types +/// (). In case the type +/// is empty this trait will return a NULL pointer, which should be handled in C. pub trait CPtr { type Target; fn as_c_ptr(&self) -> *const Self::Target; diff --git a/src/ecdh.rs b/src/ecdh.rs index 70a5cbf4e..8215fa6f3 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -150,7 +150,7 @@ pub fn shared_secret_point(point: &PublicKey, scalar: &SecretKey) -> [u8; 64] { ffi::secp256k1_ecdh( ffi::secp256k1_context_no_precomp, xy.as_mut_ptr(), - point.as_ptr(), + point.as_c_ptr(), scalar.as_ptr(), Some(c_callback), ptr::null_mut(), diff --git a/src/ecdsa/mod.rs b/src/ecdsa/mod.rs index d38a4873d..f59c0dfc4 100644 --- a/src/ecdsa/mod.rs +++ b/src/ecdsa/mod.rs @@ -144,14 +144,16 @@ impl Signature { /// Obtains a raw pointer suitable for use with FFI functions #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")] pub fn as_ptr(&self) -> *const ffi::Signature { - &self.0 + self.as_c_ptr() } /// Obtains a raw mutable pointer suitable for use with FFI functions #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")] pub fn as_mut_ptr(&mut self) -> *mut ffi::Signature { - &mut self.0 + self.as_mut_c_ptr() } #[inline] @@ -199,11 +201,11 @@ impl CPtr for Signature { type Target = ffi::Signature; fn as_c_ptr(&self) -> *const Self::Target { - self.as_ptr() + &self.0 } fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - self.as_mut_ptr() + &mut self.0 } } @@ -315,7 +317,7 @@ impl Secp256k1 { counter += 1; extra_entropy[..4].copy_from_slice(&counter.to_le_bytes()); - entropy_p = extra_entropy.as_ptr().cast::(); + entropy_p = extra_entropy.as_c_ptr().cast::(); // When fuzzing, these checks will usually spinloop forever, so just short-circuit them. #[cfg(fuzzing)] diff --git a/src/ecdsa/recovery.rs b/src/ecdsa/recovery.rs index b8ac31492..a6c83e249 100644 --- a/src/ecdsa/recovery.rs +++ b/src/ecdsa/recovery.rs @@ -76,14 +76,16 @@ impl RecoverableSignature { /// Obtains a raw pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")] pub fn as_ptr(&self) -> *const ffi::RecoverableSignature { - &self.0 + self.as_c_ptr() } /// Obtains a raw mutable pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")] pub fn as_mut_ptr(&mut self) -> *mut ffi::RecoverableSignature { - &mut self.0 + self.as_mut_c_ptr() } #[inline] @@ -133,11 +135,11 @@ impl RecoverableSignature { impl CPtr for RecoverableSignature { type Target = ffi::RecoverableSignature; fn as_c_ptr(&self) -> *const Self::Target { - self.as_ptr() + &self.0 } fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - self.as_mut_ptr() + &mut self.0 } } diff --git a/src/key.rs b/src/key.rs index ef45f2d56..c178e60d8 100644 --- a/src/key.rs +++ b/src/key.rs @@ -221,7 +221,7 @@ impl SecretKey { let ret = ffi::secp256k1_keypair_sec( ffi::secp256k1_context_no_precomp, sk.as_mut_c_ptr(), - keypair.as_ptr() + keypair.as_c_ptr() ); debug_assert_eq!(ret, 1); } @@ -423,14 +423,16 @@ impl<'de> serde::Deserialize<'de> for SecretKey { impl PublicKey { /// Obtains a raw const pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")] pub fn as_ptr(&self) -> *const ffi::PublicKey { - &self.0 + self.as_c_ptr() } /// Obtains a raw mutable pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")] pub fn as_mut_ptr(&mut self) -> *mut ffi::PublicKey { - &mut self.0 + self.as_mut_c_ptr() } /// Creates a new public key from a [`SecretKey`]. @@ -507,7 +509,7 @@ impl PublicKey { let ret = ffi::secp256k1_keypair_pub( ffi::secp256k1_context_no_precomp, &mut pk, - keypair.as_ptr() + keypair.as_c_ptr() ); debug_assert_eq!(ret, 1); PublicKey(pk) @@ -733,7 +735,7 @@ impl PublicKey { ffi::secp256k1_context_no_precomp, &mut xonly_pk, &mut pk_parity, - self.as_ptr(), + self.as_c_ptr(), ); debug_assert_eq!(ret, 1); let parity = Parity::from_i32(pk_parity).expect("should not panic, pk_parity is 0 or 1"); @@ -743,19 +745,26 @@ impl PublicKey { } } +/// This trait enables interaction with the FFI layer and even though it is part of the public API +/// normal users should never need to directly interact with FFI types. impl CPtr for PublicKey { type Target = ffi::PublicKey; + + /// Obtains a const pointer suitable for use with FFI functions. fn as_c_ptr(&self) -> *const Self::Target { - self.as_ptr() + &self.0 } + /// Obtains a mutable pointer suitable for use with FFI functions. fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - self.as_mut_ptr() + &mut self.0 } } -/// Creates a new public key from a FFI public key +/// Creates a new public key from a FFI public key. +/// +/// Note, normal users should never need to interact directly with FFI types. impl From for PublicKey { #[inline] fn from(pk: ffi::PublicKey) -> PublicKey { @@ -845,14 +854,16 @@ impl_display_secret!(KeyPair); impl KeyPair { /// Obtains a raw const pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")] pub fn as_ptr(&self) -> *const ffi::KeyPair { - &self.0 + self.as_c_ptr() } /// Obtains a raw mutable pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")] pub fn as_mut_ptr(&mut self) -> *mut ffi::KeyPair { - &mut self.0 + self.as_mut_c_ptr() } /// Creates a [`KeyPair`] directly from a Secp256k1 secret key. @@ -1152,6 +1163,17 @@ impl<'de> serde::Deserialize<'de> for KeyPair { } } +impl CPtr for KeyPair { + type Target = ffi::KeyPair; + fn as_c_ptr(&self) -> *const Self::Target { + &self.0 + } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { + &mut self.0 + } +} + /// An x-only public key, used for verification of Schnorr signatures and serialized according to BIP-340. /// /// # Serde support @@ -1210,14 +1232,16 @@ impl str::FromStr for XOnlyPublicKey { impl XOnlyPublicKey { /// Obtains a raw const pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")] pub fn as_ptr(&self) -> *const ffi::XOnlyPublicKey { - &self.0 + self.as_c_ptr() } /// Obtains a raw mutable pointer suitable for use with FFI functions. #[inline] + #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")] pub fn as_mut_ptr(&mut self) -> *mut ffi::XOnlyPublicKey { - &mut self.0 + self.as_mut_c_ptr() } /// Returns the [`XOnlyPublicKey`] (and it's [`Parity`]) for `keypair`. @@ -1230,7 +1254,7 @@ impl XOnlyPublicKey { ffi::secp256k1_context_no_precomp, &mut xonly_pk, &mut pk_parity, - keypair.as_ptr(), + keypair.as_c_ptr(), ); debug_assert_eq!(ret, 1); let parity = Parity::from_i32(pk_parity).expect("should not panic, pk_parity is 0 or 1"); @@ -1570,11 +1594,11 @@ impl<'de> serde::Deserialize<'de> for Parity { impl CPtr for XOnlyPublicKey { type Target = ffi::XOnlyPublicKey; fn as_c_ptr(&self) -> *const Self::Target { - self.as_ptr() + &self.0 } fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - self.as_mut_ptr() + &mut self.0 } } diff --git a/src/schnorr.rs b/src/schnorr.rs index f5f602db2..0a677ea01 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -112,7 +112,7 @@ impl Secp256k1 { self.ctx, sig.as_mut_c_ptr(), msg.as_c_ptr(), - keypair.as_ptr(), + keypair.as_c_ptr(), nonce_data, ) );