Much more safety documentation

bindings/rust/s2n-tls/src/callbacks/pkey.rs

@@ -24,13 +24,30 @@ pub struct PrivateKeyOperation {
 
 /// # Safety
 ///
-/// Safety: s2n_async_pkey_op objects can be sent across threads
+/// NonNull / the raw s2n_async_pkey_op pointer isn't Send because its data may
+/// be aliased (two pointers could point to the same raw memory). However, the
+/// PrivateKeyOperation interface ensures that only one owned PrivateKeyOperation
+/// can exist for each s2n_async_pkey_op C object.
+///
+/// In particular, the only method of obtaining a PrivateKeyOperation is via
+/// PrivateKeyCallback::handle_operation, which returns unique memory owned by
+/// the application.
+///
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// do not expose the raw s2n_async_pkey_op pointer, return owned PrivateKeyOperation
+/// objects, or allow the creation of PrivateKeyOperations from raw pointers.
+///
 unsafe impl Send for PrivateKeyOperation {}
 
 /// # Safety
 ///
-/// Safety: All C methods that mutate the s2n_async_pkey_op are wrapped
-/// in Rust methods that require a mutable reference.
+/// NonNull / the raw s2n_async_pkey_op pointer isn't Sync because it allows access
+/// to mutable pointers even from immutable references. However, the PrivateKeyOperation
+/// interface enforces that all mutating methods correctly require self or &mut self.
+///
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// correctly use either self, &self or &mut self depending on their behavior.
+///
 unsafe impl Sync for PrivateKeyOperation {}
 
 impl PrivateKeyOperation {

bindings/rust/s2n-tls/src/cert_chain.rs

@@ -28,6 +28,10 @@ impl CertificateChain<'_> {
         }
     }
 
+    /// # Safety
+    ///
+    /// Caller must ensure ptr is a valid reference to a [`s2n_cert_chain_and_key`] object
+    /// Caller must ensure they are not creating a duplicate CertificateChain (see Send safety note).
     pub(crate) unsafe fn from_ptr_reference<'a>(
         ptr: NonNull<s2n_cert_chain_and_key>,
     ) -> CertificateChain<'a> {
@@ -72,16 +76,40 @@ impl CertificateChain<'_> {
         self.len() == 0
     }
 
-    pub(crate) fn as_mut_ptr(&mut self) -> NonNull<s2n_cert_chain_and_key> {
+    /// # Safety
+    ///
+    /// Caller must ensure they are not creating the possibility of duplicate
+    /// CertificateChain (see Send safety note).
+    /// This should ONLY be used to pass the CertificateChain to C methods.
+    pub(crate) unsafe fn as_mut_ptr(&mut self) -> NonNull<s2n_cert_chain_and_key> {
         self.ptr
     }
 }
 
-// # Safety
-//
-// s2n_cert_chain_and_key objects can be sent across threads.
+/// # Safety
+///
+/// NonNull / the raw s2n_cert_chain_and_key pointer isn't Send because its data
+/// may be aliased (two pointers could point to the same raw memory). However,
+/// the CertificateChain interface ensures that only one owned CertificateChain
+/// can exist for each s2n_cert_chain_and_key C object.
+/// Additionally, the CertificateChain is immutable once created.
+///
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// do not expose the raw s2n_cert_chain_and_key pointer, return owned CertificateChain objects,
+/// or allow the creation of CertificateChains from raw pointers. Failing that,
+/// no method should take a &mut CertificateChain argument.
 unsafe impl Send for CertificateChain<'_> {}
 
