From 9b07e8e8c59938b6b681368917138d1c3938d20b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 1 Dec 2022 15:22:24 +1100 Subject: [PATCH] secp-sys: Use NonNull in API instead of *mut T Currently we expect non-null pointers when we take `*mut T` parameters, however we do not check that the pointers are non-null because we never set VERIFY in our C build. We can use the `NonNull` type to enforce no-null-ness as long as we use `NonNull::new`. In a couple of instances we manually check that a buffer is not empty and therefore that the pointer to it is non-null so we can safely use `NonNull::new_unchecked`. Replace mutable pointer parameters `*mut T` (e.g. `*mut c_void`) and return types with `NonNull`. Fix #546 --- secp256k1-sys/src/lib.rs | 45 ++++++++++++++++++++------------------ src/context.rs | 47 +++++++++++++++------------------------- src/lib.rs | 17 +++++++-------- 3 files changed, 49 insertions(+), 60 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 243c941..3a5da2d 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -39,6 +39,7 @@ pub mod types; pub mod recovery; use core::{slice, ptr}; +use core::ptr::NonNull; use types::*; /// Flag for context to enable no precomputation @@ -512,7 +513,7 @@ extern "C" { // Contexts #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_destroy")] - pub fn secp256k1_context_preallocated_destroy(cx: *mut Context); + pub fn secp256k1_context_preallocated_destroy(cx: NonNull); // Signatures #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_ecdsa_signature_parse_der")] @@ -585,16 +586,16 @@ extern "C" { pub fn secp256k1_context_preallocated_size(flags: c_uint) -> size_t; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_create")] - pub fn secp256k1_context_preallocated_create(prealloc: *mut c_void, flags: c_uint) -> *mut Context; + pub fn secp256k1_context_preallocated_create(prealloc: NonNull, flags: c_uint) -> NonNull; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_clone_size")] pub fn secp256k1_context_preallocated_clone_size(cx: *const Context) -> size_t; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_clone")] - pub fn secp256k1_context_preallocated_clone(cx: *const Context, prealloc: *mut c_void) -> *mut Context; + pub fn secp256k1_context_preallocated_clone(cx: *const Context, prealloc: NonNull) -> NonNull; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_randomize")] - pub fn secp256k1_context_randomize(cx: *mut Context, + pub fn secp256k1_context_randomize(cx: NonNull, seed32: *const c_uchar) -> c_int; // Pubkeys @@ -789,7 +790,7 @@ extern "C" { /// The newly created secp256k1 raw context. #[cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))] #[cfg_attr(docsrs, doc(cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))))] -pub unsafe fn secp256k1_context_create(flags: c_uint) -> *mut Context { +pub unsafe fn secp256k1_context_create(flags: c_uint) -> NonNull { rustsecp256k1_v0_6_1_context_create(flags) } @@ -800,7 +801,7 @@ pub unsafe fn secp256k1_context_create(flags: c_uint) -> *mut Context { #[allow(clippy::missing_safety_doc)] // Documented above. #[cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))] #[cfg_attr(docsrs, doc(cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))))] -pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_create(flags: c_uint) -> *mut Context { +pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_create(flags: c_uint) -> NonNull { use core::mem; use crate::alloc::alloc; assert!(ALIGN_TO >= mem::align_of::()); @@ -817,7 +818,8 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_create(flags: c_uint) -> * (ptr as *mut usize).write(bytes); // We must offset a whole ALIGN_TO in order to preserve the same alignment // this means we "lose" ALIGN_TO-size_of(usize) for padding. - let ptr = ptr.add(ALIGN_TO) as *mut c_void; + let ptr = ptr.add(ALIGN_TO); + let ptr = NonNull::new_unchecked(ptr as *mut c_void); // Checked above. secp256k1_context_preallocated_create(ptr, flags) } @@ -832,7 +834,7 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_create(flags: c_uint) -> * /// `ctx` must be a valid pointer to a block of memory created using [`secp256k1_context_create`]. #[cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))] #[cfg_attr(docsrs, doc(cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))))] -pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) { +pub unsafe fn secp256k1_context_destroy(ctx: NonNull) { rustsecp256k1_v0_6_1_context_destroy(ctx) } @@ -840,9 +842,10 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) { #[allow(clippy::missing_safety_doc)] // Documented above. #[cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))] #[cfg_attr(docsrs, doc(cfg(all(feature = "alloc", not(rust_secp_no_symbol_renaming)))))] -pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_destroy(ctx: *mut Context) { +pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_destroy(mut ctx: NonNull) { use crate::alloc::alloc; secp256k1_context_preallocated_destroy(ctx); + let ctx: *mut Context = ctx.as_mut(); let ptr = (ctx as *mut u8).sub(ALIGN_TO); let bytes = (ptr as *mut usize).read(); let layout = alloc::Layout::from_size_align(bytes, ALIGN_TO).unwrap(); @@ -966,8 +969,8 @@ mod fuzz_dummy { extern "C" { fn rustsecp256k1_v0_6_1_context_preallocated_size(flags: c_uint) -> size_t; - fn rustsecp256k1_v0_6_1_context_preallocated_create(prealloc: *mut c_void, flags: c_uint) -> *mut Context; - fn rustsecp256k1_v0_6_1_context_preallocated_clone(cx: *const Context, prealloc: *mut c_void) -> *mut Context; + fn rustsecp256k1_v0_6_1_context_preallocated_create(prealloc: NonNull, flags: c_uint) -> NonNull; + fn rustsecp256k1_v0_6_1_context_preallocated_clone(cx: *const Context, prealloc: NonNull) -> NonNull; } #[cfg(feature = "lowmemory")] @@ -985,7 +988,7 @@ mod fuzz_dummy { const HAVE_CONTEXT_WORKING: usize = 1; const HAVE_CONTEXT_DONE: usize = 2; static mut PREALLOCATED_CONTEXT: [u8; CTX_SIZE] = [0; CTX_SIZE]; - pub unsafe fn secp256k1_context_preallocated_create(prealloc: *mut c_void, flags: c_uint) -> *mut Context { + pub unsafe fn secp256k1_context_preallocated_create(prealloc: NonNull, flags: c_uint) -> NonNull { // While applications should generally avoid creating too many contexts, sometimes fuzzers // perform tasks repeatedly which real applications may only do rarely. Thus, we want to // avoid being overly slow here. We do so by having a static context and copying it into @@ -998,9 +1001,9 @@ mod fuzz_dummy { if have_ctx == HAVE_CONTEXT_NONE { assert!(rustsecp256k1_v0_6_1_context_preallocated_size(SECP256K1_START_SIGN | SECP256K1_START_VERIFY) + std::mem::size_of::() <= CTX_SIZE); assert_eq!(rustsecp256k1_v0_6_1_context_preallocated_create( - PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void, + NonNull::new_unchecked(PREALLOCATED_CONTEXT[..].as_mut_ptr() as *mut c_void), SECP256K1_START_SIGN | SECP256K1_START_VERIFY), - PREALLOCATED_CONTEXT[..].as_ptr() as *mut Context); + NonNull::new_unchecked(PREALLOCATED_CONTEXT[..].as_mut_ptr() as *mut Context)); assert_eq!(HAVE_PREALLOCATED_CONTEXT.swap(HAVE_CONTEXT_DONE, Ordering::AcqRel), HAVE_CONTEXT_WORKING); } else if have_ctx == HAVE_CONTEXT_DONE { @@ -1015,25 +1018,25 @@ mod fuzz_dummy { std::thread::yield_now(); } } - ptr::copy_nonoverlapping(PREALLOCATED_CONTEXT[..].as_ptr(), prealloc as *mut u8, CTX_SIZE); - let ptr = (prealloc as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::()); + ptr::copy_nonoverlapping(PREALLOCATED_CONTEXT[..].as_ptr(), prealloc.as_ptr() as *mut u8, CTX_SIZE); + let ptr = (prealloc.as_ptr()).add(CTX_SIZE).sub(std::mem::size_of::()); (ptr as *mut c_uint).write(flags); - prealloc as *mut Context + NonNull::new_unchecked(prealloc.as_ptr() as *mut Context) } pub unsafe fn secp256k1_context_preallocated_clone_size(_cx: *const Context) -> size_t { CTX_SIZE } - pub unsafe fn secp256k1_context_preallocated_clone(cx: *const Context, prealloc: *mut c_void) -> *mut Context { + pub unsafe fn secp256k1_context_preallocated_clone(cx: *const Context, prealloc: NonNull) -> NonNull { let orig_ptr = (cx as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::()); - let new_ptr = (prealloc as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::()); + let new_ptr = (prealloc.as_ptr() as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::()); let flags = (orig_ptr as *mut c_uint).read(); (new_ptr as *mut c_uint).write(flags); rustsecp256k1_v0_6_1_context_preallocated_clone(cx, prealloc) } - pub unsafe fn secp256k1_context_randomize(cx: *mut Context, + pub unsafe fn secp256k1_context_randomize(cx: NonNull, _seed32: *const c_uchar) -> c_int { // This function is really slow, and unsuitable for fuzzing - check_context_flags(cx, 0); + check_context_flags(cx.as_ptr(), 0); 1 } diff --git a/src/context.rs b/src/context.rs index d90d332..f1183d8 100644 --- a/src/context.rs +++ b/src/context.rs @@ -204,18 +204,12 @@ mod alloc_only { let size = unsafe { ffi::secp256k1_context_preallocated_size(C::FLAGS) }; 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); - } + let ptr = NonNull::new(ptr as *mut c_void) + .unwrap_or_else(|| alloc::handle_alloc_error(layout)); #[allow(unused_mut)] // ctx is not mutated under some feature combinations. let mut ctx = Secp256k1 { - ctx: unsafe { - NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create( - ptr as *mut c_void, - C::FLAGS, - )) - }, + ctx: unsafe { ffi::secp256k1_context_preallocated_create(ptr, C::FLAGS) }, phantom: PhantomData, }; @@ -269,16 +263,11 @@ mod alloc_only { 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); - } + let ptr = NonNull::new(ptr as *mut c_void) + .unwrap_or_else(|| alloc::handle_alloc_error(layout)); + Secp256k1 { - ctx: unsafe { - NonNull::new_unchecked(ffi::secp256k1_context_preallocated_clone( - self.ctx.as_ptr(), - ptr as *mut c_void, - )) - }, + ctx: unsafe { ffi::secp256k1_context_preallocated_clone(self.ctx.as_ptr(), ptr) }, phantom: PhantomData, } } @@ -327,13 +316,11 @@ impl<'buf, C: Context + 'buf> Secp256k1 { if buf.len() < Self::preallocate_size_gen() { return Err(Error::NotEnoughMemory); } + // Safe because buf is not null since it is not empty. + let buf = unsafe { NonNull::new_unchecked(buf.as_mut_c_ptr() as *mut c_void) }; + Ok(Secp256k1 { - ctx: unsafe { - NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create( - buf.as_mut_c_ptr() as *mut c_void, - C::FLAGS, - )) - }, + ctx: unsafe { ffi::secp256k1_context_preallocated_create(buf, AllPreallocated::FLAGS) }, phantom: PhantomData, }) } @@ -361,9 +348,9 @@ impl<'buf> Secp256k1> { /// * Violating these may lead to Undefined Behavior. /// pub unsafe fn from_raw_all( - raw_ctx: *mut ffi::Context, + raw_ctx: NonNull, ) -> ManuallyDrop>> { - ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData }) + ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData }) } } @@ -392,9 +379,9 @@ impl<'buf> Secp256k1> { /// * This list *is not* exhaustive, and any violation may lead to Undefined Behavior. /// pub unsafe fn from_raw_signing_only( - raw_ctx: *mut ffi::Context, + raw_ctx: NonNull, ) -> ManuallyDrop>> { - ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData }) + ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData }) } } @@ -423,8 +410,8 @@ impl<'buf> Secp256k1> { /// * This list *is not* exhaustive, and any violation may lead to Undefined Behavior. /// pub unsafe fn from_raw_verification_only( - raw_ctx: *mut ffi::Context, + raw_ctx: NonNull, ) -> ManuallyDrop>> { - ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData }) + ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData }) } } diff --git a/src/lib.rs b/src/lib.rs index 4091e8a..125c60c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -389,7 +389,7 @@ impl Drop for Secp256k1 { fn drop(&mut self) { unsafe { let size = ffi::secp256k1_context_preallocated_clone_size(self.ctx.as_ptr()); - ffi::secp256k1_context_preallocated_destroy(self.ctx.as_ptr()); + ffi::secp256k1_context_preallocated_destroy(self.ctx); C::deallocate(self.ctx.as_ptr() as _, size); } @@ -434,7 +434,7 @@ impl Secp256k1 { /// 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.as_ptr(), seed.as_c_ptr()); + let err = ffi::secp256k1_context_randomize(self.ctx, 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 @@ -558,12 +558,11 @@ 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 = - Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_full) }, phantom: PhantomData }; + let full: Secp256k1 = Secp256k1 { ctx: ctx_full, phantom: PhantomData }; let sign: Secp256k1 = - Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_sign) }, phantom: PhantomData }; + Secp256k1 { ctx: ctx_sign, phantom: PhantomData }; let vrfy: Secp256k1 = - Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_vrfy) }, phantom: PhantomData }; + Secp256k1 { ctx: ctx_vrfy, phantom: PhantomData }; let (sk, pk) = full.generate_keypair(&mut rand::thread_rng()); let msg = Message::from_slice(&[2u8; 32]).unwrap(); @@ -593,9 +592,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.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 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 (sk, pk) = full.generate_keypair(&mut rand::thread_rng()); let msg = Message::from_slice(&[2u8; 32]).unwrap();