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

Introduce new error handling convention #635

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 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
65 changes: 52 additions & 13 deletions src/context.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: CC0-1.0

use core::fmt;
use core::marker::PhantomData;
use core::mem::ManuallyDrop;
use core::ptr::NonNull;
Expand All @@ -8,7 +9,7 @@ use core::ptr::NonNull;
pub use self::alloc_only::*;
use crate::ffi::types::{c_uint, c_void, AlignedType};
use crate::ffi::{self, CPtr};
use crate::{Error, Secp256k1};
use crate::Secp256k1;

#[cfg(all(feature = "global-context", feature = "std"))]
/// Module implementing a singleton pattern for a global `Secp256k1` context.
Expand Down Expand Up @@ -320,30 +321,51 @@ unsafe impl<'buf> PreallocatedContext<'buf> for AllPreallocated<'buf> {}
unsafe impl<'buf> PreallocatedContext<'buf> for SignOnlyPreallocated<'buf> {}
unsafe impl<'buf> PreallocatedContext<'buf> for VerifyOnlyPreallocated<'buf> {}

/// A preallocated buffer, enforces the invariant that the buffer is big enough.
#[allow(missing_debug_implementations)]
pub struct PreallocatedBuffer<'buf>(&'buf [AlignedType]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be mut as it's unsound otherwise.

Also FYI I probably meant something else but can't remember what it was. I guess I meant &mut AlignedType which is unsound. We could also just use unsized type pub struct PreallocatedBuffer([AligneType]); which is more idiomatic.

In ideal world we would use an extern type but those aren't even stable.

Copy link
Member

Choose a reason for hiding this comment

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

We should also probably make AlignedType contain MaybeUninit or something like that, as the C code might write padding bytes to it.

I think it wasn't stable (enough) back when I implemented this logic

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it wasn't stable back then. Definitely should. I promoted this to #707 because I expect this old PR to get closed.


impl<'buf> ffi::CPtr for PreallocatedBuffer<'buf> {
type Target = AlignedType;
fn as_c_ptr(&self) -> *const Self::Target { self.0.as_c_ptr() }
fn as_mut_c_ptr(&mut self) -> *mut Self::Target { self.0.as_mut_c_ptr() }
}

impl<'buf, C: Context + PreallocatedContext<'buf>> Secp256k1<C> {
/// Wraps `buf` in a `PreallocatedBuffer`.
///
/// # Errors
///
/// Returns `NotEnoughMemoryError` if the buffer is too small.
pub fn buffer(
buf: &'buf mut [AlignedType],
) -> Result<PreallocatedBuffer, NotEnoughMemoryError> {
if buf.len() < Self::preallocate_size_gen() {
return Err(NotEnoughMemoryError);
}
Ok(PreallocatedBuffer(buf))
}
Copy link
Collaborator

@Kixunil Kixunil Nov 10, 2023

Choose a reason for hiding this comment

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

This also should have an unsafe constructor to initialize it from pointer and size.

See also #665


/// Lets you create a context with a preallocated buffer in a generic manner (sign/verify/all).
pub fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<C>, Error> {
pub fn preallocated_gen_new(buf: &'buf mut PreallocatedBuffer) -> Secp256k1<C> {
#[cfg(target_arch = "wasm32")]
ffi::types::sanity_checks_for_wasm();

if buf.len() < Self::preallocate_size_gen() {
return Err(Error::NotEnoughMemory);
}
// Safe because buf is not null since it is not empty.
let buf = unsafe { NonNull::new_unchecked(buf.as_mut_c_ptr() as *mut c_void) };

Ok(Secp256k1 {
Secp256k1 {
ctx: unsafe { ffi::secp256k1_context_preallocated_create(buf, AllPreallocated::FLAGS) },
phantom: PhantomData,
})
}
}
}

impl<'buf> Secp256k1<AllPreallocated<'buf>> {
/// Creates a new Secp256k1 context with all capabilities.
pub fn preallocated_new(
buf: &'buf mut [AlignedType],
) -> Result<Secp256k1<AllPreallocated<'buf>>, Error> {
buf: &'buf mut PreallocatedBuffer<'buf>,
) -> Secp256k1<AllPreallocated<'buf>> {
Secp256k1::preallocated_gen_new(buf)
}
/// Uses the ffi `secp256k1_context_preallocated_size` to check the memory size needed for a context.
Expand Down Expand Up @@ -377,8 +399,8 @@ impl<'buf> Secp256k1<AllPreallocated<'buf>> {
impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
/// Creates a new Secp256k1 context that can only be used for signing.
pub fn preallocated_signing_only(
buf: &'buf mut [AlignedType],
) -> Result<Secp256k1<SignOnlyPreallocated<'buf>>, Error> {
buf: &'buf mut PreallocatedBuffer<'buf>,
) -> Secp256k1<SignOnlyPreallocated<'buf>> {
Secp256k1::preallocated_gen_new(buf)
}

