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<T>`.

Fix #546

Author: tcharding

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<Context>);
 
     // 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<c_void>, flags: c_uint) -> NonNull<Context>;
 
     #[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<c_void>) -> NonNull<Context>;
 
     #[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<Context>,
                                        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<Context> {
     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<Context> {
     use core::mem;
     use crate::alloc::alloc;
     assert!(ALIGN_TO >= mem::align_of::<usize>());
@@ -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,17 +834,18 @@ 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<Context>) {
     rustsecp256k1_v0_6_1_context_destroy(ctx)
 }
 
 #[no_mangle]
 #[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<Context>) {
     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<c_void>, flags: c_uint) -> NonNull<Context>;
+        fn rustsecp256k1_v0_6_1_context_preallocated_clone(cx: *const Context, prealloc: NonNull<c_void>) -> NonNull<Context>;
     }
 
     #[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<c_void>, flags: c_uint) -> NonNull<Context> {
         // 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::<c_uint>() <= 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::<c_uint>());
+        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::<c_uint>());
         (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<c_void>) -> NonNull<Context> {
         let orig_ptr = (cx as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::<c_uint>());
-        let new_ptr = (prealloc as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::<c_uint>());
+        let new_ptr = (prealloc.as_ptr() as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::<c_uint>());
         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<Context>,
                                               _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
     }
 

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<C> {
         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<AllPreallocated<'buf>> {
     /// * Violating these may lead to Undefined Behavior.
     ///
     pub unsafe fn from_raw_all(
-        raw_ctx: *mut ffi::Context,
+        raw_ctx: NonNull<ffi::Context>,
     ) -> ManuallyDrop<Secp256k1<AllPreallocated<'buf>>> {
-        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<SignOnlyPreallocated<'buf>> {
     /// * 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<ffi::Context>,
     ) -> ManuallyDrop<Secp256k1<SignOnlyPreallocated<'buf>>> {
-        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<VerifyOnlyPreallocated<'buf>> {
     /// * 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<ffi::Context>,
     ) -> ManuallyDrop<Secp256k1<VerifyOnlyPreallocated<'buf>>> {
-        ManuallyDrop::new(Secp256k1 { ctx: NonNull::new_unchecked(raw_ctx), phantom: PhantomData })
+        ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData })
     }
 }

src/lib.rs

@@ -389,7 +389,7 @@ impl<C: Context> Drop for Secp256k1<C> {
     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<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.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<AllPreallocated> =
-            Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_full) }, phantom: PhantomData };
+        let full: Secp256k1<AllPreallocated> = Secp256k1 { ctx: ctx_full, phantom: PhantomData };
         let sign: Secp256k1<SignOnlyPreallocated> =
-            Secp256k1 { ctx: unsafe { NonNull::new_unchecked(ctx_sign) }, phantom: PhantomData };
+            Secp256k1 { ctx: ctx_sign, phantom: PhantomData };
         let vrfy: Secp256k1<VerifyOnlyPreallocated> =
-            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();