Skip to content

Commit

Permalink
Minimise FFI in the public API
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tcharding committed Nov 8, 2022
1 parent 497654e commit 68c7385
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 29 deletions.
8 changes: 5 additions & 3 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 (<https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts>)
/// 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
/// (<https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts>). 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;
Expand Down
2 changes: 1 addition & 1 deletion src/ecdh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
12 changes: 7 additions & 5 deletions src/ecdsa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
}
}

Expand Down Expand Up @@ -315,7 +317,7 @@ impl<C: Signing> Secp256k1<C> {

counter += 1;
extra_entropy[..4].copy_from_slice(&counter.to_le_bytes());
entropy_p = extra_entropy.as_ptr().cast::<ffi::types::c_void>();
entropy_p = extra_entropy.as_c_ptr().cast::<ffi::types::c_void>();

// When fuzzing, these checks will usually spinloop forever, so just short-circuit them.
#[cfg(fuzzing)]
Expand Down
10 changes: 6 additions & 4 deletions src/ecdsa/recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
}
}

Expand Down
54 changes: 39 additions & 15 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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`].
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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");
Expand All @@ -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<ffi::PublicKey> for PublicKey {
#[inline]
fn from(pk: ffi::PublicKey) -> PublicKey {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand All @@ -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");
Expand Down Expand Up @@ -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
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/schnorr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ impl<C: Signing> Secp256k1<C> {
self.ctx,
sig.as_mut_c_ptr(),
msg.as_c_ptr(),
keypair.as_ptr(),
keypair.as_c_ptr(),
nonce_data,
)
);
Expand Down

0 comments on commit 68c7385

Please sign in to comment.