Expand All @@ -401,8 +423,8 @@ impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
/// Creates a new Secp256k1 context that can only be used for verification
pub fn preallocated_verification_only(
buf: &'buf mut [AlignedType],
) -> Result<Secp256k1<VerifyOnlyPreallocated<'buf>>, Error> {
buf: &'buf mut PreallocatedBuffer<'buf>,
) -> Secp256k1<VerifyOnlyPreallocated<'buf>> {
Secp256k1::preallocated_gen_new(buf)
}

Expand All @@ -421,3 +443,20 @@ impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
ManuallyDrop::new(Secp256k1 { ctx: raw_ctx, phantom: PhantomData })
}
}

/// Not enough memory for a preallocated buffer.
#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
#[allow(missing_copy_implementations)] // Don't implement Copy when we use non_exhaustive.
pub struct NotEnoughMemoryError;

impl fmt::Display for NotEnoughMemoryError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("not enough memory to use as a preallocated buffer")
}
}

#[cfg(feature = "std")]
impl std::error::Error for NotEnoughMemoryError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }
}
8 changes: 4 additions & 4 deletions src/ecdh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use secp256k1_sys::types::{c_int, c_uchar, c_void};

use crate::ffi::{self, CPtr};
use crate::key::{PublicKey, SecretKey};
use crate::{constants, Error};
use crate::{constants, hex, Error};

