From 83ccd1b2e6effbd4d1e67f8379cb949c432353e4 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 22 Oct 2026 16:37:06 +0100 Subject: [PATCH] Use `ptr::NonNull` for pointer checks Use ? and let-else to improve code structure --- librustls/src/acceptor.rs | 14 +++++---- librustls/src/certificate.rs | 33 +++++++++---------- librustls/src/client.rs | 9 +++--- librustls/src/connection.rs | 61 +++++++++++++++++++----------------- librustls/src/error.rs | 16 +++++----- librustls/src/ffi.rs | 45 ++++++++++---------------- librustls/src/server.rs | 13 ++++---- 7 files changed, 94 insertions(+), 97 deletions(-) diff --git a/librustls/src/acceptor.rs b/librustls/src/acceptor.rs index 0b94cc54..add6d200 100644 --- a/librustls/src/acceptor.rs +++ b/librustls/src/acceptor.rs @@ -1,3 +1,5 @@ +use std::ptr::NonNull; + use libc::{EINVAL, EIO, c_void, size_t}; use rustls::server::{Accepted, AcceptedAlert, Acceptor}; @@ -123,9 +125,9 @@ impl rustls_acceptor { ) -> rustls_io_result { ffi_panic_boundary! { let acceptor = try_mut_from_ptr!(acceptor); - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_io_result(EINVAL); - } + }; let callback = try_callback!(callback); let mut reader = CallbackReader { callback, userdata }; @@ -135,7 +137,7 @@ impl rustls_acceptor { Err(e) => return rustls_io_result(e.raw_os_error().unwrap_or(EIO)), }; unsafe { - *out_n = n_read; + out_n.write(n_read); } rustls_io_result(0) @@ -489,9 +491,9 @@ impl rustls_accepted_alert { out_n: *mut size_t, ) -> rustls_io_result { ffi_panic_boundary! { - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_io_result(EINVAL); - } + }; let accepted_alert = try_mut_from_ptr!(accepted_alert); let mut writer = CallbackWriter { callback: try_callback!(callback), @@ -502,7 +504,7 @@ impl rustls_accepted_alert { Err(e) => return rustls_io_result(e.raw_os_error().unwrap_or(EIO)), }; unsafe { - *out_n = n_written; + out_n.write(n_written); } rustls_io_result(0) } diff --git a/librustls/src/certificate.rs b/librustls/src/certificate.rs index 0e164423..de032efb 100644 --- a/librustls/src/certificate.rs +++ b/librustls/src/certificate.rs @@ -1,6 +1,6 @@ use std::ffi::CStr; use std::marker::PhantomData; -use std::ptr::null; +use std::ptr::{NonNull, null}; use std::slice; use libc::{c_char, size_t}; @@ -42,13 +42,15 @@ pub extern "C" fn rustls_certificate_get_der( ) -> rustls_result { ffi_panic_boundary! { let cert = try_ref_from_ptr!(cert); - if out_der_data.is_null() || out_der_len.is_null() { + let (Some(out_der_data), Some(out_der_len)) = + (NonNull::new(out_der_data), NonNull::new(out_der_len)) + else { return rustls_result::NullParameter; - } + }; let der = cert.as_ref(); unsafe { - *out_der_data = der.as_ptr(); - *out_der_len = der.len(); + out_der_data.write(der.as_ptr()); + out_der_len.write(der.len()); } rustls_result::Ok } @@ -235,12 +237,13 @@ impl rustls_certified_key { }; let certified_key = try_ref_from_ptr!(certified_key); let mut new_key = certified_key.clone(); - if !ocsp_response.is_null() { - let ocsp_slice = unsafe { &*ocsp_response }; - new_key.ocsp = Some(Vec::from(try_slice!(ocsp_slice.data, ocsp_slice.len))); - } else { - new_key.ocsp = None; - } + new_key.ocsp = match NonNull::new(ocsp_response.cast_mut()) { + Some(ocsp_response) => { + let ocsp_slice = unsafe { ocsp_response.as_ref() }; + Some(Vec::from(try_slice!(ocsp_slice.data, ocsp_slice.len))) + } + None => None, + }; *cloned_key_out = to_arc_const_ptr(new_key); rustls_result::Ok } @@ -383,12 +386,10 @@ impl rustls_root_cert_store_builder { Some(b) => b, }; - let filename = unsafe { - if filename.is_null() { - return rustls_result::NullParameter; - } - CStr::from_ptr(filename) + let Some(filename) = NonNull::new(filename.cast_mut()) else { + return rustls_result::NullParameter; }; + let filename = unsafe { CStr::from_ptr(filename.as_ptr()) }; let filename = filename.to_bytes(); let filename = match std::str::from_utf8(filename) { diff --git a/librustls/src/client.rs b/librustls/src/client.rs index 64c28a8b..c6951c2c 100644 --- a/librustls/src/client.rs +++ b/librustls/src/client.rs @@ -1,5 +1,6 @@ use std::ffi::CStr; use std::fmt::{Debug, Formatter}; +use std::ptr::NonNull; use std::slice; use std::sync::Arc; @@ -889,12 +890,10 @@ impl rustls_client_config { alpn_protocols: Vec>, conn_out: *mut *mut rustls_connection, ) -> rustls_result { - let server_name = unsafe { - if server_name.is_null() { - return rustls_result::NullParameter; - } - CStr::from_ptr(server_name) + let Some(server_name) = NonNull::new(server_name.cast_mut()) else { + return rustls_result::NullParameter; }; + let server_name = unsafe { CStr::from_ptr(server_name.as_ptr()) }; let Ok(server_name) = server_name.to_str() else { return rustls_result::InvalidDnsNameError; }; diff --git a/librustls/src/connection.rs b/librustls/src/connection.rs index 1e55e587..26c77a15 100644 --- a/librustls/src/connection.rs +++ b/librustls/src/connection.rs @@ -1,5 +1,5 @@ use std::io::{ErrorKind, Read, Write}; -use std::{ffi::c_void, ptr::null}; +use std::{ffi::c_void, ptr::NonNull, ptr::null}; use std::{ptr::null_mut, slice}; use libc::{EINVAL, EIO, size_t}; @@ -146,9 +146,9 @@ impl rustls_connection { ) -> rustls_io_result { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_io_result(EINVAL); - } + }; let callback = try_callback!(callback); let mut reader = CallbackReader { callback, userdata }; @@ -157,7 +157,7 @@ impl rustls_connection { Err(e) => return rustls_io_result(e.raw_os_error().unwrap_or(EIO)), }; unsafe { - *out_n = n_read; + out_n.write(n_read); } rustls_io_result(0) @@ -183,9 +183,9 @@ impl rustls_connection { ) -> rustls_io_result { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_io_result(EINVAL); - } + }; let callback = try_callback!(callback); let mut writer = CallbackWriter { callback, userdata }; @@ -194,7 +194,7 @@ impl rustls_connection { Err(e) => return rustls_io_result(e.raw_os_error().unwrap_or(EIO)), }; unsafe { - *out_n = n_written; + out_n.write(n_written); } rustls_io_result(0) @@ -220,9 +220,9 @@ impl rustls_connection { ) -> rustls_io_result { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_io_result(EINVAL); - } + }; let callback = try_callback!(callback); let mut writer = VectoredCallbackWriter { callback, userdata }; @@ -231,7 +231,7 @@ impl rustls_connection { Err(e) => return rustls_io_result(e.raw_os_error().unwrap_or(EIO)), }; unsafe { - *out_n = n_written; + out_n.write(n_written); } rustls_io_result(0) @@ -391,17 +391,19 @@ impl rustls_connection { ) { ffi_panic_boundary! { let conn = try_ref_from_ptr!(conn); - if protocol_out.is_null() || protocol_out_len.is_null() { + let (Some(protocol_out), Some(protocol_out_len)) = + (NonNull::new(protocol_out), NonNull::new(protocol_out_len)) + else { return; - } + }; match conn.alpn_protocol() { Some(p) => unsafe { - *protocol_out = p.as_ptr(); - *protocol_out_len = p.len(); + protocol_out.write(p.as_ptr()); + protocol_out_len.write(p.len()); }, None => unsafe { - *protocol_out = null(); - *protocol_out_len = 0; + protocol_out.write(null()); + protocol_out_len.write(0); }, } } @@ -527,15 +529,15 @@ impl rustls_connection { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); let write_buf = try_slice!(buf, count); - if out_n.is_null() { + let Some(out_n) = NonNull::new(out_n) else { return rustls_result::NullParameter; - } + }; let n_written = match conn.writer().write(write_buf) { Ok(n) => n, Err(_) => return rustls_result::Io, }; unsafe { - *out_n = n_written; + out_n.write(n_written); } rustls_result::Ok } @@ -562,16 +564,16 @@ impl rustls_connection { ) -> rustls_result { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); - if buf.is_null() { + let Some(buf) = NonNull::new(buf) else { return rustls_result::NullParameter; - } - if out_n.is_null() { + }; + let Some(out_n) = NonNull::new(out_n) else { return rustls_result::NullParameter; - } + }; // Safety: the memory pointed at by buf must be initialized // (required by documentation of this function). - let read_buf = try_slice_mut!(buf, count); + let read_buf = try_slice_mut!(buf.as_ptr(), count); let n_read = match conn.reader().read(read_buf) { Ok(n) => n, @@ -584,7 +586,7 @@ impl rustls_connection { Err(_) => return rustls_result::Io, }; unsafe { - *out_n = n_read; + out_n.write(n_read); } rustls_result::Ok } @@ -613,10 +615,11 @@ impl rustls_connection { ) -> rustls_result { ffi_panic_boundary! { let conn = try_mut_from_ptr!(conn); - if buf.is_null() || out_n.is_null() { + let (Some(buf), Some(out_n)) = (NonNull::new(buf), NonNull::new(out_n)) else { return rustls_result::NullParameter; - } - let mut read_buf: std::io::BorrowedBuf<'_, u8> = try_slice_mut!(buf, count).into(); + }; + let mut read_buf: std::io::BorrowedBuf<'_, u8> = + try_slice_mut!(buf.as_ptr(), count).into(); let n_read = match conn.reader().read_buf(read_buf.unfilled()) { Ok(()) => read_buf.filled().len(), @@ -629,7 +632,7 @@ impl rustls_connection { Err(_) => return rustls_result::Io, }; unsafe { - *out_n = n_read; + out_n.write(n_read); } rustls_result::Ok } diff --git a/librustls/src/error.rs b/librustls/src/error.rs index b520ee0d..a6918f7d 100644 --- a/librustls/src/error.rs +++ b/librustls/src/error.rs @@ -1,5 +1,6 @@ use std::cmp::min; use std::fmt::Display; +use std::ptr::NonNull; use std::sync::Arc; use libc::{c_char, c_uint, size_t}; @@ -224,17 +225,18 @@ impl rustls_result { out_n: *mut size_t, ) { ffi_panic_boundary! { - if buf.is_null() { + let (Some(buf), Some(out_n)) = (NonNull::new(buf), NonNull::new(out_n)) else { return; - } - if out_n.is_null() { - return; - } + }; let error_str = rustls_result::from(result).to_string(); let out_len = min(len, error_str.len()); unsafe { - std::ptr::copy_nonoverlapping(error_str.as_ptr() as *mut c_char, buf, out_len); - *out_n = out_len; + std::ptr::copy_nonoverlapping( + error_str.as_ptr() as *mut c_char, + buf.as_ptr(), + out_len, + ); + out_n.write(out_len); } } } diff --git a/librustls/src/ffi.rs b/librustls/src/ffi.rs index 07c72f9e..48fd2641 100644 --- a/librustls/src/ffi.rs +++ b/librustls/src/ffi.rs @@ -1,4 +1,5 @@ use std::mem; +use std::ptr::NonNull; use std::sync::Arc; /// Used to mark that pointer to a [`Castable`]'s underlying `Castable::RustType` is provided @@ -103,10 +104,8 @@ pub(crate) fn clone_arc(ptr: *const C) -> Option> where C: Castable, { - if ptr.is_null() { - return None; - } - let rs_typed = cast_const_ptr::(ptr); + let ptr = NonNull::new(ptr.cast_mut())?; + let rs_typed = cast_const_ptr::(ptr.as_ptr()); let r = unsafe { Arc::from_raw(rs_typed) }; let val = Arc::clone(&r); mem::forget(r); @@ -126,10 +125,8 @@ pub(crate) fn to_box(ptr: *mut C) -> Option> where C: Castable, { - if ptr.is_null() { - return None; - } - let rs_typed = cast_mut_ptr(ptr); + let ptr = NonNull::new(ptr)?; + let rs_typed = cast_mut_ptr(ptr.as_ptr()); unsafe { Some(Box::from_raw(rs_typed)) } } @@ -148,11 +145,9 @@ pub(crate) fn free_arc(ptr: *const C) where C: Castable, { - if ptr.is_null() { - return; + if let Some(ptr) = NonNull::new(ptr.cast_mut()) { + drop(unsafe { Arc::from_raw(cast_const_ptr(ptr.as_ptr().cast_const())) }); } - let rs_typed = cast_const_ptr(ptr); - drop(unsafe { Arc::from_raw(rs_typed) }); } /// Convert a mutable pointer to a [`Castable`] to an optional `Box` over the underlying @@ -248,10 +243,8 @@ pub(crate) use try_mut_from_ptr; /// /// 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) }, - } + let mut from = NonNull::new(from)?; + unsafe { Some(from.as_mut()) } } /// If the provided pointer to a pointer to a [`Castable`] is non-null, convert it to a mutable @@ -306,10 +299,8 @@ pub(crate) fn try_from_ptr<'a, C>(from: *mut *const C) -> Option<&'a mut *const where C: Castable, { - match from.is_null() { - true => None, - false => unsafe { Some(&mut *from) }, - } + let mut from = NonNull::new(from)?; + unsafe { Some(from.as_mut()) } } /// If the provided pointer to pointer to a [`Castable`] is non-null, convert it to a mutable @@ -373,10 +364,9 @@ pub(crate) use try_box_from_ptr; macro_rules! try_slice { ( $ptr:expr, $count:expr ) => { - if $ptr.is_null() { - return $crate::panic::NullParameterOrDefault::value(); - } else { - unsafe { slice::from_raw_parts($ptr, $count) } + match ::std::ptr::NonNull::new($ptr.cast_mut()) { + Some(ptr) => unsafe { slice::from_raw_parts(ptr.as_ptr().cast_const(), $count) }, + None => return $crate::panic::NullParameterOrDefault::value(), } }; } @@ -385,10 +375,9 @@ pub(crate) use try_slice; macro_rules! try_slice_mut { ( $ptr:expr, $count:expr ) => { - if $ptr.is_null() { - return $crate::panic::NullParameterOrDefault::value(); - } else { - unsafe { slice::from_raw_parts_mut($ptr, $count) } + match ::std::ptr::NonNull::new($ptr) { + Some(ptr) => unsafe { slice::from_raw_parts_mut(ptr.as_ptr(), $count) }, + None => return $crate::panic::NullParameterOrDefault::value(), } }; } diff --git a/librustls/src/server.rs b/librustls/src/server.rs index 9ca65101..376ed083 100644 --- a/librustls/src/server.rs +++ b/librustls/src/server.rs @@ -1,5 +1,6 @@ use std::ffi::c_void; use std::fmt::{Debug, Formatter}; +use std::ptr::NonNull; use std::slice; use std::sync::Arc; @@ -482,11 +483,11 @@ impl rustls_server_config { conn_out: *mut *mut rustls_connection, ) -> rustls_result { ffi_panic_boundary! { - if conn_out.is_null() { + let Some(mut conn_out) = NonNull::new(conn_out) else { return rustls_result::NullParameter; - } + }; let config = try_clone_arc!(config); - let conn_out = try_mut_from_ptr_ptr!(conn_out); + let conn_out = unsafe { conn_out.as_mut() }; let server_connection = match ServerConnection::new(config) { Ok(sc) => sc, @@ -771,15 +772,15 @@ pub extern "C" fn rustls_client_hello_select_certified_key( hello.signature_schemes.data, hello.signature_schemes.len )); - if out_key.is_null() { + let Some(out_key) = NonNull::new(out_key) else { return rustls_result::NullParameter; - } + }; let keys_ptrs = try_slice!(certified_keys, certified_keys_len); for &key_ptr in keys_ptrs { let key_ref = try_ref_from_ptr!(key_ptr); if key_ref.key.choose_scheme(&schemes).is_some() { unsafe { - *out_key = key_ptr; + out_key.write(key_ptr); } return rustls_result::Ok; }