Skip to content

Commit

Permalink
Use NonNull for Secp256k1 inner context field
Browse files Browse the repository at this point in the history
For raw pointers that can never be null Rust provides the
`core::ptr::NonNull` type. Our `Secp256k1` type has an inner field that
is a non-null pointer; use `NonNull` for it.

Fix: #534
  • Loading branch information
tcharding committed Nov 30, 2022
1 parent 3760ef6 commit 082c3bd
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 40 deletions.
24 changes: 16 additions & 8 deletions src/context.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use core::marker::PhantomData;
use core::mem::ManuallyDrop;
use core::ptr::NonNull;

#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
Expand Down Expand Up @@ -116,6 +117,7 @@ mod private {
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
mod alloc_only {
use core::marker::PhantomData;
use core::ptr::NonNull;

use super::private;
use crate::alloc::alloc;
Expand Down Expand Up @@ -209,7 +211,10 @@ mod alloc_only {
#[allow(unused_mut)] // ctx is not mutated under some feature combinations.
let mut ctx = Secp256k1 {
ctx: unsafe {
ffi::secp256k1_context_preallocated_create(ptr as *mut c_void, C::FLAGS)
NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create(

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

Looks like this could've been let ptr = NonNull::new(ptr).unwrap_or_else(|| handle_alloc_error(layout)); above. Assuming secp256k1_context_preallocated_create returns the same ptr.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 30, 2022

Member

Oh, neat, it does return the same pointer. I didn't realize that it returned anything at all.

We do need to call secp256k1_context_preallocated_create though to initialize things ... I'm not sure in your suggestion where we do that.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

ffi::secp256k1_context_preallocated_create(ptr.as_ptr()) after creating ptr = NonNull...

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 30, 2022

Member

Oh, nice! So then we're always using .as_ptr before handing off over FFI, and we're always using NonNull::new rather than new_unchecked (I assume we can do this in all the context_crate places).

I should just hold off on merging stuff for a few days until you've had a chance to chime in :)

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

Actually, this is less bad than constant rebases 😆

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 30, 2022

Member

Ok, trying to fix this it's actually a bit annoying... secp256k1_context_preallocated_create does just pass the pointer through, but it also serves to "cast" it from a *mut u8 to a *mut ffi::Context. Neither the input nor output is a NonNull.

On the input side it's fine to do ptr.as_ptr() (or rather ptr.as_mut()) as you suggest -- but then on the output side I've gotta go through the NonNull::new_unchecked dance again.

Now, I could change the FFI signature of ffi::secp256k1_context_preallocated_size to take and return a NonNull, since it looks like the C code will indeed exhibit UB if you pass it a NULL pointer[1], but this would be a semver-breaking change in secp-sys.

Lemme promote this to a real issue.

[1] If you compile with -DVERIFY it'll assert. But we don't.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 1, 2022

Collaborator

Can we cast it using ptr.cast()?

This comment has been minimized.

Copy link
@apoelstra

apoelstra Dec 1, 2022

Member

Ah, yep, that'll work. (I'm a little surprised that NonNull::cast exists, but I guess it's a fairly common need.) It's a tiny bit ugly but lets us do this simple cleanup without needing to mess with the FFI.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 1, 2022

Collaborator

cast is also available on *mut T so I guess it's for consistency. It's handy when you explicitly do not want to change mutability.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Dec 1, 2022

Member

That's awesome, TIL

ptr as *mut c_void,
C::FLAGS,
))
},
phantom: PhantomData,
};
Expand Down Expand Up @@ -261,15 +266,18 @@ mod alloc_only {

impl<C: Context> Clone for Secp256k1<C> {
fn clone(&self) -> Secp256k1<C> {
let size = unsafe { ffi::secp256k1_context_preallocated_clone_size(self.ctx as _) };
let size = unsafe { ffi::secp256k1_context_preallocated_clone_size(self.ctx.as_ptr()) };
let layout = alloc::Layout::from_size_align(size, ALIGN_TO).unwrap();
let ptr = unsafe { alloc::alloc(layout) };
if ptr.is_null() {
alloc::handle_alloc_error(layout);
}
Secp256k1 {
ctx: unsafe {
ffi::secp256k1_context_preallocated_clone(self.ctx, ptr as *mut c_void)
NonNull::new_unchecked(ffi::secp256k1_context_preallocated_clone(
self.ctx.as_ptr(),
ptr as *mut c_void,
))
},
phantom: PhantomData,
}
Expand Down Expand Up @@ -321,10 +329,10 @@ impl<'buf, C: Context + 'buf> Secp256k1<C> {
}
Ok(Secp256k1 {
ctx: unsafe {
ffi::secp256k1_context_preallocated_create(
NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create(

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

FTR noticed #543 when reviewing this one.

buf.as_mut_c_ptr() as *mut c_void,
C::FLAGS,
)
))
},
phantom: PhantomData,
})
Expand Down Expand Up @@ -355,7 +363,7 @@ impl<'buf> Secp256k1<AllPreallocated<'buf>> {
pub unsafe fn from_raw_all(
raw_ctx: *mut ffi::Context,
) -> ManuallyDrop<Secp256k1<AllPreallocated<'buf>>> {
ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData })
ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData })
}
}

