secp256k1-sys: remove CPtr impl for &[T] and associated weirdness
#816
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In rust-bitcoin#794 I accidentally changed a *mut to *const. This is incorrect and unsound.
We take an optional `data` argument to `shared_secret`, but the upstream function we call never uses it. However, this argument *does* use the CPtr impl on &[T] in order to obtain a pointer to pass across the FFI boundary. This impl is very dangerous, and its use here is sound only because the resulting pointer is never used. See rust-bitcoin#627 (comment)
In sort_pubkeys() we convert a slice of PublicKeys to a slice of ffi::PublicKeys. This is OK, because we have repr(transparent) allowing us to do this. But we do so in an unnecessarily-complicated way, which winds up going through the CPtr implementation on &[u8], and which I think may technically be unsound. Simplify it and avoid using this impl. The next commit will remove it entirely.
|
I am equivocating on whether I should backport the last commit to secp256k1-sys 0.9.x, 0.10.x and 0.11.x. Doing so would be a breaking change (and would break rust-secp256k1, necessitating a minor release of those) and technically speaking there is no unsoundness here so it is not urgent to do so. But on the other hand, |
|
For anyone struggling to follow my story here -- checkout |
tcharding
approved these changes
Jul 1, 2025
|
This is a bit over my head. I went to read the Rustnomicon and decided I don't want to do that ATM, one day ... |
chain-forgexcr45
added a commit
to chain-forgexcr45/rust-secp256k1
that referenced
this pull request
Sep 28, 2025
…mpl for `&[T]` and associated weirdness
debae9017592c4659bb0d64294d1cdadbf8e652f secp256k1-sys: remove CPtr impl for &[T] (Andrew Poelstra)
2cef56192f8107ab4ee2c8f5e1131708769b82fc key.rs: clean up some pointer mangling in sort_pubkeys() (Andrew Poelstra)
44bcb893053d96bf5f082c6acd45e89226128300 ellswift: remove unused `data` argument from `shared_secret` (Andrew Poelstra)
2139aff0d705142ffb40883023bf47d4978dae70 secp256k1-sys: fix type signature in secp256k1_ec_pubkey_sort (Andrew Poelstra)
Pull request description:
Fixes the signature of secp256k1_ec_pubkey_sort to use `*mut` rather than `*const`.
Separately, our call to `secp256k1_ec_pubkey_sort`, while correctly starting from a `&mut [PublicKey]`, briefly constructed a `&[ffi::PublicKey]` slice before obtaining a `*mut` pointer from this. This was unsound for completely unnecessary reasons, and only compiled because we have an impl of the `CPtr` trait for `&[T]`, which provides a `as_mut_c_ptr()` method which basically cannot be safely used.
Removing that `&[T]` impl revealed another case where it was (unnecessarily) being used: in `EllSwift::shared_secret`, where we use it to obtain a `*mut` pointer from a `&[u8]`, which we pass across the FFI boundary which simply drops it.
ACKs for top commit:
tcharding:
ACK debae9017592c4659bb0d64294d1cdadbf8e652f
Tree-SHA512: 2245883cbd0695dcf495a075d530155c6f496cf7fe78c2560b567888dc5fb5f3160970c8bbd44abd67e240bccf41555fc71fd3f74daac11d446fc2b4783028ed
william2332-limf
added a commit
to william2332-limf/rust-secp256k1
that referenced
this pull request
Oct 2, 2025
…mpl for `&[T]` and associated weirdness
debae9017592c4659bb0d64294d1cdadbf8e652f secp256k1-sys: remove CPtr impl for &[T] (Andrew Poelstra)
2cef56192f8107ab4ee2c8f5e1131708769b82fc key.rs: clean up some pointer mangling in sort_pubkeys() (Andrew Poelstra)
44bcb893053d96bf5f082c6acd45e89226128300 ellswift: remove unused `data` argument from `shared_secret` (Andrew Poelstra)
2139aff0d705142ffb40883023bf47d4978dae70 secp256k1-sys: fix type signature in secp256k1_ec_pubkey_sort (Andrew Poelstra)
Pull request description:
Fixes the signature of secp256k1_ec_pubkey_sort to use `*mut` rather than `*const`.
Separately, our call to `secp256k1_ec_pubkey_sort`, while correctly starting from a `&mut [PublicKey]`, briefly constructed a `&[ffi::PublicKey]` slice before obtaining a `*mut` pointer from this. This was unsound for completely unnecessary reasons, and only compiled because we have an impl of the `CPtr` trait for `&[T]`, which provides a `as_mut_c_ptr()` method which basically cannot be safely used.
Removing that `&[T]` impl revealed another case where it was (unnecessarily) being used: in `EllSwift::shared_secret`, where we use it to obtain a `*mut` pointer from a `&[u8]`, which we pass across the FFI boundary which simply drops it.
ACKs for top commit:
tcharding:
ACK debae9017592c4659bb0d64294d1cdadbf8e652f
Tree-SHA512: 2245883cbd0695dcf495a075d530155c6f496cf7fe78c2560b567888dc5fb5f3160970c8bbd44abd67e240bccf41555fc71fd3f74daac11d446fc2b4783028ed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the signature of secp256k1_ec_pubkey_sort to use
*mutrather than*const.Separately, our call to
secp256k1_ec_pubkey_sort, while correctly starting from a&mut [PublicKey], briefly constructed a&[ffi::PublicKey]slice before obtaining a*mutpointer from this. This was unsound for completely unnecessary reasons, and only compiled because we have an impl of theCPtrtrait for&[T], which provides aas_mut_c_ptr()method which basically cannot be safely used.Removing that
&[T]impl revealed another case where it was (unnecessarily) being used: inEllSwift::shared_secret, where we use it to obtain a*mutpointer from a&[u8], which we pass across the FFI boundary which simply drops it.