Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up once code #359

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
124 changes: 90 additions & 34 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,6 @@ impl<T> CPtr for [T] {
#[cfg(fuzzing)]
mod fuzz_dummy {
use super::*;
use core::sync::atomic::{AtomicUsize, Ordering};

#[cfg(rust_secp_no_symbol_renaming)] compile_error!("We do not support fuzzing with rust_secp_no_symbol_renaming");

Expand All @@ -673,60 +672,117 @@ mod fuzz_dummy {
fn rustsecp256k1_v0_4_1_context_preallocated_clone(cx: *const Context, prealloc: *mut c_void) -> *mut Context;
}

mod once {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some kind of descriptive comment describing why its here, that it's only used in fuzzing, and that we don't really care too much...................all that said, maybe we should care a bit more, I realized yesterday that global-context-less-secure relies on std, even though the whole point of it is to avoid relying on std (it predates no-std so that's expected, but still). It only actually relies on std::sync::Once, so maybe we should use this there as well. Not sure how to get it across both crates, though, maybe just make a single file and symlink them across the two crates to avoid making it public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think it should condinionaly use Once from std as suggested in #352

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of the global-context-less-secure feature is to work on no-std, and sadly Once is not no-std as it requires thread-parking support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since cargo features are additive it's still useful to avoid performance issues if two crates turn on these features. Although maybe they should've been --cfg parameters, not features...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I misread your comment again lol. Yes, conditionally using Once if std is turned on is fine, but we still need to somehow have this once available in secp256k1 if we want to fix global-context-less-secure not building with no-std.

use core::sync::atomic::{AtomicUsize, Ordering};

const NONE: usize = 0;
const WORKING: usize = 1;
const DONE: usize = 2;

pub(crate) struct Once(AtomicUsize);

impl Once {
pub(crate) const INIT: Once = Once(AtomicUsize::new(NONE));

pub(crate) fn run(&self, f: impl FnOnce()) {
// Acquire ordering because if it's DONE the following reads must go after this load.
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
let mut have_ctx = self.0.load(Ordering::Acquire);
if have_ctx == NONE {
// Ordering: on success we're only signalling to other thread that work is in progress
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
// without transferring other data, so it should be Relaxed, on failure the value may be DONE,
// so we want to Acquire to safely proceed in that case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 'so we want Acquire to safely proceed ...'?

// However compare_exchange doesn't allow failure ordering to be stronger than success
// so both are Acquire.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, this whole comment might look better like this (includes change above):

                    // Ordering: on success we're only signalling to other thread that work is in
                    // progress without transferring other data, so it should be Relaxed, on failure
                    // the value may be DONE, so we want Acquire to safely proceed in that case.
                    // However compare_exchange doesn't allow failure ordering to be stronger than
                    // success so both are Acquire.

match self.0.compare_exchange(NONE, WORKING, Ordering::Acquire, Ordering::Acquire) {
Ok(_) => {
f();
// We wrote data in memory that other threads may read so we need Release
self.0.store(DONE, Ordering::Release);
return;
},
Err(value) => have_ctx = value,
}
}
while have_ctx != DONE {
// Another thread is building, just busy-loop until they're done.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to the whole loop, right? Perhaps clearer if its outside the loop (above the while line).

assert_eq!(have_ctx, WORKING);
// This thread will read whatever the other thread wrote so this needs to be Acquire.
have_ctx = self.0.load(Ordering::Acquire);
#[cfg(feature = "std")]
::std::thread::yield_now();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR yield_now is not great. Futex would be the best, cond var as a fallback.

}
}
}

#[cfg(all(test, feature = "std"))]
mod tests {
use super::Once;

#[test]
fn test_once() {
use std::cell::UnsafeCell;
static ONCE: Once = Once::INIT;
struct PretendSync(UnsafeCell<u32>);

static VALUE: PretendSync = PretendSync(UnsafeCell::new(42));
unsafe impl Sync for PretendSync {}

let threads = (0..5).map(|_| ::std::thread::spawn(|| {
ONCE.run(|| unsafe {
assert_eq!(*VALUE.0.get(), 42);
*VALUE.0.get() = 47;
Kixunil marked this conversation as resolved.
Show resolved Hide resolved
});
unsafe {
assert_eq!(*VALUE.0.get(), 47);
}
}))
.collect::<Vec<_>>();
for thread in threads {
thread.join().unwrap();
}
unsafe {
assert_eq!(*VALUE.0.get(), 47);
}
}
}
}

#[cfg(feature = "lowmemory")]
const CTX_SIZE: usize = 1024 * 65;
#[cfg(not(feature = "lowmemory"))]
const CTX_SIZE: usize = 1024 * (1024 + 128);
// Contexts
pub unsafe fn secp256k1_context_preallocated_size(flags: c_uint) -> size_t {
assert!(rustsecp256k1_v0_4_1_context_preallocated_size(flags) + std::mem::size_of::<c_uint>() <= CTX_SIZE);
assert!(rustsecp256k1_v0_4_1_context_preallocated_size(flags) + core::mem::size_of::<c_uint>() <= CTX_SIZE);
CTX_SIZE
}

static HAVE_PREALLOCATED_CONTEXT: AtomicUsize = AtomicUsize::new(0);
const HAVE_CONTEXT_NONE: usize = 0;
const HAVE_CONTEXT_WORKING: usize = 1;
const HAVE_CONTEXT_DONE: usize = 2;
static HAVE_PREALLOCATED_CONTEXT: once::Once = once::Once::INIT;
static mut PREALLOCATED_CONTEXT: [u8; CTX_SIZE] = [0; CTX_SIZE];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized this variable is very wrong due to alignment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so the fix should be [AlignedType; CTX_SIZE / mem::size_of::<AlignedType>()], right? Although maybe we should have a separate type for that.

Now looking at ffi::Context and it's an int. WTF?

Copy link
Member

@elichai elichai Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk what CTX_SIZE is and how was it calculated.
we can't have a type for that because we don't know the size of the context in compile time.
although everything here won't be needed when we start supporting the static contexts

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't know the size of the context in compile time.

I think we can know the maximum and guard against it changing by calling secp256k1_context_preallocated_size

pub unsafe fn secp256k1_context_preallocated_create(prealloc: *mut c_void, flags: c_uint) -> *mut 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
// new buffers instead of recalculating it. Because we shouldn't rely on std, we use a
// simple hand-written OnceFlag built out of an atomic to gate the global static.
let mut have_ctx = HAVE_PREALLOCATED_CONTEXT.load(Ordering::Relaxed);
while have_ctx != HAVE_CONTEXT_DONE {
if have_ctx == HAVE_CONTEXT_NONE {
have_ctx = HAVE_PREALLOCATED_CONTEXT.swap(HAVE_CONTEXT_WORKING, Ordering::AcqRel);
if have_ctx == HAVE_CONTEXT_NONE {
assert!(rustsecp256k1_v0_4_1_context_preallocated_size(SECP256K1_START_SIGN | SECP256K1_START_VERIFY) + std::mem::size_of::<c_uint>() <= CTX_SIZE);
assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(
PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void,
SECP256K1_START_SIGN | SECP256K1_START_VERIFY),
PREALLOCATED_CONTEXT[..].as_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 {
// Another thread finished while we were swapping.
HAVE_PREALLOCATED_CONTEXT.store(HAVE_CONTEXT_DONE, Ordering::Release);
}
} else {
// Another thread is building, just busy-loop until they're done.
assert_eq!(have_ctx, HAVE_CONTEXT_WORKING);
have_ctx = HAVE_PREALLOCATED_CONTEXT.load(Ordering::Acquire);
#[cfg(feature = "std")]
std::thread::yield_now();
}
}

HAVE_PREALLOCATED_CONTEXT.run(|| {
assert!(rustsecp256k1_v0_4_1_context_preallocated_size(SECP256K1_START_SIGN | SECP256K1_START_VERIFY) + core::mem::size_of::<c_uint>() <= CTX_SIZE);
assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(
PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void,
SECP256K1_START_SIGN | SECP256K1_START_VERIFY),
PREALLOCATED_CONTEXT[..].as_ptr() as *mut Context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assign this pointer to a local variable?

            let ptr = PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void;
            assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(ptr, SECP256K1_START_SIGN | SECP256K1_START_VERIFY), ptr);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are different types. Which is strange. It'd still be shorter to cast one of them but would be better to look if we can unify them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear, I missed the type difference :(

});

ptr::copy_nonoverlapping(PREALLOCATED_CONTEXT[..].as_ptr(), prealloc 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 *mut u8).add(CTX_SIZE).sub(core::mem::size_of::<c_uint>());
(ptr as *mut c_uint).write(flags);
prealloc 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 {
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 orig_ptr = (cx as *mut u8).add(CTX_SIZE).sub(core::mem::size_of::<c_uint>());
let new_ptr = (prealloc as *mut u8).add(CTX_SIZE).sub(core::mem::size_of::<c_uint>());
let flags = (orig_ptr as *mut c_uint).read();
(new_ptr as *mut c_uint).write(flags);
rustsecp256k1_v0_4_1_context_preallocated_clone(cx, prealloc)
Expand All @@ -745,7 +801,7 @@ mod fuzz_dummy {
let cx_flags = if cx == secp256k1_context_no_precomp {
1
} else {
let ptr = (cx as *const u8).add(CTX_SIZE).sub(std::mem::size_of::<c_uint>());
let ptr = (cx as *const u8).add(CTX_SIZE).sub(core::mem::size_of::<c_uint>());
(ptr as *const c_uint).read()
};
assert_eq!(cx_flags & 1, 1); // SECP256K1_FLAGS_TYPE_CONTEXT
Expand Down