From 85681cece7e5b66a6e72abece678b9f0ae65a782 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 07:49:16 +1100 Subject: [PATCH 1/2] secp256k1-sys: Document safety constraints Functions that are `unsafe` should include a `# Safety` section. Because we have wrapper functions to handle symbol renaming we essentially have duplicate functions i.e., they require the same docs, instead of duplicating the docs put the symbol renamed function below the non-renamed function and add a docs linking to the non-renamed function. Also add attribute to stop the linter warning about the missing safety docs section. Remove the clippy attribute for `missing_safety_doc`. --- secp256k1-sys/src/lib.rs | 56 +++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 6239d0b..496d1bc 100644 --- a/secp256k1-sys/src/lib.rs +++ b/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; From 6d747301e8dcdf5744ced973d2e2c36cdce21be4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 08:28:57 +1100 Subject: [PATCH 2/2] secp256k1: Document safety constraints Add a `# Safety` section to all unsafe traits, methods, and functions. Remove the clippy attribute for `missing_safety_doc`. --- src/context.rs | 8 ++++++++ src/lib.rs | 1 - 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/context.rs b/src/context.rs index 937ae5d..3d61fe8 100644 --- a/src/context.rs +++ b/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); } diff --git a/src/lib.rs b/src/lib.rs index a443560..7471a18 100644 --- a/src/lib.rs +++ b/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))]