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

Fully describe safety requirements #561

Merged
merged 1 commit into from
Feb 6, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 18 additions & 25 deletions src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sweet, my brain worked!

Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to?

No. Upstream had an actual release :).

Maybe we could start with using All as the default type parameter and change the generic functions to accept any type parameter and deprecate the All, SignOnly and VerifyOnly types. This is not breaking but reflects it in the API.

Another strategy might be to stop exposing context objects in the API at all.

Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another strategy might be to stop exposing context objects in the API at all.

Would we use a global singleton for the context and leave the secp256k1-sys API the same?

Copy link
Member

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.

/// * 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should mention AlignedType as the one providing the correct alignment information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realised I never added anything on AlignedType as you suggest here.

/// `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`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The lifetime of raw_ctx pointer must outlive 'buf.

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 ManuallyDrop::drop (or drop_in_place) will only destroy the context, it will not free memory behind the pointer.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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>>> {
Expand All @@ -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>>> {
Expand All @@ -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>>> {
Expand Down