Use #[repr(transparent)] newtype to hold FFI error codes

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()
     })
 }

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

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);

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<Result> 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::<bssl::Result>(), mem::size_of::<Underlying>());
+            assert_eq!(mem::align_of::<bssl::Result>(), mem::align_of::<Underlying>());
+        }
+
+        #[test]
+        fn semantics() {
+            assert!(Result::from(bssl::Result(0)).is_err());
+            assert!(Result::from(bssl::Result(1)).is_ok());
+        }
+    }
+}

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<T>` is `fe` in curve25519/internal.h.
@@ -87,11 +87,9 @@ impl ExtPoint {
                           -> Result<Self, error::Unspecified> {
         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<T>);
     fn GFp_x25519_fe_tobytes(bytes: &mut EncodedPoint, elem: &Elem<T>);
     fn GFp_x25519_ge_frombytes_vartime(h: &mut ExtPoint, s: &EncodedPoint)
-                                       -> c::int;
+                                       -> bssl::Result;
 }

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);

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;
     }

src/rsa/bigint.rs

@@ -245,7 +245,7 @@ impl<M> Modulus<M> {
         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<Larger, Smaller: NotMuchSmallerModulus<Larger>>(
     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<M>(
         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,15 +928,15 @@ 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,
                                      a_mont: *const limb::Limb,
                                      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,