null-safe set_boxed_mut_ptr/set_arc_mut_ptr

Previously the `set_boxed_mut_ptr()` and `set_arc_mut_ptr()` helper fns
used for assigning out parameters across the FFI boundary took `*mut
*mut C` and `*mut *const C` for the destination argument `dst`. Using
these safely required callers always verify that `dst != NULL`. In
practice it's very easy to forget to do this and danger lurks!

We could modify these helpers to do the check itself, but we tend to use
these fns near the end of a function to assign a result in a success
case and we would prefer `NULL` checking happen at the beginning of the
function.

One proposed solution is to modify these setter functions to take `&mut
*mut C` and `&mut *const C`. By using new helper fns to carefully
construct a `&mut` from the input double pointer we can front-load
the `NULL` check and the assignment in the set fns can proceed knowing
there's no possibility for a `NULL` outer pointer.

This commit implements this strategy, updating the argument type of
`set_boxed_mut_ptr` and `set_arc_mut_ptr` to take `&mut (*const|*mut)
C`. New `try_mut_from_ptr_ptr` and `try_ref_from_ptr_ptr` macros allow
converting from `*mut *mut C` and `*mut *const C` to the reference
types, bailing early for `NULL`.

Author: cpu

src/acceptor.rs

@@ -13,10 +13,9 @@ use crate::rslice::{rustls_slice_bytes, rustls_str};
 use crate::server::rustls_server_config;
 use crate::{
     ffi_panic_boundary, free_box, rustls_result, set_boxed_mut_ptr, to_box, to_boxed_mut_ptr,
-    try_callback, try_clone_arc, try_mut_from_ptr, try_ref_from_ptr, try_take, Castable,
-    OwnershipBox,
+    try_callback, try_clone_arc, try_mut_from_ptr, try_mut_from_ptr_ptr, try_ref_from_ptr,
+    try_take, Castable, OwnershipBox,
 };
-use rustls_result::NullParameter;
 
 /// A buffer and parser for ClientHello bytes. This allows reading ClientHello
 /// before choosing a rustls_server_config. It's useful when the server
