diff --git a/STYLE.md b/STYLE.md index 64256527b1..af5a3094b9 100644 --- a/STYLE.md +++ b/STYLE.md @@ -91,29 +91,33 @@ already a safer function for doing the conversion in [ring::polyfill](src/polyfill.rs). If not, add one to `ring::polyfill`. The C code generally uses the C `int` type as a return value, where 1 indicates -success and 0 indicates failure. Sometimes the C code has functions that return -pointers, and a NULL pointer indicates failure. The module -[ring::bssl](src/bssl.rs) contains some utilities for mapping these return -values to `Result<(), ()>` and `Result<*mut T, ()>`, respectively. They should -be used as in the following example (note the placement of `unsafe`): +success and 0 indicates failure. The module [ring::bssl](src/bssl.rs) contains +a [transparent] `Result` type which should be used as the return type when +declaring foreign functions which follow this convention. A +`ring::bssl::Result` should be converted to a `std::result::Result` using the +pattern in the following example (note the placement of `unsafe`): + +[transparent]: https://doc.rust-lang.org/nightly/reference/type-layout.html#the-transparent-representation + ```rust +extern { + unsafe_fn1() -> bssl::Result; + /* ... */ +} + fn foo() -> Result<(), ()> { - try!(bssl::map_result(unsafe { + Result::from(unsafe { unsafe_fn2(when, the, entire, thing, does, not, fit, on, a, single, line) - })); + })?; - try!(bssl::map_result(unsafe { + Result::from(unsafe { unsafe_fn1() // Use the same style even when the call fits on one line. - })); - - let ptr = try!(bssl::map_ptr_result(unsafe { - unsafe_fn_returning_pointer() - })); + })?; // The return value of `foo` will be the mapped result of calling // `unsafe_fn3`. - bssl::map_result(unsafe { + Result::from(unsafe { unsafe_fn3() }) } diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index 350a392f36..e0fca7f1b7 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -132,6 +132,8 @@ int GFp_vpaes_set_encrypt_key(const uint8_t *userKey, unsigned bits, void GFp_vpaes_encrypt(const uint8_t *in, uint8_t *out, const AES_KEY *key); #endif +// XXX: Returns zero on success, violating the return value convention. +// TODO: should return void anyway, as it can only fail if passed a null pointer. int GFp_AES_set_encrypt_key(const uint8_t *user_key, unsigned bits, AES_KEY *key) { // Keep this in sync with |gcm128_init_gmult_ghash| and |aes_ctr|. diff --git a/src/aead/aes_gcm.rs b/src/aead/aes_gcm.rs index 50d2082e62..1c3e758ce5 100644 --- a/src/aead/aes_gcm.rs +++ b/src/aead/aes_gcm.rs @@ -44,7 +44,7 @@ pub static AES_256_GCM: aead::Algorithm = aead::Algorithm { fn aes_gcm_init(ctx_buf: &mut [u8], key: &[u8]) -> Result<(), error::Unspecified> { - bssl::map_result(unsafe { + Result::from(unsafe { GFp_aes_gcm_init(ctx_buf.as_mut_ptr(), ctx_buf.len(), key.as_ptr(), key.len()) }) @@ -55,7 +55,7 @@ fn aes_gcm_seal(ctx: &[u64; aead::KEY_CTX_BUF_ELEMS], tag: &mut [u8; aead::TAG_LEN]) -> Result<(), error::Unspecified> { let ctx = polyfill::slice::u64_as_u8(ctx); - bssl::map_result(unsafe { + Result::from(unsafe { GFp_aes_gcm_seal(ctx.as_ptr(), in_out.as_mut_ptr(), in_out.len(), tag, nonce, ad.as_ptr(), ad.len()) }) @@ -66,7 +66,7 @@ fn aes_gcm_open(ctx: &[u64; aead::KEY_CTX_BUF_ELEMS], in_out: &mut [u8], tag_out: &mut [u8; aead::TAG_LEN]) -> Result<(), error::Unspecified> { let ctx = polyfill::slice::u64_as_u8(ctx); - bssl::map_result(unsafe { + Result::from(unsafe { GFp_aes_gcm_open(ctx.as_ptr(), in_out.as_mut_ptr(), in_out.len() - in_prefix_len, tag_out, nonce, in_out[in_prefix_len..].as_ptr(), ad.as_ptr(), @@ -99,19 +99,19 @@ const GCM128_SERIALIZED_LEN: usize = 16 * 16; extern { fn GFp_aes_gcm_init(ctx_buf: *mut u8, ctx_buf_len: c::size_t, - key: *const u8, key_len: c::size_t) -> c::int; + key: *const u8, key_len: c::size_t) -> bssl::Result; fn GFp_aes_gcm_seal(ctx_buf: *const u8, in_out: *mut u8, in_out_len: c::size_t, tag_out: &mut [u8; aead::TAG_LEN], nonce: &[u8; aead::NONCE_LEN], ad: *const u8, - ad_len: c::size_t) -> c::int; + ad_len: c::size_t) -> bssl::Result; fn GFp_aes_gcm_open(ctx_buf: *const u8, out: *mut u8, in_out_len: c::size_t, tag_out: &mut [u8; aead::TAG_LEN], nonce: &[u8; aead::NONCE_LEN], in_: *const u8, - ad: *const u8, ad_len: c::size_t) -> c::int; + ad: *const u8, ad_len: c::size_t) -> bssl::Result; } @@ -172,6 +172,8 @@ mod tests { } extern "C" { + // XXX: This function does not adhere to the return value conventions. + #[must_use] fn GFp_AES_set_encrypt_key(key: *const u8, bits: usize, aes_key: *mut AES_KEY) -> c::int; fn GFp_AES_encrypt(in_: *const u8, out: *mut u8, key: *const AES_KEY); diff --git a/src/bssl.rs b/src/bssl.rs index eb03f7f76a..b157ee7381 100644 --- a/src/bssl.rs +++ b/src/bssl.rs @@ -14,10 +14,22 @@ use {c, error}; -pub fn map_result(bssl_result: c::int) -> Result<(), error::Unspecified> { - match bssl_result { - 1 => Ok(()), - _ => Err(error::Unspecified), +/// A `c::int` returned from a foreign function containing **1** if the function was successful +/// or **0** if an error occurred. This is the convention used by C code in `ring`. +#[derive(Clone, Copy, Debug)] +#[must_use] +#[repr(transparent)] +pub struct Result(c::int); + +impl From for ::std::result::Result<(), error::Unspecified> { + fn from(ret: Result) -> Self { + match ret.0 { + 1 => Ok(()), + c => { + debug_assert_eq!(c, 0, "`bssl::Result` value must be 0 or 1"); + Err(error::Unspecified) + }, + } } } @@ -34,6 +46,7 @@ macro_rules! bssl_test { fn $fn_name() { use $crate::{c, init}; extern { + #[must_use] fn $bssl_test_main_fn_name() -> c::int; } @@ -47,3 +60,24 @@ macro_rules! bssl_test { } } } + +#[cfg(test)] +mod tests { + mod result { + use {bssl, c}; + use core::mem; + + #[test] + fn size_and_alignment() { + type Underlying = c::int; + assert_eq!(mem::size_of::(), mem::size_of::()); + assert_eq!(mem::align_of::(), mem::align_of::()); + } + + #[test] + fn semantics() { + assert!(Result::from(bssl::Result(0)).is_err()); + assert!(Result::from(bssl::Result(1)).is_ok()); + } + } +} diff --git a/src/ec/curve25519/ops.rs b/src/ec/curve25519/ops.rs index 344053ff33..06e91eebd8 100644 --- a/src/ec/curve25519/ops.rs +++ b/src/ec/curve25519/ops.rs @@ -15,7 +15,7 @@ //! Elliptic curve operations on the birationally equivalent curves Curve25519 //! and Edwards25519. -use {bssl, c, error, limb}; +use {bssl, error, limb}; use std::marker::PhantomData; // Elem` is `fe` in curve25519/internal.h. @@ -87,11 +87,9 @@ impl ExtPoint { -> Result { let mut point = Self::new_at_infinity(); - bssl::map_result(unsafe { + Result::from(unsafe { GFp_x25519_ge_frombytes_vartime(&mut point, encoded) - })?; - - Ok(point) + }).map(|()| point) } pub fn into_encoded_point(self) -> EncodedPoint { @@ -157,5 +155,5 @@ extern { fn GFp_x25519_fe_neg(f: &mut Elem); fn GFp_x25519_fe_tobytes(bytes: &mut EncodedPoint, elem: &Elem); fn GFp_x25519_ge_frombytes_vartime(h: &mut ExtPoint, s: &EncodedPoint) - -> c::int; + -> bssl::Result; } diff --git a/src/poly1305.rs b/src/poly1305.rs index b8af271fa0..ed60a5f79c 100644 --- a/src/poly1305.rs +++ b/src/poly1305.rs @@ -15,7 +15,7 @@ // TODO: enforce maximum input length. -use {c, chacha, constant_time, error, polyfill}; +use {bssl, c, chacha, constant_time, error, polyfill}; use core; impl SigningContext { @@ -195,10 +195,10 @@ struct Funcs { } #[inline] -fn init(state: &mut Opaque, key: &KeyBytes, func: &mut Funcs) -> i32 { - unsafe { +fn init(state: &mut Opaque, key: &KeyBytes, func: &mut Funcs) -> Result<(), error::Unspecified> { + Result::from(unsafe { GFp_poly1305_init_asm(state, key, func) - } + }) } #[repr(u32)] @@ -235,7 +235,7 @@ pub struct SigningContext { extern { fn GFp_poly1305_init_asm(state: &mut Opaque, key: &KeyBytes, - out_func: &mut Funcs) -> c::int; + out_func: &mut Funcs) -> bssl::Result; fn GFp_poly1305_blocks(state: &mut Opaque, input: *const u8, len: c::size_t, should_pad: Pad); fn GFp_poly1305_emit(state: &mut Opaque, mac: &mut Tag, nonce: &Nonce); diff --git a/src/rand.rs b/src/rand.rs index 5a8e1484e6..a21efb4ed3 100644 --- a/src/rand.rs +++ b/src/rand.rs @@ -150,6 +150,7 @@ mod sysrand_chunk { #[link(name = "advapi32")] extern "system" { #[link_name = "SystemFunction036"] + #[must_use] fn RtlGenRandom(random_buffer: *mut u8, random_buffer_length: c::win32::ULONG) -> c::win32::BOOLEAN; @@ -274,6 +275,7 @@ mod darwin { static kSecRandomDefault: &'static SecRandomRef; // For now `rnd` must be `kSecRandomDefault`. + #[must_use] fn SecRandomCopyBytes(rnd: &'static SecRandomRef, count: c::size_t, bytes: *mut u8) -> c::int; } diff --git a/src/rsa/bigint.rs b/src/rsa/bigint.rs index 61db147685..823e2d0627 100644 --- a/src/rsa/bigint.rs +++ b/src/rsa/bigint.rs @@ -245,7 +245,7 @@ impl Modulus { if n.len() > MODULUS_MAX_LIMBS { return Err(error::Unspecified); } - bssl::map_result(unsafe { + Result::from(unsafe { GFp_bn_mul_mont_check_num_limbs(n.len()) })?; if limb::limbs_are_even_constant_time(&n) != limb::LimbMask::False { @@ -459,7 +459,7 @@ pub fn elem_reduced>( tmp.copy_from_slice(&a.limbs); let mut r = m.zero(); - bssl::map_result(unsafe { + Result::from(unsafe { GFp_bn_from_montgomery_in_place(r.limbs.as_mut_ptr(), r.limbs.len(), tmp.as_mut_ptr(), tmp.len(), m.limbs.as_ptr(), m.limbs.len(), &m.n0) @@ -743,7 +743,7 @@ pub fn elem_exp_consttime( limbs: base.limbs, encoding: PhantomData, }; - bssl::map_result(unsafe { + Result::from(unsafe { GFp_BN_mod_exp_mont_consttime(r.limbs.as_mut_ptr(), r.limbs.as_ptr(), exponent.limbs.as_ptr(), oneR.0.limbs.as_ptr(), m.limbs.as_ptr(), @@ -915,7 +915,7 @@ extern { fn GFp_bn_mul_mont(r: *mut limb::Limb, a: *const limb::Limb, b: *const limb::Limb, n: *const limb::Limb, n0: &N0, num_limbs: c::size_t); - fn GFp_bn_mul_mont_check_num_limbs(num_limbs: c::size_t) -> c::int; + fn GFp_bn_mul_mont_check_num_limbs(num_limbs: c::size_t) -> bssl::Result; fn GFp_bn_neg_inv_mod_r_u64(n: u64) -> u64; @@ -928,7 +928,7 @@ extern { fn GFp_bn_from_montgomery_in_place(r: *mut limb::Limb, num_r: c::size_t, a: *mut limb::Limb, num_a: c::size_t, n: *const limb::Limb, num_n: c::size_t, - n0: &N0) -> c::int; + n0: &N0) -> bssl::Result; // `r` and `a` may alias. fn GFp_BN_mod_exp_mont_consttime(r: *mut limb::Limb, @@ -936,7 +936,7 @@ extern { p: *const limb::Limb, one_mont: *const limb::Limb, n: *const limb::Limb, - num_limbs: c::size_t, n0: &N0) -> c::int; + num_limbs: c::size_t, n0: &N0) -> bssl::Result; // `r` and `a` may alias. fn LIMBS_add_mod(r: *mut limb::Limb, a: *const limb::Limb,