Merge pull request #233 from elichai/alloc-AlignedType2

Making sure everything is aligned correctly. Succeeder of #141

Author: apoelstra

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()
 }

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 = []

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::<usize>() >= mem::align_of::<u8>());
-    assert_eq!(mem::size_of::<usize>(), mem::size_of::<&usize>());
-
-    let word_size = mem::size_of::<usize>();
-    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::<usize>());
+    assert!(ALIGN_TO >= mem::align_of::<&usize>());
+    assert!(ALIGN_TO >= mem::size_of::<usize>());
+
+    // 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")))]

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,18 +26,39 @@ 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::<AlignedType>();
+
+
 #[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() {
         assert_eq!(TypeId::of::<types::c_int>(), TypeId::of::<raw::c_int>());
         assert_eq!(TypeId::of::<types::c_uchar>(), TypeId::of::<raw::c_uchar>());
         assert_eq!(TypeId::of::<types::c_uint>(), TypeId::of::<raw::c_uint>());
         assert_eq!(TypeId::of::<types::c_char>(), TypeId::of::<raw::c_char>());
+
+        assert!(mem::align_of::<AlignedType>() >= mem::align_of::<self::libc::max_align_t>());
     }
 }
 

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::<AlignedType>();
 
     /// Represents the set of capabilities needed for signing.
     pub enum SignOnly {}
@@ -113,26 +114,29 @@ 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);
         }
     }
 
     unsafe impl Context for VerifyOnly {
         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);
         }
     }
 
     unsafe impl Context for All {
         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<C: Context> Clone for Secp256k1<C> {
         fn clone(&self) -> Secp256k1<C> {
-            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,14 +226,14 @@ 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
     }
 }
 
 impl<'buf, C: Context + 'buf> Secp256k1<C> {
     /// 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<Secp256k1<C>, Error> {
+    pub fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<C>, Error> {
         #[cfg(target_arch = "wasm32")]
         ffi::types::sanity_checks_for_wasm();
 
@@ -241,14 +247,14 @@ impl<'buf, C: Context + 'buf> Secp256k1<C> {
                     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.
         })
     }
 }
 
 impl<'buf> Secp256k1<AllPreallocated<'buf>> {
     /// Creates a new Secp256k1 context with all capabilities
-    pub fn preallocated_new(buf: &'buf mut [u8]) -> Result<Secp256k1<AllPreallocated<'buf>>, Error> {
+    pub fn preallocated_new(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<AllPreallocated<'buf>>, Error> {
         Secp256k1::preallocated_gen_new(buf)
     }
     /// Uses the ffi `secp256k1_context_preallocated_size` to check the memory size needed for a context
@@ -271,14 +277,14 @@ impl<'buf> Secp256k1<AllPreallocated<'buf>> {
         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.
         })
     }
 }
 
 impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
     /// Creates a new Secp256k1 context that can only be used for signing
-    pub fn preallocated_signing_only(buf: &'buf mut [u8]) -> Result<Secp256k1<SignOnlyPreallocated<'buf>>, Error> {
+    pub fn preallocated_signing_only(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<SignOnlyPreallocated<'buf>>, Error> {
         Secp256k1::preallocated_gen_new(buf)
     }
 
@@ -303,14 +309,14 @@ impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
         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.
         })
     }
 }
 
 impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
     /// Creates a new Secp256k1 context that can only be used for verification
-    pub fn preallocated_verification_only(buf: &'buf mut [u8]) -> Result<Secp256k1<VerifyOnlyPreallocated<'buf>>, Error> {
+    pub fn preallocated_verification_only(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<VerifyOnlyPreallocated<'buf>>, Error> {
         Secp256k1::preallocated_gen_new(buf)
     }
 
@@ -335,7 +341,7 @@ impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
         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.
         })
     }
 }