@@ -187,12 +186,8 @@ impl rustls_acceptor {
     ) -> rustls_result {
         ffi_panic_boundary! {
             let acceptor: &mut Acceptor = try_mut_from_ptr!(acceptor);
-            if out_accepted.is_null() {
-                return NullParameter;
-            }
-            if out_alert.is_null() {
-                return NullParameter;
-            }
+            let out_accepted = try_mut_from_ptr_ptr!(out_accepted);
+            let out_alert = try_mut_from_ptr_ptr!(out_alert);
             match acceptor.accept() {
                 Ok(None) => rustls_result::AcceptorNotReady,
                 Ok(Some(accepted)) => {
@@ -426,12 +421,8 @@ impl rustls_accepted {
             let accepted: &mut Option<Accepted> = try_mut_from_ptr!(accepted);
             let accepted = try_take!(accepted);
             let config: Arc<ServerConfig> = try_clone_arc!(config);
-            if out_conn.is_null() {
-                return NullParameter;
-            }
-            if out_alert.is_null() {
-                return NullParameter;
-            }
+            let out_conn = try_mut_from_ptr_ptr!(out_conn);
+            let out_alert = try_mut_from_ptr_ptr!(out_alert);
             match accepted.into_connection(config) {
                 Ok(built) => {
                     let wrapped = crate::connection::Connection::from_server(built);

src/cipher.rs

@@ -22,8 +22,8 @@ use crate::error::{self, rustls_result};
 use crate::rslice::{rustls_slice_bytes, rustls_str};
 use crate::{
     ffi_panic_boundary, free_arc, free_box, set_arc_mut_ptr, set_boxed_mut_ptr, to_arc_const_ptr,
-    to_boxed_mut_ptr, try_clone_arc, try_mut_from_ptr, try_ref_from_ptr, try_slice, try_take,
-    Castable, OwnershipArc, OwnershipBox, OwnershipRef,
+    to_boxed_mut_ptr, try_clone_arc, try_mut_from_ptr, try_mut_from_ptr_ptr, try_ref_from_ptr,
+    try_ref_from_ptr_ptr, try_slice, try_take, Castable, OwnershipArc, OwnershipBox, OwnershipRef,
 };
 use rustls_result::{AlreadyUsed, NullParameter};
 
@@ -592,7 +592,7 @@ impl rustls_root_cert_store_builder {
         ffi_panic_boundary! {
             let builder: &mut Option<RootCertStoreBuilder> = try_mut_from_ptr!(builder);
             let builder = try_take!(builder);
-
+            let root_cert_store_out = try_ref_from_ptr_ptr!(root_cert_store_out);
             set_arc_mut_ptr(root_cert_store_out, builder.roots);
 
             rustls_result::Ok
@@ -893,6 +893,7 @@ impl rustls_web_pki_client_cert_verifier_builder {
             let client_verifier_builder: &mut Option<ClientCertVerifierBuilder> =
                 try_mut_from_ptr!(builder);
             let client_verifier_builder = try_take!(client_verifier_builder);
+            let verifier_out = try_mut_from_ptr_ptr!(verifier_out);
 
             let mut builder = WebPkiClientVerifier::builder_with_provider(
                     client_verifier_builder.roots,
@@ -925,7 +926,6 @@ impl rustls_web_pki_client_cert_verifier_builder {
             };
 
             set_boxed_mut_ptr(verifier_out, verifier);
-
             rustls_result::Ok
         }
     }
@@ -1103,6 +1103,7 @@ impl ServerCertVerifierBuilder {
             let server_verifier_builder: &mut Option<ServerCertVerifierBuilder> =
                 try_mut_from_ptr!(builder);
             let server_verifier_builder = try_take!(server_verifier_builder);
+            let verifier_out = try_mut_from_ptr_ptr!(verifier_out);
 
             let mut builder = WebPkiServerVerifier::builder_with_provider(
                     server_verifier_builder.roots,

src/client.rs

@@ -23,8 +23,8 @@ use crate::rslice::NulByte;
 use crate::rslice::{rustls_slice_bytes, rustls_slice_slice_bytes, rustls_str};
 use crate::{
     ffi_panic_boundary, free_arc, free_box, set_boxed_mut_ptr, to_arc_const_ptr, to_boxed_mut_ptr,
-    try_box_from_ptr, try_clone_arc, try_mut_from_ptr, try_ref_from_ptr, try_slice, userdata_get,
-    Castable, OwnershipArc, OwnershipBox,
+    try_box_from_ptr, try_clone_arc, try_mut_from_ptr, try_mut_from_ptr_ptr, try_ref_from_ptr,
+    try_slice, userdata_get, Castable, OwnershipArc, OwnershipBox,
 };
 
 /// A client config being constructed. A builder can be modified by,
@@ -184,6 +184,8 @@ impl rustls_client_config_builder {
                 }
             }
 
+            let builder_out = try_mut_from_ptr_ptr!(builder_out);
+
             let provider = rustls::crypto::CryptoProvider {
                 cipher_suites: cs_vec,
                 ..rustls::crypto::ring::default_provider()
@@ -575,6 +577,7 @@ impl rustls_client_config {
                 CStr::from_ptr(server_name)
             };
             let config: Arc<ClientConfig> = try_clone_arc!(config);
+            let conn_out = try_mut_from_ptr_ptr!(conn_out);
             let server_name: &str = match server_name.to_str() {
                 Ok(s) => s,
                 Err(std::str::Utf8Error { .. }) => return rustls_result::InvalidDnsNameError,

src/lib.rs

@@ -485,17 +485,11 @@ where
 /// Converts a [`Castable`]'s underlying [`Castable::RustType`] to a mutable pointer
 /// to a `Box` over the rust type and sets the `dst` out pointer to the resulting mutable `Box`
 /// pointer. See [`to_boxed_mut_ptr`] for more information.
-///
-/// ## Unsafety:
-///
-/// `dst` must not be `NULL`.
-pub(crate) fn set_boxed_mut_ptr<C>(dst: *mut *mut C, src: C::RustType)
+pub(crate) fn set_boxed_mut_ptr<C>(dst: &mut *mut C, src: C::RustType)
 where
     C: Castable<Ownership = OwnershipBox>,
 {
-    unsafe {
-        *dst = to_boxed_mut_ptr(src);
-    }
+    *dst = to_boxed_mut_ptr(src);
 }
 
 /// Converts a [`Castable`]'s underlying [`Castable::RustType`] to a const pointer
@@ -505,13 +499,11 @@ where
 /// ## Unsafety:
 ///
 /// `dst` must not be `NULL`.
-pub(crate) fn set_arc_mut_ptr<C>(dst: *mut *const C, src: C::RustType)
+pub(crate) fn set_arc_mut_ptr<C>(dst: &mut *const C, src: C::RustType)
 where
     C: Castable<Ownership = OwnershipArc>,
 {
-    unsafe {
-        *dst = to_arc_const_ptr(src);
-    }
+    *dst = to_arc_const_ptr(src);
 }
 
 /// Converts a mutable pointer to a [`Castable`] to an optional ref to the underlying
@@ -540,6 +532,32 @@ macro_rules! try_mut_from_ptr {
 
 pub(crate) use try_mut_from_ptr;
 
+/// Converts a mutable pointer to a mutable pointer to a [`Castable`] to an optional mutable ref to
+/// the mutable pointer to the  [`Castable::RustType`].
+///
+/// Does nothing, returning `None`, when passed `NULL`.
+pub(crate) fn try_from_mut_mut<'a, C: Castable>(from: *mut *mut C) -> Option<&'a mut *mut C> {
+    match from.is_null() {
+        true => None,
+        false => unsafe { Some(&mut *from) },
+    }
+}
+
+/// If the provided pointer to a pointer to a [`Castable`] is non-null, convert it to a mutable
+/// reference to a pointer using [`try_from_mut_mut`]. Otherwise, return
+/// [`rustls_result::NullParameter`], or an appropriate default (`false`, `0`, `NULL`) based on the
+/// context. See [`try_from_mut_mut`] for more information.
+macro_rules! try_mut_from_ptr_ptr {
+    ( $var:ident ) => {
+        match $crate::try_from_mut_mut($var) {
+            Some(c) => c,
+            None => return $crate::panic::NullParameterOrDefault::value(),
+        }
+    };
+}
+
+pub(crate) use try_mut_from_ptr_ptr;
+
 /// Converts a const pointer to a [`Castable`] to an optional ref to the underlying
 /// [`Castable::RustType`]. See [`cast_const_ptr`] for more information.
 ///
@@ -569,6 +587,37 @@ macro_rules! try_ref_from_ptr {
 
 pub(crate) use try_ref_from_ptr;
 
+/// Converts a mut pointer to a const pointer to a [`Castable`] to an optional mut ref to the
+/// const pointer to the underlying [`Castable::RustType`].
+///
+/// Does nothing, returning `None` when passed `NULL`.
+pub(crate) fn try_from_ptr<'a, C>(from: *mut *const C) -> Option<&'a mut *const C>
+where
+    C: Castable,
+{
+    match from.is_null() {
+        true => None,
+        false => unsafe { Some(&mut *from) },
+    }
+}
+
+/// If the provided pointer to pointer to a [`Castable`] is non-null, convert it to a mutable
+/// reference to a pointer to the [`Castable`] using
+/// [`try_from_ptr`]. Otherwise, return [`rustls_result::NullParameter`], or an appropriate default
+/// (`false`, `0`, `NULL`) based on the context;
+///
+/// See [`try_from_ptr`] for more information.
+macro_rules! try_ref_from_ptr_ptr {
+    ( $var:ident ) => {
+        match $crate::try_from_ptr($var) {
+            Some(c) => c,
+            None => return $crate::panic::NullParameterOrDefault::value(),
+        }
+    };
+}
+
+pub(crate) use try_ref_from_ptr_ptr;
+
 /// If the provided pointer to a [`Castable`] is non-null, convert it to a reference to an `Arc` over
 /// the underlying rust type using [`try_arc_from`]. Otherwise, return
 /// [`rustls_result::NullParameter`], or an appropriate default (`false`, `0`, `NULL`) based on the

src/server.rs

@@ -27,8 +27,8 @@ use crate::session::{
 };
 use crate::{
     ffi_panic_boundary, free_arc, free_box, set_boxed_mut_ptr, to_arc_const_ptr, to_boxed_mut_ptr,
-    try_box_from_ptr, try_clone_arc, try_mut_from_ptr, try_ref_from_ptr, try_slice, userdata_get,
-    Castable, OwnershipArc, OwnershipBox, OwnershipRef,
+    try_box_from_ptr, try_clone_arc, try_mut_from_ptr, try_mut_from_ptr_ptr, try_ref_from_ptr,
+    try_slice, userdata_get, Castable, OwnershipArc, OwnershipBox, OwnershipRef,
 };
 
 /// A server config being constructed. A builder can be modified by,
@@ -141,6 +141,8 @@ impl rustls_server_config_builder {
                 }
             }
 
+            let builder_out = try_mut_from_ptr_ptr!(builder_out);
+
             let provider = rustls::crypto::CryptoProvider {
                 cipher_suites: cs_vec,
                 ..rustls::crypto::ring::default_provider()
@@ -325,6 +327,7 @@ impl rustls_server_config {
                 return NullParameter;
             }
             let config: Arc<ServerConfig> = try_clone_arc!(config);
+            let conn_out = try_mut_from_ptr_ptr!(conn_out);
 
             let server_connection = match ServerConnection::new(config) {
                 Ok(sc) => sc,