From d9e378d2b5852e9716179c4265c634fc0cf81e1c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 17 Oct 2023 12:07:26 -0600 Subject: [PATCH] rename to_arc to clone_arc to_arc converts a `*const C` into an `Option>`. It had a lot of verbiage to explain what it was doing with reference counts. I think the conceptual model becomes a lot simpler if we rename the function to reflect what it is really doing: cloning the arc. --- src/acceptor.rs | 6 +++--- src/client.rs | 8 ++++---- src/lib.rs | 39 ++++++++++++--------------------------- src/server.rs | 12 ++++++------ 4 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/acceptor.rs b/src/acceptor.rs index a559d841..3672a956 100644 --- a/src/acceptor.rs +++ b/src/acceptor.rs @@ -12,8 +12,8 @@ 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_arc_from_ptr, try_callback, try_mut_from_ptr, try_ref_from_ptr, try_take, Castable, - OwnershipBox, + try_arc_from_ptr, try_callback, try_clone_arc, try_mut_from_ptr, try_ref_from_ptr, try_take, + Castable, OwnershipBox, }; use rustls_result::NullParameter; @@ -400,7 +400,7 @@ impl rustls_accepted { ffi_panic_boundary! { let accepted: &mut Option = try_mut_from_ptr!(accepted); let accepted = try_take!(accepted); - let config: Arc = try_arc_from_ptr!(config); + let config: Arc = try_clone_arc!(config); match accepted.into_connection(config) { Ok(built) => { let wrapped = crate::connection::Connection::from_server(built); diff --git a/src/client.rs b/src/client.rs index a76247f5..6fef5af5 100644 --- a/src/client.rs +++ b/src/client.rs @@ -22,8 +22,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_arc_from_ptr, try_box_from_ptr, 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_ref_from_ptr, try_slice, userdata_get, + Castable, OwnershipArc, OwnershipBox, }; /// A client config being constructed. A builder can be modified by, @@ -442,7 +442,7 @@ impl rustls_client_config_builder { let keys_ptrs: &[*const rustls_certified_key] = try_slice!(certified_keys, certified_keys_len); let mut keys: Vec> = Vec::new(); for &key_ptr in keys_ptrs { - let certified_key: Arc = try_arc_from_ptr!(key_ptr); + let certified_key: Arc = try_clone_arc!(key_ptr); keys.push(certified_key); } config.cert_resolver = Some(Arc::new(ResolvesClientCertFromChoices { keys })); @@ -545,7 +545,7 @@ impl rustls_client_config { } CStr::from_ptr(server_name) }; - let config: Arc = try_arc_from_ptr!(config); + let config: Arc = try_clone_arc!(config); let server_name: &str = match server_name.to_str() { Ok(s) => s, Err(std::str::Utf8Error { .. }) => return rustls_result::InvalidDnsNameError, diff --git a/src/lib.rs b/src/lib.rs index 04a7e1df..ad00c3f5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -368,19 +368,16 @@ where Arc::into_raw(Arc::new(src)) as *const _ } -/// Convert a const pointer to a [`Castable`] to an optional `Arc` over the underlying rust type. +/// Given a const pointer to a [`Castable`] representing an `Arc`, clone the `Arc` and return +/// the corresponding Rust type. /// -/// Sometimes we create an `Arc`, then call `into_raw` and return the resulting raw pointer -/// to C, e.g. using [`to_arc_const_ptr`]. C can then call a rustls-ffi function multiple times -/// using that same raw pointer. On each call, we need to reconstruct the Arc, but once we reconstruct -/// the Arc, its reference count will be decremented on drop. We need to reference count to stay at 1, -/// because the C code is holding a copy. +/// The caller still owns its copy of the `Arc`. In other words, the reference count of the +/// `Arc` will be incremented by 1 by the end of this function. /// -/// This function turns the raw pointer back into an Arc, clones it to increment the reference count -/// (which will make it 2 in this particular case), and `mem::forget`s the clone. The `mem::forget` -/// prevents the reference count from being decremented when we exit this function, so it will stay -/// at 2 as long as we are in Rust code. Once the caller drops its `Arc`, the reference count will -/// go back down to 1, since the C code's copy remains. +/// To achieve that, we need to `mem::forget` the `Arc` we get back from `into_raw`, because +/// `into_raw` _does_ take back ownership. If we called `into_raw` without `mem::forget`, at the +/// end of the function the reference count would be decremented, potentially to 0, causing +/// memory to be freed. /// /// Does nothing, returning `None`, when passed a `NULL` pointer. Can only be used when the /// `Castable` has specified a cast type equal to [`OwnershipArc`]. @@ -389,7 +386,7 @@ where /// /// If non-null, `ptr` must be a pointer that resulted from previously calling `Arc::into_raw`, /// e.g. from using [`to_arc_const_ptr`]. -pub(crate) fn to_arc(ptr: *const C) -> Option> +pub(crate) fn clone_arc(ptr: *const C) -> Option> where C: Castable, { @@ -566,32 +563,20 @@ macro_rules! try_ref_from_ptr { pub(crate) use try_ref_from_ptr; -/// Convert a const pointer to a [`Castable`] to an optional `Arc` over the underlying -/// [`Castable::RustType`]. See [`to_arc`] for more information. -/// -/// Does nothing, returning `None`, when passed `NULL`. Can only be used with `Castable`'s that -/// specify an ownership type of [`OwnershipArc`]. -pub(crate) fn try_arc_from(from: *const C) -> Option> -where - C: Castable, -{ - to_arc(from) -} - /// 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 /// context. See [`try_arc_from`] for more information. -macro_rules! try_arc_from_ptr { +macro_rules! try_clone_arc { ( $var:ident ) => { - match $crate::try_arc_from($var) { + match $crate::clone_arc($var) { Some(c) => c, None => return $crate::panic::NullParameterOrDefault::value(), } }; } -pub(crate) use try_arc_from_ptr; +pub(crate) use try_clone_arc; /// Convert a mutable pointer to a [`Castable`] to an optional `Box` over the underlying /// [`Castable::RustType`]. diff --git a/src/server.rs b/src/server.rs index 4b9276ad..2234af10 100644 --- a/src/server.rs +++ b/src/server.rs @@ -30,8 +30,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_arc_from_ptr, try_box_from_ptr, 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_ref_from_ptr, try_slice, userdata_get, + Castable, OwnershipArc, OwnershipBox, OwnershipRef, }; /// A server config being constructed. A builder can be modified by, @@ -170,7 +170,7 @@ impl rustls_server_config_builder { ) { ffi_panic_boundary! { let builder: &mut ServerConfigBuilder = try_mut_from_ptr!(builder); - let verifier: Arc = try_arc_from_ptr!(verifier); + let verifier: Arc = try_clone_arc!(verifier); builder.verifier = verifier; } } @@ -186,7 +186,7 @@ impl rustls_server_config_builder { ) { ffi_panic_boundary! { let builder: &mut ServerConfigBuilder = try_mut_from_ptr!(builder); - let verifier: Arc = try_arc_from_ptr!(verifier); + let verifier: Arc = try_clone_arc!(verifier); builder.verifier = verifier; } @@ -273,7 +273,7 @@ impl rustls_server_config_builder { let keys_ptrs: &[*const rustls_certified_key] = try_slice!(certified_keys, certified_keys_len); let mut keys: Vec> = Vec::new(); for &key_ptr in keys_ptrs { - let certified_key: Arc = try_arc_from_ptr!(key_ptr); + let certified_key: Arc = try_clone_arc!(key_ptr); keys.push(certified_key); } builder.cert_resolver = Some(Arc::new(ResolvesServerCertFromChoices::new(&keys))); @@ -333,7 +333,7 @@ impl rustls_server_config { conn_out: *mut *mut rustls_connection, ) -> rustls_result { ffi_panic_boundary! { - let config: Arc = try_arc_from_ptr!(config); + let config: Arc = try_clone_arc!(config); let server_connection = match ServerConnection::new(config) { Ok(sc) => sc,