Merge rust-bitcoin/rust-secp256k1#551: secp-sys: Use NonNull in API instead of *mut T

9b07e8e8c5 secp-sys: Use NonNull in API instead of *mut T (Tobin C. Harding)

Pull request description:

  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

  ### Note

  The description above fully explains the issue to the best of my knowledge, if the description is not spot on then I'm not fully understanding the issue. Please correct me if this is the case.

  > One unfortunate thing is that this means that we wouldn't be able to implement CPtr for secp256k1::Context, which is our designated "expose types from secp256k1-sys which is not stable" semver escape hatch.

  You've lost me here? `secp256k1::Context` is a trait did you mean `secp256k1::Secp256k1` or `secp256k1_sys::Context`?

ACKs for top commit:
  apoelstra:
    ACK 9b07e8e8c5

Tree-SHA512: 37aceebfa62e590ce8cc282c35b014ad018e5cfbea99402ed3aa1fcbaa69e01a01c1c1f32351f5f15a7d270e31da5b239ee5bc11d2343cf866082ad85df6a622

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,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<Context>) {
     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<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_create(prealloc: NonNull<c_void>, flags: c_uint) -> NonNull<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_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);
+        ptr::copy_nonoverlapping(PREALLOCATED_CONTEXT[..].as_ptr(), prealloc.as_ptr() as *mut u8, CTX_SIZE);
-        let ptr = (prealloc as *mut u8).add(CTX_SIZE).sub(std::mem::size_of::<c_uint>());
+        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() {
+            let ptr = NonNull::new(ptr as *mut c_void)
-                alloc::handle_alloc_error(layout);
+                .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 {
+                ctx: unsafe { ffi::secp256k1_context_preallocated_create(ptr, C::FLAGS) },
-                    NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create(
-                        ptr as *mut c_void,
-                        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() {
+            let ptr = NonNull::new(ptr as *mut c_void)
-                alloc::handle_alloc_error(layout);
+                .unwrap_or_else(|| alloc::handle_alloc_error(layout));
-            }
+
             Secp256k1 {
-                ctx: unsafe {
+                ctx: unsafe { ffi::secp256k1_context_preallocated_clone(self.ctx.as_ptr(), ptr) },
-                    NonNull::new_unchecked(ffi::secp256k1_context_preallocated_clone(
-                        self.ctx.as_ptr(),
-                        ptr as *mut c_void,
-                    ))
-                },
                 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 {
+            ctx: unsafe { ffi::secp256k1_context_preallocated_create(buf, AllPreallocated::FLAGS) },
-                NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create(
-                    buf.as_mut_c_ptr() as *mut c_void,
-                    C::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> =
+        let full: Secp256k1<AllPreallocated> = Secp256k1 { ctx: ctx_full, phantom: PhantomData };
-            Secp256k1 { ctx: unsafe { NonNull::new_unchecked(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 full = unsafe { Secp256k1::from_raw_all(ctx_full.ctx) };
-        let mut sign = unsafe { Secp256k1::from_raw_signing_only(ctx_sign.ctx.as_ptr()) };
+        let mut sign = unsafe { Secp256k1::from_raw_signing_only(ctx_sign.ctx) };
-        let mut vrfy = unsafe { Secp256k1::from_raw_verification_only(ctx_vrfy.ctx.as_ptr()) };
+        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();