// The logic for displaying shared secrets relies on this (see `secret.rs`).
const SHARED_SECRET_SIZE: usize = constants::SECRET_KEY_SIZE;
Expand Down Expand Up @@ -79,9 +79,9 @@ impl SharedSecret {

impl str::FromStr for SharedSecret {
type Err = Error;
fn from_str(s: &str) -> Result<SharedSecret, Error> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
let mut res = [0u8; SHARED_SECRET_SIZE];
match crate::from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(SHARED_SECRET_SIZE) => Ok(SharedSecret::from_bytes(res)),
_ => Err(Error::InvalidSharedSecret),
}
Expand Down Expand Up @@ -160,7 +160,7 @@ impl ::serde::Serialize for SharedSecret {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
let mut buf = [0u8; SHARED_SECRET_SIZE * 2];
s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
s.serialize_str(hex::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
} else {
s.serialize_bytes(self.as_ref())
}
Expand Down
8 changes: 3 additions & 5 deletions src/ecdsa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ pub use self::serialized_signature::SerializedSignature;
use crate::ffi::CPtr;
#[cfg(feature = "global-context")]
use crate::SECP256K1;
use crate::{
ffi, from_hex, Error, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification,
};
use crate::{ffi, hex, Error, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification};

/// An ECDSA signature
#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
Expand All @@ -37,9 +35,9 @@ impl fmt::Display for Signature {

impl str::FromStr for Signature {
type Err = Error;
fn from_str(s: &str) -> Result<Signature, Error> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
let mut res = [0u8; 72];
match from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(x) => Signature::from_der(&res[0..x]),
_ => Err(Error::InvalidSignature),
}
Expand Down
11 changes: 6 additions & 5 deletions src/ellswift.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use core::str::FromStr;
use ffi::CPtr;
use secp256k1_sys::types::{c_int, c_uchar, c_void};

use crate::{constants, ffi, from_hex, Error, PublicKey, Secp256k1, SecretKey, Verification};
use crate::{constants, ffi, hex, Error, PublicKey, Secp256k1, SecretKey, Verification};

unsafe extern "C" fn hash_callback<F>(
output: *mut c_uchar,
Expand Down Expand Up @@ -288,15 +288,16 @@ impl ElligatorSwiftParty {
}

impl FromStr for ElligatorSwift {
type Err = Error;

fn from_str(hex: &str) -> Result<Self, Self::Err> {
let mut ser = [0u8; 64];
let parsed = from_hex(hex, &mut ser);
let parsed = hex::from_hex(hex, &mut ser);
match parsed {
Ok(64) => Ok(ElligatorSwift::from_array(ser)),
_ => Err(Error::InvalidEllSwift),
}
}
type Err = Error;
}

impl fmt::LowerHex for ElligatorSwift {
Expand Down Expand Up @@ -328,7 +329,7 @@ mod tests {
use crate::ellswift::{ElligatorSwiftParty, ElligatorSwiftSharedSecret};
#[cfg(all(not(secp256k1_fuzz), feature = "alloc"))]
use crate::SecretKey;
use crate::{from_hex, PublicKey, XOnlyPublicKey};
use crate::{hex, PublicKey, XOnlyPublicKey};

#[test]
#[cfg(all(not(secp256k1_fuzz), feature = "alloc"))]
Expand Down Expand Up @@ -603,7 +604,7 @@ mod tests {
#[inline]
fn parse_test(ell: &str, x: &str, parity: u32) -> EllswiftDecodeTest {
let mut enc = [0u8; 64];
from_hex(ell, &mut enc).unwrap();
hex::from_hex(ell, &mut enc).unwrap();
let xo = XOnlyPublicKey::from_str(x).unwrap();
let parity = if parity == 0 { crate::Parity::Even } else { crate::Parity::Odd };
let pk = PublicKey::from_x_only_public_key(xo, parity);
Expand Down
54 changes: 54 additions & 0 deletions src/hex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// SPDX-License-Identifier: CC0-1.0

//! Conversion to and from hexadecimal strings.

use core::str;

/// Utility function used to parse hex into a target u8 buffer. Returns
/// the number of bytes converted or an error if it encounters an invalid
/// character or unexpected end of string.
pub(crate) fn from_hex(hex: &str, target: &mut [u8]) -> Result<usize, ()> {
if hex.len() % 2 == 1 || hex.len() > target.len() * 2 {
return Err(());
}

let mut b = 0;
let mut idx = 0;
for c in hex.bytes() {
b <<= 4;
match c {
b'A'..=b'F' => b |= c - b'A' + 10,
b'a'..=b'f' => b |= c - b'a' + 10,
b'0'..=b'9' => b |= c - b'0',
_ => return Err(()),
}
if (idx & 1) == 1 {
target[idx / 2] = b;
b = 0;
}
idx += 1;
}
Ok(idx / 2)
}

/// Utility function used to encode hex into a target u8 buffer. Returns
/// a reference to the target buffer as an str. Returns an error if the target
/// buffer isn't big enough.
#[inline]
pub(crate) fn to_hex<'a>(src: &[u8], target: &'a mut [u8]) -> Result<&'a str, ()> {
let hex_len = src.len() * 2;
if target.len() < hex_len {
return Err(());
}
const HEX_TABLE: [u8; 16] = *b"0123456789abcdef";

let mut i = 0;
for &b in src {
target[i] = HEX_TABLE[usize::from(b >> 4)];
target[i + 1] = HEX_TABLE[usize::from(b & 0b00001111)];
i += 2;
}
let result = &target[..hex_len];
debug_assert!(str::from_utf8(result).is_ok());
return unsafe { Ok(str::from_utf8_unchecked(result)) };
}
31 changes: 14 additions & 17 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ use crate::ffi::{self, CPtr};
use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey};
#[cfg(feature = "global-context")]
use crate::SECP256K1;
use crate::{
constants, ecdsa, from_hex, schnorr, Message, Scalar, Secp256k1, Signing, Verification,
};
use crate::{constants, ecdsa, hex, schnorr, Message, Scalar, Secp256k1, Signing, Verification};
#[cfg(feature = "hashes")]
use crate::{hashes, ThirtyTwoByteHash};

Expand Down Expand Up @@ -112,9 +110,9 @@ impl ffi::CPtr for SecretKey {

impl str::FromStr for SecretKey {
type Err = Error;
fn from_str(s: &str) -> Result<SecretKey, Error> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
let mut res = [0u8; constants::SECRET_KEY_SIZE];
match from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_slice(&res),
_ => Err(Error::InvalidSecretKey),
}
Expand Down Expand Up @@ -165,9 +163,9 @@ impl fmt::Display for PublicKey {

impl str::FromStr for PublicKey {
type Err = Error;
fn from_str(s: &str) -> Result<PublicKey, Error> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
let mut res = [0u8; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(constants::PUBLIC_KEY_SIZE) =>
PublicKey::from_slice(&res[0..constants::PUBLIC_KEY_SIZE]),
Ok(constants::UNCOMPRESSED_PUBLIC_KEY_SIZE) => PublicKey::from_slice(&res),
Expand Down Expand Up @@ -385,7 +383,7 @@ impl serde::Serialize for SecretKey {
fn serialize<S: serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
s.serialize_str(hex::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
} else {
let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
for byte in self.0.iter() {
Expand Down Expand Up @@ -859,7 +857,7 @@ impl Keypair {
#[inline]
pub fn from_seckey_str<C: Signing>(secp: &Secp256k1<C>, s: &str) -> Result<Keypair, Error> {
let mut res = [0u8; constants::SECRET_KEY_SIZE];
match from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(constants::SECRET_KEY_SIZE) =>
Keypair::from_seckey_slice(secp, &res[0..constants::SECRET_KEY_SIZE]),
_ => Err(Error::InvalidPublicKey),
Expand Down Expand Up @@ -1041,8 +1039,7 @@ impl serde::Serialize for Keypair {
if s.is_human_readable() {
let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
s.serialize_str(
crate::to_hex(&self.secret_bytes(), &mut buf)
.expect("fixed-size hex serialization"),
hex::to_hex(&self.secret_bytes(), &mut buf).expect("fixed-size hex serialization"),
)
} else {
let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
Expand Down Expand Up @@ -1132,9 +1129,9 @@ impl fmt::Display for XOnlyPublicKey {

impl str::FromStr for XOnlyPublicKey {
type Err = Error;
fn from_str(s: &str) -> Result<XOnlyPublicKey, Error> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
let mut res = [0u8; constants::SCHNORR_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
match hex::from_hex(s, &mut res) {
Ok(constants::SCHNORR_PUBLIC_KEY_SIZE) =>
XOnlyPublicKey::from_slice(&res[0..constants::SCHNORR_PUBLIC_KEY_SIZE]),
_ => Err(Error::InvalidPublicKey),
Expand Down Expand Up @@ -1559,13 +1556,13 @@ mod test {

use super::{Keypair, Parity, PublicKey, Secp256k1, SecretKey, XOnlyPublicKey, *};
use crate::Error::{InvalidPublicKey, InvalidSecretKey};
use crate::{constants, from_hex, to_hex, Scalar};
use crate::{constants, hex, Scalar};

#[cfg(not(secp256k1_fuzz))]
macro_rules! hex {
($hex:expr) => {{
let mut result = vec![0; $hex.len() / 2];
from_hex($hex, &mut result).expect("valid hex string");
hex::from_hex($hex, &mut result).expect("valid hex string");
result
}};
}
Expand Down Expand Up @@ -1745,7 +1742,7 @@ mod test {

let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
assert_eq!(
to_hex(&sk[..], &mut buf).unwrap(),
hex::to_hex(&sk[..], &mut buf).unwrap(),
"0100000000000000020000000000000003000000000000000400000000000000"
);
}
Expand Down Expand Up @@ -2422,7 +2419,7 @@ mod test {
let ctx = crate::Secp256k1::new();
let keypair = Keypair::new(&ctx, &mut rand::thread_rng());
let mut buf = [0_u8; constants::SECRET_KEY_SIZE * 2]; // Holds hex digits.
let s = to_hex(&keypair.secret_key().secret_bytes(), &mut buf).unwrap();
let s = hex::to_hex(&keypair.secret_key().secret_bytes(), &mut buf).unwrap();
let parsed_key = Keypair::from_str(s).unwrap();
assert_eq!(parsed_key, keypair);
}
Expand Down
Loading