+/// # Safety
+///
+/// NonNull / the raw s2n_cert_chain_and_key pointer isn't Sync because it allows
+/// access to mutable pointers even from immutable references. However, the CertificateChain
+/// interface enforces that all mutating methods correctly require &mut self.
+///
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// correctly use either &self or &mut self depending on their behavior.
+unsafe impl Sync for CertificateChain<'_> {}
+
 impl Drop for CertificateChain<'_> {
     fn drop(&mut self) {
         if self.is_owned {
@@ -148,7 +176,24 @@ impl<'a> Certificate<'a> {
     }
 }
 
-// # Safety
-//
-// Certificates just reference data in the chain, so share the Send-ness of the chain.
+/// # Safety
+///
+/// NonNull / the raw s2n_cert pointer isn't Send because its data
+/// may be aliased (two pointers could point to the same raw memory). Multiple
+/// Certificates can reference the same memory, since multiple iterators over
+/// CertificateChain can exist at once. However, the Certificate is still Send
+/// because it is immutable.
+///
+/// No mechanism enforces this. Library developers MUST ensure that the Certificate
+/// is NEVER mutated. No method should take a &mut Certificate argument.
 unsafe impl Send for Certificate<'_> {}
+
+/// # Safety
+///
+/// NonNull / the raw s2n_cert pointer isn't Sync because it allows access
+/// to mutable pointers even from immutable references. However, the Certificate is
+/// still Sync because it is immutable.
+///
+/// No mechanism enforces this. Library developers MUST ensure that the Certificate
+/// is NEVER mutated. No method should take a &mut Certificate argument.
+unsafe impl Sync for Certificate<'_> {}

bindings/rust/s2n-tls/src/config.rs

@@ -23,13 +23,39 @@ pub struct Config(NonNull<s2n_config>);
 
 /// # Safety
 ///
-/// Safety: s2n_config objects can be sent across threads
+/// NonNull / the raw s2n_config pointer isn't Send because its data may be aliased
+/// (two pointers could point to the same raw memory). Because Config implements
+/// Clone, multiple Configs are expected to point to the same raw memory.
+/// However, the Config is still Send because it is immutable after creation.
+///
+/// For example: an application can create and then clone a Config, creating two
+/// Configs representing the same C s2n_config struct. However, neither Config can
+/// modify that shared memory, so multiple threads accessing it remains safe.
+///
+/// No mechanism enforces this. Library developers MUST ensure that the Config
+/// is NEVER mutated. No method should take a &mut Config argument.
+///
+/// The Context stored on a Config must also be Send. The Context is essentially
+/// a field on the Config, just stored in C instead of in Rust.
+/// A test exists to enforce this. In particular, note that the Context includes
+/// a reference counter, but that reference counter is safely atomic and can be
+/// safely incremented by different Configs on different threads.
+///
 unsafe impl Send for Config {}
 
 /// # Safety
 ///
-/// Safety: All C methods that mutate the s2n_config are wrapped
-/// in Rust methods that require a mutable reference.
+/// NonNull / the raw s2n_config pointer isn't Sync because it allows access
+/// to mutable pointers even from immutable references. However, the Config is
+/// still Sync because it is immutable after creation.
+///
+/// No mechanism enforces this. Library developers MUST ensure that the Config
+/// is NEVER mutated. No method should take a &mut Config argument.
+///
+/// The Context stored on a Config must also be Sync. The Context is essentially
+/// a field on the Config, just stored in C instead of in Rust.
+/// A test exists to enforce this.
+///
 unsafe impl Sync for Config {}
 
 impl Config {
@@ -61,7 +87,13 @@ impl Config {
         config
     }
 
-    pub(crate) fn as_mut_ptr(&mut self) -> *mut s2n_config {
+    /// # Safety
+    ///
+    /// This method must ONLY be used:
+    /// - In the Builder
+    /// - To call non-mutating C methods
+    /// The Config must NOT be modified after being built!
+    pub(crate) unsafe fn as_mut_ptr(&mut self) -> *mut s2n_config {
         self.0.as_ptr()
     }
 
@@ -730,7 +762,10 @@ impl Builder {
     }
 
     fn as_mut_ptr(&mut self) -> *mut s2n_config {
-        self.config.as_mut_ptr()
+        // Safety:
+        // The mutable pointer is only used inside the builder, which does not allow
+        // modification of complete Configs or risk thread safety.
+        unsafe { self.config.as_mut_ptr() }
     }
 }
 

bindings/rust/s2n-tls/src/connection.rs

@@ -61,22 +61,54 @@ impl fmt::Debug for Connection {
 
 /// # Safety
 ///
-/// s2n_connection objects can be sent across threads
+/// NonNull / the raw s2n_connection pointer isn't Send because its data may be
+/// aliased (two pointers could point to the same raw memory).
+///
+/// For example: Two NonNull<s2n_connection> objects can both reference the same memory.
+/// If you sent one of the NonNulls to another thread, then both threads would
+/// own references to the same memory and therefore mutate it at the same time,
+/// violating thread safety.
+///
+/// However, the Connection is Send because the interface ensures that only one
+/// owned Connection can exist for each s2n_connection C object.
+///
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// do not expose the raw s2n_connection pointer, return owned Connection objects,
+/// or allow the creation of Connections from raw pointers. Because C methods like
+/// callbacks can expose the raw pointers, pay particular attention to the thread
+/// safety of callbacks.
+///
+/// The Context stored on a Connection must also be Send. The Context is essentially
+/// a field on the Connection, just stored in C instead of in Rust.
+/// A test exists to enforce this.
+///
 unsafe impl Send for Connection {}
 
-/// # Sync
+/// # Safety
 ///
-/// Although NonNull isn't Sync and allows access to mutable pointers even from
-/// immutable references, the Connection interface enforces that all mutating
+/// NonNull / the raw s2n_connection pointer isn't Sync because it allows access
+/// to mutable pointers even from immutable references.
+///
+/// For example: Multiple immutable references to the same NonNull<s2n_connection>
+/// are allowed to exist at once. If one &NonNull is sent to another thread, and
+/// as_ptr() is called on both &NonNull (allowed because NonNull<T> implements From for &T),
+/// then both threads can obtain *mut s2n_connection pointers to the same memory
+/// and therefore mutate it at the same time, violating thread safety.
+///
+/// However, the Connection is Sync because the interface enforces that all mutating
 /// methods correctly require &mut self.
 ///
-/// Developers and reviewers MUST ensure that new methods correctly use
-/// either &self or &mut self depending on their behavior. No mechanism enforces this.
+/// No mechanism enforces this. Library developers MUST ensure that new methods
+/// correctly use either &self or &mut self depending on their behavior.
 ///
 /// Note: Although non-mutating methods like getters should be thread-safe by definition,
 /// technically the only thread safety guarantee provided by the underlying C library
 /// is that s2n_send and s2n_recv can be called concurrently.
 ///
+/// The Context stored on a Connection must also be Sync. The Context is essentially
+/// a field on the Connection, just stored in C instead of in Rust.
+/// A test exists to enforce this.
+///
 unsafe impl Sync for Connection {}
 
 impl Connection {
@@ -122,13 +154,19 @@ impl Connection {
         Self::new(Mode::Server)
     }
 
-    pub(crate) fn as_ptr(&mut self) -> *mut s2n_connection {
+    /// # Safety
+    ///
+    /// Caller must ensure they are not creating the possibility of duplicate
+    /// Connections (see Send safety note).
+    /// This should ONLY be used to pass the Connection to C methods.
+    pub(crate) unsafe fn as_ptr(&mut self) -> *mut s2n_connection {
         self.connection.as_ptr()
     }
 
     /// # Safety
     ///
     /// Caller must ensure s2n_connection is a valid reference to a [`s2n_connection`] object
+    /// Caller must ensure they are not creating a duplicate Connection (see Send safety note).
     pub(crate) unsafe fn from_raw(connection: NonNull<s2n_connection>) -> Self {
         Self { connection }
     }