Fully describe safety requirements #561

tcharding · 2022-12-08T01:39:43Z

Currently we have a wildcard on safety requirements, saying more or less "plus a bunch of other stuff we don't mention". This is not helpful.

Attempt to fully describe the safety requirements of creating a context from a raw context (all, signing only, and verification only).

Fix: #544

Note

This is best effort only, will require some thought to review. To do this I read https://doc.rust-lang.org/reference/behavior-considered-undefined.html and then I flicked through depend/secp256k1/src/secp256k1.c and util.h to look for things that could cause things in the linked to list of UB.

Kixunil · 2022-12-08T08:30:41Z

src/context.rs

+    /// * 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.
+    /// * `raw_ctx` must point to writable memory (cannot be `ffi::secp256k1_context_no_precomp`).


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.

I thought the second point said that, I'll improve it to clarify. Thanks

It does but claiming it's safety is wrong and it might not be clear how to drop it properly.

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.

Kixunil · 2022-12-08T08:31:40Z

src/context.rs

+    /// * 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.
+    /// * `raw_ctx` must point to writable memory (cannot be `ffi::secp256k1_context_no_precomp`).


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.

Are you trolling me with your cutnpasta? :) (I'm joking, thanks for making sure I don't only fix one instance of the docs)

Kixunil · 2022-12-08T08:34:21Z

src/context.rs

+    /// * 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.
+    /// * `raw_ctx` must point to writable memory (cannot be `ffi::secp256k1_context_no_precomp`).


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.

Kixunil · 2022-12-08T08:45:56Z

src/context.rs

    ///
+    /// This is highly unsafe due to the number of conditions that aren't checked, specifically:
+    ///
+    /// * `raw_ctx` needs to be a valid Secp256k1 context pointer that was generated by *exactly*


Pointer being generated is kinda vague. Maybe something like this:

raw_ctx must be a valid pointer (live, aligned...) to the memory that was initialized by secp256k1_context_preallocated_create (either called directly, or from this library by one of the context creation methods - all of them call it internally)

One issue with this is this promises we always call that function somehow and there won't be alternative initialization methods that are incompatible with this. I think this is reasonable assumption, right @apoelstra ?

Yep, that seems like a reasonable assumption for the indefinite future.

Cool, thanks.

Kixunil · 2022-12-08T08:46:46Z

src/context.rs

    ///
+    /// * `raw_ctx` needs to be a valid Secp256k1 context pointer that was generated by *exactly*


Same pointer comment here

Kixunil · 2022-12-08T08:58:34Z

src/context.rs

+    /// * `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.


This was written before we started fetching size from context during drop so it mandated exact flags. I think today it should be fine to require that at least the signing flag was supplied.

However to my knowledge secp256k1 asserts validity so it shouldn't lead to UB anyway? OTOH keeping it UB allows us more flexibility. But then wee should explain it's UB for flexibility and "no, we didn't document this incorrectly".

I also think that, to the extent we can get away with it, we should avoid mentioning flags since they are going to go away with the next major release. (Since upstream merged bitcoin-core/secp256k1#1126 there are basically no more flags.)

Does this mean that the size of context will stay compile-time-known forever? And having a custom mutable context is only needed for signing due to randomization?

Both true, yes :).

Cool! So in that case we could drop AlignedType and make ffi::Context to have the size and alignment same as in C using the same trick I made PR with (just twice). That'd simplify many things.

I removed the whole bullet point, commenting here to double check we are happy with that?

Kixunil · 2022-12-08T08:59:29Z

src/context.rs

    ///
+    /// * `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


Only verification may be required - same as above.

Kixunil · 2022-12-08T09:06:21Z

Also the other part of the issue is we don't provide any guarantees or restriction with regards to pointer we return. E.g. one footgun which I only noticed recently is that the context must not be moved - it's effectively Pin<SomeRef<ffi::ExternContext>> (which would be the proper type that would allow us to get rid of memory management but sadly, extern types are unstable).

This is not visibly documented in the Rust library and given how common moves are in Rust it may be even bigger footgun than in C.

apoelstra · 2022-12-08T14:28:24Z

I don't understand why contexts can't be moved. They don't have any self-referential pointers even in the C code.

Kixunil · 2022-12-08T14:37:22Z

They don't but the last time I checked the docs moving is forbidden so I assume they may in the future.

apoelstra · 2022-12-08T16:00:02Z

They don't but the last time I checked the docs moving is forbidden so I assume they may in the future.

Huh, TIL. I don't know why this warning is there -- presumably to allow for future iterations to use self-referential pointers, I suppose -- but despite the warning, it is safe (with all current versions of the C library) to do moves or copies. The warning also claims that we can't read from the memory, a requirement that makes no sense and can't be enforced in C or Rust or anywhere. So we may want to PR upstream to clarify it. The warning was added in bitcoin-core/secp256k1#566 which has a ton of discussion, including by me and Elichai.

If Pin were reasonably ergonomic to use, I'd agree with you, but because it's such a heavy cognitive burden and also causes ergonomic problems, I'd rather not. Even if we could make it work across the FFI boundary on stable Rust.

Kixunil · 2022-12-08T16:18:01Z

Oh crap, I read that wrong, it's concerning prealloc pointer not the returned one. So it's pretty much Rust borrow rules.

Maybe it's time to lift the restriction to resemble stacked borrows instead? :)

heavy cognitive burden and also causes ergonomic problems

This only concerns people working with FFI. It's a non-issue for anyone else. Even if we wanted to make bare context using Rust Box/other structures we would just have methods taking self: Pin<&mut self> which are no less ergonomic.

But still, I'd rather not to either.

tcharding · 2022-12-08T21:12:08Z

Thanks for the reviews. Will re-spin when I can give this my full concentration.