Expand Down Expand Up @@ -386,7 +394,7 @@ impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
pub unsafe fn from_raw_signing_only(
raw_ctx: *mut ffi::Context,
) -> ManuallyDrop<Secp256k1<SignOnlyPreallocated<'buf>>> {
ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData })
ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData })
}
}

Expand Down Expand Up @@ -417,6 +425,6 @@ impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
pub unsafe fn from_raw_verification_only(

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

OT: I wanted to complain about list being not exhaustive because it makes the fn useless. If it's truly not exhaustive the function should be just removed from the API.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 30, 2022

Member

I think these functions are not long for this world anyway. Upstream in bitcoin-core/secp256k1#1126 they are getting rid of the notion of signing/verification contexts, and we are mirroring that change in #538.

raw_ctx: *mut ffi::Context,
) -> ManuallyDrop<Secp256k1<VerifyOnlyPreallocated<'buf>>> {
ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData })
ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData })

This comment has been minimized.

Copy link
@Kixunil

Kixunil Nov 30, 2022

Collaborator

Maybe it'd make sense to push this into API? It'll be API-breaking but I think this library will have a few breaking changes anyway.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 30, 2022

Member

Yeah, I think we should definitely move it into the Rust API ... and maybe into the ffi API (see #546)

}
}
12 changes: 8 additions & 4 deletions src/ecdsa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ impl<C: Signing> Secp256k1<C> {
// an invalid signature from a valid `Message` and `SecretKey`
assert_eq!(
ffi::secp256k1_ecdsa_sign(
self.ctx,
self.ctx.as_ptr(),
&mut ret,
msg.as_c_ptr(),
sk.as_c_ptr(),
Expand Down Expand Up @@ -307,7 +307,7 @@ impl<C: Signing> Secp256k1<C> {
// an invalid signature from a valid `Message` and `SecretKey`
assert_eq!(
ffi::secp256k1_ecdsa_sign(
self.ctx,
self.ctx.as_ptr(),
&mut ret,
msg.as_c_ptr(),
sk.as_c_ptr(),
Expand Down Expand Up @@ -388,8 +388,12 @@ impl<C: Verification> Secp256k1<C> {
pk: &PublicKey,
) -> Result<(), Error> {
unsafe {
if ffi::secp256k1_ecdsa_verify(self.ctx, sig.as_c_ptr(), msg.as_c_ptr(), pk.as_c_ptr())
== 0
if ffi::secp256k1_ecdsa_verify(
self.ctx.as_ptr(),
sig.as_c_ptr(),
msg.as_c_ptr(),
pk.as_c_ptr(),
) == 0
{
Err(Error::IncorrectSignature)
} else {
Expand Down
9 changes: 7 additions & 2 deletions src/ecdsa/recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl<C: Signing> Secp256k1<C> {
// an invalid signature from a valid `Message` and `SecretKey`
assert_eq!(
ffi::secp256k1_ecdsa_sign_recoverable(
self.ctx,
self.ctx.as_ptr(),
&mut ret,
msg.as_c_ptr(),
sk.as_c_ptr(),
Expand Down Expand Up @@ -208,7 +208,12 @@ impl<C: Verification> Secp256k1<C> {
) -> Result<key::PublicKey, Error> {
unsafe {
let mut pk = super_ffi::PublicKey::new();
if ffi::secp256k1_ecdsa_recover(self.ctx, &mut pk, sig.as_c_ptr(), msg.as_c_ptr()) != 1
if ffi::secp256k1_ecdsa_recover(
self.ctx.as_ptr(),
&mut pk,
sig.as_c_ptr(),
msg.as_c_ptr(),
) != 1
{
return Err(Error::InvalidSignature);
}
Expand Down
33 changes: 21 additions & 12 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ impl PublicKey {
let mut pk = ffi::PublicKey::new();
// We can assume the return value because it's not possible to construct
// an invalid `SecretKey` without transmute trickery or something.
let res = ffi::secp256k1_ec_pubkey_create(secp.ctx, &mut pk, sk.as_c_ptr());
let res = ffi::secp256k1_ec_pubkey_create(secp.ctx.as_ptr(), &mut pk, sk.as_c_ptr());
debug_assert_eq!(res, 1);
PublicKey(pk)
}
Expand Down Expand Up @@ -575,7 +575,7 @@ impl PublicKey {
#[must_use = "you forgot to use the negated public key"]
pub fn negate<C: Verification>(mut self, secp: &Secp256k1<C>) -> PublicKey {
unsafe {
let res = ffi::secp256k1_ec_pubkey_negate(secp.ctx, &mut self.0);
let res = ffi::secp256k1_ec_pubkey_negate(secp.ctx.as_ptr(), &mut self.0);
debug_assert_eq!(res, 1);
}
self
Expand All @@ -593,7 +593,9 @@ impl PublicKey {
tweak: &Scalar,
) -> Result<PublicKey, Error> {
unsafe {
if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0, tweak.as_c_ptr()) == 1 {
if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx.as_ptr(), &mut self.0, tweak.as_c_ptr())
== 1
{
Ok(self)
} else {
Err(Error::InvalidTweak)
Expand All @@ -613,7 +615,9 @@ impl PublicKey {
other: &Scalar,
) -> Result<PublicKey, Error> {
unsafe {
if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx, &mut self.0, other.as_c_ptr()) == 1 {
if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx.as_ptr(), &mut self.0, other.as_c_ptr())
== 1
{
Ok(self)
} else {
Err(Error::InvalidTweak)
Expand Down Expand Up @@ -817,7 +821,7 @@ impl KeyPair {
pub fn from_secret_key<C: Signing>(secp: &Secp256k1<C>, sk: &SecretKey) -> KeyPair {
unsafe {
let mut kp = ffi::KeyPair::new();
if ffi::secp256k1_keypair_create(secp.ctx, &mut kp, sk.as_c_ptr()) == 1 {
if ffi::secp256k1_keypair_create(secp.ctx.as_ptr(), &mut kp, sk.as_c_ptr()) == 1 {
KeyPair(kp)
} else {
panic!("the provided secret key is invalid: it is corrupted or was not produced by Secp256k1 library")
Expand All @@ -842,7 +846,7 @@ impl KeyPair {

unsafe {
let mut kp = ffi::KeyPair::new();
if ffi::secp256k1_keypair_create(secp.ctx, &mut kp, data.as_c_ptr()) == 1 {
if ffi::secp256k1_keypair_create(secp.ctx.as_ptr(), &mut kp, data.as_c_ptr()) == 1 {
Ok(KeyPair(kp))
} else {
Err(Error::InvalidSecretKey)
Expand Down Expand Up @@ -895,7 +899,9 @@ impl KeyPair {
let mut data = crate::random_32_bytes(rng);
unsafe {
let mut keypair = ffi::KeyPair::new();
while ffi::secp256k1_keypair_create(secp.ctx, &mut keypair, data.as_c_ptr()) == 0 {
while ffi::secp256k1_keypair_create(secp.ctx.as_ptr(), &mut keypair, data.as_c_ptr())
== 0
{
data = crate::random_32_bytes(rng);
}
KeyPair(keypair)
Expand Down Expand Up @@ -946,8 +952,11 @@ impl KeyPair {
tweak: &Scalar,
) -> Result<KeyPair, Error> {
unsafe {
let err =
ffi::secp256k1_keypair_xonly_tweak_add(secp.ctx, &mut self.0, tweak.as_c_ptr());
let err = ffi::secp256k1_keypair_xonly_tweak_add(
secp.ctx.as_ptr(),
&mut self.0,
tweak.as_c_ptr(),
);
if err != 1 {
return Err(Error::InvalidTweak);
}
Expand Down Expand Up @@ -1243,7 +1252,7 @@ impl XOnlyPublicKey {
unsafe {
let mut pubkey = ffi::PublicKey::new();
let mut err = ffi::secp256k1_xonly_pubkey_tweak_add(
secp.ctx,
secp.ctx.as_ptr(),
&mut pubkey,
self.as_c_ptr(),
tweak.as_c_ptr(),
Expand All @@ -1253,7 +1262,7 @@ impl XOnlyPublicKey {
}

err = ffi::secp256k1_xonly_pubkey_from_pubkey(
secp.ctx,
secp.ctx.as_ptr(),
&mut self.0,
&mut pk_parity,
&pubkey,
Expand Down Expand Up @@ -1306,7 +1315,7 @@ impl XOnlyPublicKey {
let tweaked_ser = tweaked_key.serialize();
unsafe {
let err = ffi::secp256k1_xonly_pubkey_tweak_add_check(
secp.ctx,
secp.ctx.as_ptr(),
tweaked_ser.as_c_ptr(),
tweaked_parity.to_i32(),
&self.0,
Expand Down
27 changes: 15 additions & 12 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ pub mod schnorr;
mod serde_util;

use core::marker::PhantomData;
use core::ptr::NonNull;
use core::{fmt, mem, str};

#[cfg(feature = "bitcoin-hashes")]
Expand Down Expand Up @@ -369,7 +370,7 @@ impl std::error::Error for Error {

/// The secp256k1 engine, used to execute all signature operations.
pub struct Secp256k1<C: Context> {
ctx: *mut ffi::Context,
ctx: NonNull<ffi::Context>,
phantom: PhantomData<C>,
}

Expand All @@ -387,9 +388,10 @@ impl<C: Context> Eq for Secp256k1<C> {}
impl<C: Context> Drop for Secp256k1<C> {
fn drop(&mut self) {
unsafe {
let size = ffi::secp256k1_context_preallocated_clone_size(self.ctx);
ffi::secp256k1_context_preallocated_destroy(self.ctx);
C::deallocate(self.ctx as _, size);
let size = ffi::secp256k1_context_preallocated_clone_size(self.ctx.as_ptr());
ffi::secp256k1_context_preallocated_destroy(self.ctx.as_ptr());

C::deallocate(self.ctx.as_ptr() as _, size);
}
}
}
Expand All @@ -405,7 +407,7 @@ impl<C: Context> Secp256k1<C> {
/// shouldn't be needed with normal usage of the library. It enables
/// extending the Secp256k1 with more cryptographic algorithms outside of
/// this crate.
pub fn ctx(&self) -> &*mut ffi::Context { &self.ctx }
pub fn ctx(&self) -> NonNull<ffi::Context> { self.ctx }

/// Returns the required memory for a preallocated context buffer in a generic manner(sign/verify/all).
pub fn preallocate_size_gen() -> usize {
Expand All @@ -432,7 +434,7 @@ impl<C: Context> Secp256k1<C> {
/// see comment in libsecp256k1 commit d2275795f by Gregory Maxwell.
pub fn seeded_randomize(&mut self, seed: &[u8; 32]) {
unsafe {
let err = ffi::secp256k1_context_randomize(self.ctx, seed.as_c_ptr());
let err = ffi::secp256k1_context_randomize(self.ctx.as_ptr(), seed.as_c_ptr());
// This function cannot fail; it has an error return for future-proofing.
// We do not expose this error since it is impossible to hit, and we have
// precedent for not exposing impossible errors (for example in
Expand Down Expand Up @@ -556,11 +558,12 @@ mod tests {
let ctx_sign = unsafe { ffi::secp256k1_context_create(SignOnlyPreallocated::FLAGS) };
let ctx_vrfy = unsafe { ffi::secp256k1_context_create(VerifyOnlyPreallocated::FLAGS) };

let full: Secp256k1<AllPreallocated> = Secp256k1 { ctx: ctx_full, phantom: PhantomData };
let full: Secp256k1<AllPreallocated> =
Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_full) }, phantom: PhantomData };
let sign: Secp256k1<SignOnlyPreallocated> =
Secp256k1 { ctx: ctx_sign, phantom: PhantomData };
Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_sign) }, phantom: PhantomData };
let vrfy: Secp256k1<VerifyOnlyPreallocated> =
Secp256k1 { ctx: ctx_vrfy, phantom: PhantomData };
Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_vrfy) }, phantom: PhantomData };

let (sk, pk) = full.generate_keypair(&mut rand::thread_rng());
let msg = Message::from_slice(&[2u8; 32]).unwrap();
Expand Down Expand Up @@ -590,9 +593,9 @@ mod tests {
let ctx_sign = Secp256k1::signing_only();
let ctx_vrfy = Secp256k1::verification_only();

let mut full = unsafe { Secp256k1::from_raw_all(ctx_full.ctx) };
let mut sign = unsafe { Secp256k1::from_raw_signing_only(ctx_sign.ctx) };
let mut vrfy = unsafe { Secp256k1::from_raw_verification_only(ctx_vrfy.ctx) };
let mut full = unsafe { Secp256k1::from_raw_all(ctx_full.ctx.as_ptr()) };
let mut sign = unsafe { Secp256k1::from_raw_signing_only(ctx_sign.ctx.as_ptr()) };
let mut vrfy = unsafe { Secp256k1::from_raw_verification_only(ctx_vrfy.ctx.as_ptr()) };

let (sk, pk) = full.generate_keypair(&mut rand::thread_rng());
let msg = Message::from_slice(&[2u8; 32]).unwrap();
Expand Down
4 changes: 2 additions & 2 deletions src/schnorr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl<C: Signing> Secp256k1<C> {
assert_eq!(
1,
ffi::secp256k1_schnorrsig_sign(
self.ctx,
self.ctx.as_ptr(),
sig.as_mut_c_ptr(),
msg.as_c_ptr(),
keypair.as_c_ptr(),
Expand Down Expand Up @@ -168,7 +168,7 @@ impl<C: Verification> Secp256k1<C> {
) -> Result<(), Error> {
unsafe {
let ret = ffi::secp256k1_schnorrsig_verify(
self.ctx,
self.ctx.as_ptr(),
sig.as_c_ptr(),
msg.as_c_ptr(),
32,
Expand Down

0 comments on commit 082c3bd

Please sign in to comment.