Merge rust-bitcoin/rust-secp256k1#524: Document unsafe code

6d747301e8 secp256k1: Document safety constraints (Tobin C. Harding)
85681cece7 secp256k1-sys: Document safety constraints (Tobin C. Harding)

Pull request description:

  Functions that are `unsafe` should include a `# Safety` section.

  - Patch 1: Documents `unsafe` methods in `secp256k1-sys`. Please not this includes a minor refactor.
  - Patch 2: Documents the `unsafe` `Context` trait

  Together these two patches remove `#![allow(clippy::missing_safety_doc)]` from the repo.

  Fix: #447

  ## Note to reviewers

  The only function that was curly to understand the safety of was `secp256k1_context_create`, all the other stuff should be trivial to review.

ACKs for top commit:
  apoelstra:
    ACK 6d747301e8

Tree-SHA512: 247216c3f9e655fe8c2854b71613b31b6241318e877e83a1e4873ce84e481975a832d05cd748577f437f88b166ff287a537d26c012568e7378caed458ec55867

secp256k1-sys/src/lib.rs

@@ -19,8 +19,6 @@
 // Coding conventions
 #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case, unused_mut)]
 
-#![allow(clippy::missing_safety_doc)]
-
 #![cfg_attr(all(not(test), not(feature = "std")), no_std)]
 #![cfg_attr(docsrs, feature(doc_cfg))]
 
@@ -781,10 +779,25 @@ extern "C" {
 ///
 /// Input `flags` control which parts of the context to initialize.
 ///
+/// # Safety
+///
+/// This function is unsafe because it calls unsafe functions however (assuming no bugs) no
+/// undefined behavior is possible.
+///
 /// # Returns
 ///
 /// 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 {
+    rustsecp256k1_v0_6_1_context_create(flags)
+}
+
+/// A reimplementation of the C function `secp256k1_context_create` in rust.
+///
+/// See [`secp256k1_context_create`] for documentation and safety constraints.
 #[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_create(flags: c_uint) -> *mut Context {
@@ -805,19 +818,23 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_create(flags: c_uint) -> *
     secp256k1_context_preallocated_create(ptr, flags)
 }
 
-#[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 {
-    rustsecp256k1_v0_6_1_context_create(flags)
-}
-
 /// A reimplementation of the C function `secp256k1_context_destroy` in rust.
 ///
 /// This function destroys and deallcates the context created by `secp256k1_context_create`.
 ///
 /// The pointer shouldn't be used after passing to this function, consider it as passing it to `free()`.
 ///
+/// # Safety
+///
+///  `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) {
+    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) {
@@ -829,13 +846,6 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_context_destroy(ctx: *mut Context)
     alloc::dealloc(ptr, layout);
 }
 
-#[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) {
-    rustsecp256k1_v0_6_1_context_destroy(ctx)
-}
-
-
 /// **This function is an override for the C function, this is the an edited version of the original description:**
 ///
 /// A callback function to be called when an illegal argument is passed to
@@ -854,6 +864,12 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) {
 ///
 /// See also secp256k1_default_error_callback_fn.
 ///
+///
+/// # Safety
+///
+/// `message` string should be a null terminated C string and, up to the first null byte, must be valid UTF8.
+///
+/// For exact safety constraints see [`std::slice::from_raw_parts`] and [`std::str::from_utf8_unchecked`].
 #[no_mangle]
 #[cfg(not(rust_secp_no_symbol_renaming))]
 pub unsafe extern "C" fn rustsecp256k1_v0_6_1_default_illegal_callback_fn(message: *const c_char, _data: *mut c_void) {
@@ -877,6 +893,11 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_default_illegal_callback_fn(messag
 ///
 /// See also secp256k1_default_illegal_callback_fn.
 ///
+/// # Safety
+///
+/// `message` string should be a null terminated C string and, up to the first null byte, must be valid UTF8.
+///
+/// For exact safety constraints see [`std::slice::from_raw_parts`] and [`std::str::from_utf8_unchecked`].
 #[no_mangle]
 #[cfg(not(rust_secp_no_symbol_renaming))]
 pub unsafe extern "C" fn rustsecp256k1_v0_6_1_default_error_callback_fn(message: *const c_char, _data: *mut c_void) {
@@ -886,6 +907,11 @@ pub unsafe extern "C" fn rustsecp256k1_v0_6_1_default_error_callback_fn(message:
     panic!("[libsecp256k1] internal consistency check failed {}", msg);
 }
 
+/// Returns the length of the `str_ptr` string.
+///
+/// # Safety
+///
+/// `str_ptr` must be valid pointer and point to a valid null terminated C string.
 #[cfg(not(rust_secp_no_symbol_renaming))]
 unsafe fn strlen(mut str_ptr: *const c_char) -> usize {
     let mut ctr = 0;

src/context.rs

@@ -62,12 +62,20 @@ pub mod global {
 
 /// A trait for all kinds of contexts that lets you define the exact flags and a function to
 /// deallocate memory. It isn't possible to implement this for types outside this crate.
+///
+/// # Safety
+///
+/// This trait is marked unsafe to allow unsafe implementations of `deallocate`.
 pub unsafe trait Context: private::Sealed {
     /// Flags for the ffi.
     const FLAGS: c_uint;
     /// A constant description of the context.
     const DESCRIPTION: &'static str;
     /// A function to deallocate the memory when the context is dropped.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must be valid. Further safety constraints may be imposed by [`std::alloc::dealloc`].
     unsafe fn deallocate(ptr: *mut u8, size: usize);
 }
 

src/lib.rs

@@ -152,7 +152,6 @@
 // Coding conventions
 #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case)]
 #![warn(missing_docs, missing_copy_implementations, missing_debug_implementations)]
-#![allow(clippy::missing_safety_doc)]
 #![cfg_attr(all(not(test), not(feature = "std")), no_std)]
 // Experimental features we need.
 #![cfg_attr(docsrs, feature(doc_cfg))]