-
Notifications
You must be signed in to change notification settings - Fork 280
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
Fully describe safety requirements #561
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -352,15 +352,22 @@ impl<'buf> Secp256k1<AllPreallocated<'buf>> { | |
|
||
/// Creates a context from a raw context. | ||
/// | ||
/// The returned [`core::mem::ManuallyDrop`] context will never deallocate the memory pointed to | ||
/// by `raw_ctx` nor destroy the context. This may lead to memory leaks. `ManuallyDrop::drop` | ||
/// (or [`core::ptr::drop_in_place`]) will only destroy the context; the caller is required to | ||
/// free the memory. | ||
/// | ||
/// # Safety | ||
/// This is highly unsafe, due to the number of conditions that aren't checked. | ||
/// * `raw_ctx` needs to be a valid Secp256k1 context pointer. | ||
/// that was generated by *exactly* the same code/version of the libsecp256k1 used here. | ||
/// * The capabilities (All/SignOnly/VerifyOnly) of the context *must* match the flags passed to libsecp256k1 | ||
/// when generating the context. | ||
/// * The user must handle the freeing of the context(using the correct functions) by himself. | ||
/// * Violating these may lead to Undefined Behavior. | ||
/// | ||
/// This is highly unsafe due to a number of conditions that aren't checked, specifically: | ||
/// | ||
/// * `raw_ctx` must be a valid pointer (live, aligned...) to memory that was initialized by | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should mention There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realised I never added anything on |
||
/// `secp256k1_context_preallocated_create` (either called directly or from this library by | ||
/// one of the context creation methods - all of which call it internally). | ||
/// * The version of `libsecp256k1` used to create `raw_ctx` must be **exactly the one linked | ||
/// into this library**. | ||
/// * The lifetime of the `raw_ctx` pointer must outlive `'buf`. | ||
/// * `raw_ctx` must point to writable memory (cannot be `ffi::secp256k1_context_no_precomp`). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The lifetime of Non-safety warning: the returned value will not deallocate the pointer, nor destroy the context which may lead to memory leaks. The caller is responsible for these. Note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought the second point said that, I'll improve it to clarify. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does but claiming it's safety is wrong and it might not be clear how to drop it properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 382206f: nit: I'd add the words "in particular," before "cannot be", to clarify that any non-writeable memory is a problem, not just the static context. |
||
pub unsafe fn from_raw_all( | ||
raw_ctx: NonNull<ffi::Context>, | ||
) -> ManuallyDrop<Secp256k1<AllPreallocated<'buf>>> { | ||
|
@@ -380,18 +387,11 @@ impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> { | |
#[inline] | ||
pub fn preallocate_signing_size() -> usize { Self::preallocate_size_gen() } | ||
|
||
/// Creates a context from a raw context. | ||
/// Creates a context from a raw context that can only be used for signing. | ||
/// | ||
/// # Safety | ||
/// | ||
/// This is highly unsafe, due to the number of conditions that aren't checked. | ||
/// * `raw_ctx` needs to be a valid Secp256k1 context pointer. | ||
/// that was generated by *exactly* the same code/version of the libsecp256k1 used here. | ||
/// * The capabilities (All/SignOnly/VerifyOnly) of the context *must* match the flags passed to libsecp256k1 | ||
/// when generating the context. | ||
/// * The user must handle the freeing of the context(using the correct functions) by himself. | ||
/// * This list *is not* exhaustive, and any violation may lead to Undefined Behavior. | ||
/// | ||
/// Please see [`Secp256k1::from_raw_all`] for full documentation and safety requirements. | ||
pub unsafe fn from_raw_signing_only( | ||
raw_ctx: NonNull<ffi::Context>, | ||
) -> ManuallyDrop<Secp256k1<SignOnlyPreallocated<'buf>>> { | ||
|
@@ -411,18 +411,11 @@ impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> { | |
#[inline] | ||
pub fn preallocate_verification_size() -> usize { Self::preallocate_size_gen() } | ||
|
||
/// Creates a context from a raw context. | ||
/// Creates a context from a raw context that can only be used for verification. | ||
/// | ||
/// # Safety | ||
/// | ||
/// This is highly unsafe, due to the number of conditions that aren't checked. | ||
/// * `raw_ctx` needs to be a valid Secp256k1 context pointer. | ||
/// that was generated by *exactly* the same code/version of the libsecp256k1 used here. | ||
/// * The capabilities (All/SignOnly/VerifyOnly) of the context *must* match the flags passed to libsecp256k1 | ||
/// when generating the context. | ||
/// * The user must handle the freeing of the context(using the correct functions) by himself. | ||
/// * This list *is not* exhaustive, and any violation may lead to Undefined Behavior. | ||
/// | ||
/// Please see [`Secp256k1::from_raw_all`] for full documentation and safety requirements. | ||
pub unsafe fn from_raw_verification_only( | ||
raw_ctx: NonNull<ffi::Context>, | ||
) -> ManuallyDrop<Secp256k1<VerifyOnlyPreallocated<'buf>>> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one still applies but is not mentioned in the new text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember why I removed this exactly because it was a long time ago but it seems such an obvious omission that I think me from the past would of had a reason. I'm out of my depth of knowledge here but I thought there was something about the libsecp context not differentiating any more? Are you sure we still want this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at
gen_new
I don't see that the libsecp context created is any different for the different All/SignOnly/VerifyOnly tags, or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, there is now no difference upstream between the capabilities, they're total no-ops. But we haven't updated the Rust API to reflect this yet because it'd be a big breaking change and we wanted to make sure upstream was finished changing first.
So I think this text is ok to drop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, my brain worked!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Upstream had an actual release :).
Another strategy might be to stop exposing context objects in the API at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need them for re-randomization, don't we?
Anyway, I prefer the API to be modified a little to not being modified at all because of the amount of work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #388 (or somewhere) we came to a solution that enables re-randomization without needing explicit context objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we use a global singleton for the context and leave the
secp256k1-sys
API the same?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tcharding yes, the secp-sys API would remain the same.