tcharding · 2022-12-11T22:59:47Z

Please re-review carefully. Note also that I've removed the duplication of the docs and instead referred folks to the docs on Secp256k1::from_raw_all to reduce the maintenance burden.

apoelstra · 2022-12-12T13:33:28Z

src/context.rs

@@ -351,15 +351,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`


In 382206f:

nit: this may lead to memory leaks should be its own sentence.

apoelstra · 2022-12-12T13:33:59Z

src/context.rs

@@ -351,15 +351,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


In 382206f:

Similarly the caller is required... should be its own sentence, or we should use a semicolon rather than comma

I used a semicolon (because it makes me feel like an English grammar guru). We now have:

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

tcharding · 2022-12-12T22:47:35Z

Force pushed changes to the docs as suggested (and posted above).

apoelstra

ACK 0bd9c96

apoelstra · 2022-12-13T15:12:12Z

Will wait on @Kixunil before merging this.

Kixunil

My brain is working at around 30% capacity now so can't say for sure but I don't see anything that's obviously wrong.

Kixunil · 2022-12-13T20:41:35Z

src/context.rs

    ///
+    /// 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


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

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

Kixunil · 2022-12-13T20:43:08Z

src/context.rs

+    ///   `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 the same as the version
+    ///   used by this library.


Is it about version (leading people to think "version number") or should it be the exact same binary? Maybe compiling with different flags could break it?

It should be the exact same binary ... I agree that "same version" understates the requirement here. But given that there are no "version numbers" for libsecp, at least not until yesterday, I don't think there's any practical way to interpret this dangerously.

I'm pretty sure I've seen version numbers whether they are "real" doesn't matter that much since people will misinterpret it.

Okay, let's change this text to clarify that the version used to create raw_ctx must be exactly the one linked into this library.

Used as suggested and force pushed, no other changes.

tcharding · 2022-12-16T00:03:30Z

Will need to be rebased on #565

apoelstra

ACK da49368

tcharding · 2023-01-30T21:29:28Z

Rebased to see if CI is working. No other changes.

apoelstra · 2023-01-30T21:46:41Z

We are getting unrelated 500 errors from a docker server now.

Kixunil · 2023-01-31T04:46:48Z

Try re-running the failed jobs, it did work in bitcoin crate.

Currently we have a wildcard on safety requirements, saying more or less "plus a bunch of other stuff we don't mention". This is not helpful. Attempt to fully describe the safety requirements of creating a context from a raw context (all, signing only, and verification only). Fix: rust-bitcoin#544

tcharding · 2023-01-31T06:21:12Z

Force push a new commit hash just to trigger CI, no changes to the PR.

tcharding · 2023-01-31T06:22:15Z

Force push a new commit hash just to trigger CI, no changes to the PR.

apoelstra

ACK e705bcf

apoelstra · 2023-01-31T13:50:58Z

@Kixunil I'd like your ACK on this before merging it.

Kixunil · 2023-02-01T05:45:37Z

src/context.rs

-    /// * `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.


This one still applies but is not mentioned in the new text.

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?

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?

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.

Sweet, my brain worked!

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.

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.

In #388 (or somewhere) we came to a solution that enables re-randomization without needing explicit context objects.

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?

@tcharding yes, the secp-sys API would remain the same.

Kixunil

ACK e705bcf

Assuming dropping of flags gets documented in the future PR.

Kixunil reviewed Dec 8, 2022

View reviewed changes

tcharding force-pushed the 12-08-safety-docs branch from ef6836d to 382206f Compare December 11, 2022 22:58

apoelstra mentioned this pull request Dec 12, 2022

Update CHANGELOG for release of v0.25.0 #550

Merged

apoelstra reviewed Dec 12, 2022

View reviewed changes

tcharding force-pushed the 12-08-safety-docs branch from 382206f to 0bd9c96 Compare December 12, 2022 22:47

apoelstra previously approved these changes Dec 13, 2022

View reviewed changes

Kixunil reviewed Dec 13, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 312cc1b December 15, 2022 23:53

tcharding force-pushed the 12-08-safety-docs branch from 0bd9c96 to 312cc1b Compare December 15, 2022 23:53

tcharding force-pushed the 12-08-safety-docs branch from 312cc1b to da49368 Compare December 21, 2022 00:52

apoelstra previously approved these changes Dec 21, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 3b2e786 January 30, 2023 21:29

tcharding force-pushed the 12-08-safety-docs branch from da49368 to 3b2e786 Compare January 30, 2023 21:29

tcharding mentioned this pull request Jan 30, 2023

Fix CI #576

Merged

tcharding force-pushed the 12-08-safety-docs branch from 3b2e786 to e705bcf Compare January 31, 2023 06:20

apoelstra approved these changes Jan 31, 2023

View reviewed changes

Kixunil requested changes Feb 1, 2023

View reviewed changes

Kixunil approved these changes Feb 5, 2023

View reviewed changes

apoelstra merged commit 6ec968a into rust-bitcoin:master Feb 6, 2023

tcharding deleted the 12-08-safety-docs branch March 15, 2023 02:40

		///
		/// * `raw_ctx` needs to be a valid Secp256k1 context pointer that was generated by exactly

Fully describe safety requirements #561

Fully describe safety requirements #561

Conversation

tcharding commented Dec 8, 2022

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Dec 8, 2022

apoelstra commented Dec 8, 2022

Kixunil commented Dec 8, 2022

apoelstra commented Dec 8, 2022

Kixunil commented Dec 8, 2022

tcharding commented Dec 8, 2022

tcharding commented Dec 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 12, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Dec 13, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 16, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jan 30, 2023

apoelstra commented Jan 30, 2023

Kixunil commented Jan 31, 2023

tcharding commented Jan 31, 2023

tcharding commented Jan 31, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment