From 7b99784837ebfe872aaa6d14832fb68597c3f42c Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Fri, 28 Aug 2020 13:58:07 +0300 Subject: [PATCH 1/4] Add AligneType and redo secp256k1_context_create with alloc --- secp256k1-sys/Cargo.toml | 3 +++ secp256k1-sys/src/lib.rs | 39 +++++++++++++++++++------------------- secp256k1-sys/src/types.rs | 25 ++++++++++++++++++++++-- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/secp256k1-sys/Cargo.toml b/secp256k1-sys/Cargo.toml index 09c6227e8..bcd700e3c 100644 --- a/secp256k1-sys/Cargo.toml +++ b/secp256k1-sys/Cargo.toml @@ -21,6 +21,9 @@ features = [ "recovery", "endomorphism", "lowmemory" ] [build-dependencies] cc = "1.0.28" +[dev-dependencies] +libc = "0.2" + [features] default = ["std"] recovery = [] diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 27de54481..170d18bec 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -534,19 +534,21 @@ extern "C" { #[no_mangle] #[cfg(all(feature = "std", not(feature = "external-symbols")))] pub unsafe extern "C" fn rustsecp256k1_v0_3_1_context_create(flags: c_uint) -> *mut Context { - use std::mem; - assert!(mem::align_of::() >= mem::align_of::()); - assert_eq!(mem::size_of::(), mem::size_of::<&usize>()); - - let word_size = mem::size_of::(); - let n_words = (secp256k1_context_preallocated_size(flags) + word_size - 1) / word_size; - - let buf = vec![0usize; n_words + 1].into_boxed_slice(); - let ptr = Box::into_raw(buf) as *mut usize; - ::core::ptr::write(ptr, n_words); - let ptr: *mut usize = ptr.offset(1); - - secp256k1_context_preallocated_create(ptr as *mut c_void, flags) + use core::mem; + use std::alloc; + assert!(ALIGN_TO >= mem::align_of::()); + assert!(ALIGN_TO >= mem::align_of::<&usize>()); + assert!(ALIGN_TO >= mem::size_of::()); + + // We need to allocate `ALIGN_TO` more bytes in order to write the amount of bytes back. + let bytes = secp256k1_context_preallocated_size(flags) + ALIGN_TO; + let layout = alloc::Layout::from_size_align(bytes, ALIGN_TO).unwrap(); + let ptr = alloc::alloc(layout); + (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; + secp256k1_context_preallocated_create(ptr, flags) } #[cfg(all(feature = "std", not(feature = "external-symbols")))] @@ -563,13 +565,12 @@ pub unsafe fn secp256k1_context_create(flags: c_uint) -> *mut Context { #[no_mangle] #[cfg(all(feature = "std", not(feature = "external-symbols")))] pub unsafe extern "C" fn rustsecp256k1_v0_3_1_context_destroy(ctx: *mut Context) { + use std::alloc; secp256k1_context_preallocated_destroy(ctx); - let ctx: *mut usize = ctx as *mut usize; - - let n_words_ptr: *mut usize = ctx.offset(-1); - let n_words: usize = ::core::ptr::read(n_words_ptr); - let slice: &mut [usize] = slice::from_raw_parts_mut(n_words_ptr , n_words+1); - let _ = Box::from_raw(slice as *mut [usize]); + 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(); + alloc::dealloc(ptr, layout); } #[cfg(all(feature = "std", not(feature = "external-symbols")))] diff --git a/secp256k1-sys/src/types.rs b/secp256k1-sys/src/types.rs index 78216af0d..98b35efe8 100644 --- a/secp256k1-sys/src/types.rs +++ b/secp256k1-sys/src/types.rs @@ -1,5 +1,5 @@ #![allow(non_camel_case_types)] -use core::fmt; +use core::{fmt, mem}; pub type c_int = i32; pub type c_uchar = u8; @@ -26,11 +26,30 @@ impl fmt::Debug for c_void { } } +/// A type that is as aligned as the biggest alignment for fundamental types in C +/// since C11 that means as aligned as `max_align_t` is. +/// the exact size/alignment is unspecified. +// 16 matches is as big as the biggest alignment in any arch that rust currently supports https://github.com/rust-lang/rust/blob/2c31b45ae878b821975c4ebd94cc1e49f6073fd0/library/std/src/sys_common/alloc.rs +#[repr(align(16))] +#[derive(Default, Copy, Clone)] +pub struct AlignedType([u8; 16]); + +impl AlignedType { + pub fn zeroed() -> Self { + AlignedType([0u8; 16]) + } +} + +pub(crate) const ALIGN_TO: usize = mem::align_of::(); + + #[cfg(test)] mod tests { + extern crate libc; use std::os::raw; + use std::mem; use std::any::TypeId; - use types; + use {types, AlignedType}; #[test] fn verify_types() { @@ -38,6 +57,8 @@ mod tests { assert_eq!(TypeId::of::(), TypeId::of::()); assert_eq!(TypeId::of::(), TypeId::of::()); assert_eq!(TypeId::of::(), TypeId::of::()); + + assert!(mem::align_of::() >= mem::align_of::()); } } From fd206ab57cc9808d894aecdac74486f048ef5407 Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Fri, 28 Aug 2020 15:20:19 +0300 Subject: [PATCH 2/4] Replace use of boxes with global allocator --- src/context.rs | 54 ++++++++++++++++++++++++++++---------------------- src/lib.rs | 12 +++++------ 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/context.rs b/src/context.rs index d78b3e129..1183de9f1 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,7 +1,6 @@ use core::marker::PhantomData; -use core::mem::ManuallyDrop; -use ptr; -use ffi::{self, CPtr}; +use core::mem::{self, ManuallyDrop}; +use ffi::{self, CPtr, types::AlignedType}; use ffi::types::{c_uint, c_void}; use Error; use Secp256k1; @@ -50,7 +49,7 @@ pub unsafe trait Context : private::Sealed { /// A constant description of the context. const DESCRIPTION: &'static str; /// A function to deallocate the memory when the context is dropped. - unsafe fn deallocate(ptr: *mut [u8]); + unsafe fn deallocate(ptr: *mut u8, size: usize); } /// Marker trait for indicating that an instance of `Secp256k1` can be used for signing. @@ -93,6 +92,8 @@ mod std_only { impl private::Sealed for VerifyOnly {} use super::*; + use std::alloc; + const ALIGN_TO: usize = mem::align_of::(); /// Represents the set of capabilities needed for signing. pub enum SignOnly {} @@ -113,8 +114,9 @@ mod std_only { const FLAGS: c_uint = ffi::SECP256K1_START_SIGN; const DESCRIPTION: &'static str = "signing only"; - unsafe fn deallocate(ptr: *mut [u8]) { - let _ = Box::from_raw(ptr); + unsafe fn deallocate(ptr: *mut u8, size: usize) { + let layout = alloc::Layout::from_size_align(size, ALIGN_TO).unwrap(); + alloc::dealloc(ptr, layout); } } @@ -122,8 +124,9 @@ mod std_only { const FLAGS: c_uint = ffi::SECP256K1_START_VERIFY; const DESCRIPTION: &'static str = "verification only"; - unsafe fn deallocate(ptr: *mut [u8]) { - let _ = Box::from_raw(ptr); + unsafe fn deallocate(ptr: *mut u8, size: usize) { + let layout = alloc::Layout::from_size_align(size, ALIGN_TO).unwrap(); + alloc::dealloc(ptr, layout); } } @@ -131,8 +134,9 @@ mod std_only { const FLAGS: c_uint = VerifyOnly::FLAGS | SignOnly::FLAGS; const DESCRIPTION: &'static str = "all capabilities"; - unsafe fn deallocate(ptr: *mut [u8]) { - let _ = Box::from_raw(ptr); + unsafe fn deallocate(ptr: *mut u8, size: usize) { + let layout = alloc::Layout::from_size_align(size, ALIGN_TO).unwrap(); + alloc::dealloc(ptr, layout); } } @@ -142,12 +146,13 @@ mod std_only { #[cfg(target_arch = "wasm32")] ffi::types::sanity_checks_for_wasm(); - let buf = vec![0u8; Self::preallocate_size_gen()].into_boxed_slice(); - let ptr = Box::into_raw(buf); + 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)}; Secp256k1 { ctx: unsafe { ffi::secp256k1_context_preallocated_create(ptr as *mut c_void, C::FLAGS) }, phantom: PhantomData, - buf: ptr, + size, } } } @@ -181,12 +186,13 @@ mod std_only { impl Clone for Secp256k1 { fn clone(&self) -> Secp256k1 { - let clone_size = unsafe {ffi::secp256k1_context_preallocated_clone_size(self.ctx)}; - let ptr_buf = Box::into_raw(vec![0u8; clone_size].into_boxed_slice()); + let size = unsafe {ffi::secp256k1_context_preallocated_clone_size(self.ctx as _)}; + let layout = alloc::Layout::from_size_align(size, ALIGN_TO).unwrap(); + let ptr = unsafe {alloc::alloc(layout)}; Secp256k1 { - ctx: unsafe { ffi::secp256k1_context_preallocated_clone(self.ctx, ptr_buf as *mut c_void) }, + ctx: unsafe { ffi::secp256k1_context_preallocated_clone(self.ctx, ptr as *mut c_void) }, phantom: PhantomData, - buf: ptr_buf, + size, } } } @@ -202,7 +208,7 @@ unsafe impl<'buf> Context for SignOnlyPreallocated<'buf> { const FLAGS: c_uint = ffi::SECP256K1_START_SIGN; const DESCRIPTION: &'static str = "signing only"; - unsafe fn deallocate(_ptr: *mut [u8]) { + unsafe fn deallocate(_ptr: *mut u8, _size: usize) { // Allocated by the user } } @@ -211,7 +217,7 @@ unsafe impl<'buf> Context for VerifyOnlyPreallocated<'buf> { const FLAGS: c_uint = ffi::SECP256K1_START_VERIFY; const DESCRIPTION: &'static str = "verification only"; - unsafe fn deallocate(_ptr: *mut [u8]) { + unsafe fn deallocate(_ptr: *mut u8, _size: usize) { // Allocated by the user } } @@ -220,7 +226,7 @@ unsafe impl<'buf> Context for AllPreallocated<'buf> { const FLAGS: c_uint = SignOnlyPreallocated::FLAGS | VerifyOnlyPreallocated::FLAGS; const DESCRIPTION: &'static str = "all capabilities"; - unsafe fn deallocate(_ptr: *mut [u8]) { + unsafe fn deallocate(_ptr: *mut u8, _size: usize) { // Allocated by the user } } @@ -241,7 +247,7 @@ impl<'buf, C: Context + 'buf> Secp256k1 { C::FLAGS) }, phantom: PhantomData, - buf: buf as *mut [u8], + size: 0, // We don't care about the size because it's the caller responsibility to deallocate. }) } } @@ -271,7 +277,7 @@ impl<'buf> Secp256k1> { ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData, - buf: ptr::null_mut::<[u8;0]>() as *mut [u8] , + size: 0, // We don't care about the size because it's the caller responsibility to deallocate. }) } } @@ -303,7 +309,7 @@ impl<'buf> Secp256k1> { ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData, - buf: ptr::null_mut::<[u8;0]>() as *mut [u8] , + size: 0, // We don't care about the size because it's the caller responsibility to deallocate. }) } } @@ -335,7 +341,7 @@ impl<'buf> Secp256k1> { ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData, - buf: ptr::null_mut::<[u8;0]>() as *mut [u8] , + size: 0, // We don't care about the size because it's the caller responsibility to deallocate. }) } } diff --git a/src/lib.rs b/src/lib.rs index 483ddd9dd..cf39ac1e9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -558,7 +558,7 @@ impl std::error::Error for Error {} pub struct Secp256k1 { ctx: *mut ffi::Context, phantom: PhantomData, - buf: *mut [u8], + size: usize, } // The underlying secp context does not contain any references to memory it does not own @@ -607,7 +607,7 @@ impl Drop for Secp256k1 { fn drop(&mut self) { unsafe { ffi::secp256k1_context_preallocated_destroy(self.ctx); - C::deallocate(self.buf); + C::deallocate(self.ctx as _, self.size); } } } @@ -781,10 +781,10 @@ mod tests { let ctx_sign = unsafe { ffi::secp256k1_context_create(SignOnlyPreallocated::FLAGS) }; let ctx_vrfy = unsafe { ffi::secp256k1_context_create(VerifyOnlyPreallocated::FLAGS) }; - let buf: *mut [u8] = &mut [0u8;0] as _; - let full: Secp256k1 = Secp256k1{ctx: ctx_full, phantom: PhantomData, buf}; - let sign: Secp256k1 = Secp256k1{ctx: ctx_sign, phantom: PhantomData, buf}; - let vrfy: Secp256k1 = Secp256k1{ctx: ctx_vrfy, phantom: PhantomData, buf}; + let size = 0; + let full: Secp256k1 = Secp256k1{ctx: ctx_full, phantom: PhantomData, size}; + let sign: Secp256k1 = Secp256k1{ctx: ctx_sign, phantom: PhantomData, size}; + let vrfy: Secp256k1 = Secp256k1{ctx: ctx_vrfy, phantom: PhantomData, size}; let (sk, pk) = full.generate_keypair(&mut thread_rng()); let msg = Message::from_slice(&[2u8; 32]).unwrap(); From 767246a2829bbb5178a6ad93c980e7f2b4fa0ab6 Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Fri, 28 Aug 2020 15:20:50 +0300 Subject: [PATCH 3/4] Make preallocated use AlignedType --- src/context.rs | 8 ++++---- src/lib.rs | 18 +++++++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/context.rs b/src/context.rs index 1183de9f1..f5eb55dd6 100644 --- a/src/context.rs +++ b/src/context.rs @@ -233,7 +233,7 @@ unsafe impl<'buf> Context for AllPreallocated<'buf> { impl<'buf, C: Context + 'buf> Secp256k1 { /// Lets you create a context with preallocated buffer in a generic manner(sign/verify/all) - pub fn preallocated_gen_new(buf: &'buf mut [u8]) -> Result, Error> { + pub fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result, Error> { #[cfg(target_arch = "wasm32")] ffi::types::sanity_checks_for_wasm(); @@ -254,7 +254,7 @@ impl<'buf, C: Context + 'buf> Secp256k1 { impl<'buf> Secp256k1> { /// Creates a new Secp256k1 context with all capabilities - pub fn preallocated_new(buf: &'buf mut [u8]) -> Result>, Error> { + pub fn preallocated_new(buf: &'buf mut [AlignedType]) -> Result>, Error> { Secp256k1::preallocated_gen_new(buf) } /// Uses the ffi `secp256k1_context_preallocated_size` to check the memory size needed for a context @@ -284,7 +284,7 @@ impl<'buf> Secp256k1> { impl<'buf> Secp256k1> { /// Creates a new Secp256k1 context that can only be used for signing - pub fn preallocated_signing_only(buf: &'buf mut [u8]) -> Result>, Error> { + pub fn preallocated_signing_only(buf: &'buf mut [AlignedType]) -> Result>, Error> { Secp256k1::preallocated_gen_new(buf) } @@ -316,7 +316,7 @@ impl<'buf> Secp256k1> { impl<'buf> Secp256k1> { /// Creates a new Secp256k1 context that can only be used for verification - pub fn preallocated_verification_only(buf: &'buf mut [u8]) -> Result>, Error> { + pub fn preallocated_verification_only(buf: &'buf mut [AlignedType]) -> Result>, Error> { Secp256k1::preallocated_gen_new(buf) } diff --git a/src/lib.rs b/src/lib.rs index cf39ac1e9..cadd93205 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,7 +151,8 @@ pub use key::PublicKey; pub use context::*; use core::marker::PhantomData; use core::ops::Deref; -use ffi::CPtr; +use core::mem; +use ffi::{CPtr, types::AlignedType}; #[cfg(feature = "global-context")] pub use context::global::SECP256K1; @@ -630,7 +631,10 @@ impl Secp256k1 { /// Returns the required memory for a preallocated context buffer in a generic manner(sign/verify/all) pub fn preallocate_size_gen() -> usize { - unsafe { ffi::secp256k1_context_preallocated_size(C::FLAGS) } + let word_size = mem::size_of::(); + let bytes = unsafe { ffi::secp256k1_context_preallocated_size(C::FLAGS) }; + + (bytes + word_size - 1) / word_size } /// (Re)randomizes the Secp256k1 context for cheap sidechannel resistance; @@ -763,7 +767,7 @@ mod tests { use super::constants; use super::{Secp256k1, Signature, Message}; use super::Error::{InvalidMessage, IncorrectSignature, InvalidSignature}; - use ffi; + use ffi::{self, types::AlignedType}; use context::*; macro_rules! hex { @@ -840,10 +844,10 @@ mod tests { #[test] fn test_preallocation() { - let mut buf_ful = vec![0u8; Secp256k1::preallocate_size()]; - let mut buf_sign = vec![0u8; Secp256k1::preallocate_signing_size()]; - let mut buf_vfy = vec![0u8; Secp256k1::preallocate_verification_size()]; -// + let mut buf_ful = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()]; + let mut buf_sign = vec![AlignedType::zeroed(); Secp256k1::preallocate_signing_size()]; + let mut buf_vfy = vec![AlignedType::zeroed(); Secp256k1::preallocate_verification_size()]; + let full = Secp256k1::preallocated_new(&mut buf_ful).unwrap(); let sign = Secp256k1::preallocated_signing_only(&mut buf_sign).unwrap(); let vrfy = Secp256k1::preallocated_verification_only(&mut buf_vfy).unwrap(); From 0638107918d31a5de7d6f01c5823e71542bcf339 Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Fri, 28 Aug 2020 18:18:53 +0300 Subject: [PATCH 4/4] Adopt no-std tests to new preacllocated_* functions --- no_std_test/src/main.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/no_std_test/src/main.rs b/no_std_test/src/main.rs index fbf87a53f..58e5c9ca4 100644 --- a/no_std_test/src/main.rs +++ b/no_std_test/src/main.rs @@ -53,6 +53,7 @@ use core::intrinsics; use core::panic::PanicInfo; use secp256k1::ecdh::SharedSecret; +use secp256k1::ffi::types::AlignedType; use secp256k1::rand::{self, RngCore}; use secp256k1::serde::Serialize; use secp256k1::*; @@ -82,7 +83,7 @@ impl RngCore for FakeRng { #[start] fn start(_argc: isize, _argv: *const *const u8) -> isize { - let mut buf = [0u8; 600_000]; + let mut buf = [AlignedType::zeroed(); 37_000]; let size = Secp256k1::preallocate_size(); unsafe { libc::printf("needed size: %d\n\0".as_ptr() as _, size) }; @@ -161,5 +162,5 @@ fn panic(info: &PanicInfo) -> ! { let mut buf = Print::new(); write(&mut buf, *msg).unwrap(); buf.print(); - unsafe { intrinsics::abort() } + intrinsics::abort() }