Add a wrapper for slice::from_raw_parts (#1644)

* Add a wrapper for slice::from_raw_parts

And add a lint to prevent the method from being invoked directly.

The documentation on the wrapper explains everything.

* Add a newline to the file

* Cleanup

---------

Co-authored-by: Lars Eggert <[email protected]>

clippy.toml

@@ -0,0 +1,3 @@
+disallowed-methods = [
+    { path = "std::slice::from_raw_parts", reason = "see null_safe_slice" }
+]

neqo-crypto/src/agent.rs

@@ -33,6 +33,7 @@ use crate::{
     ech,
     err::{is_blocked, secstatus_to_res, Error, PRErrorCode, Res},
     ext::{ExtensionHandler, ExtensionTracker},
+    null_safe_slice,
     p11::{self, PrivateKey, PublicKey},
     prio,
     replay::AntiReplay,
@@ -897,7 +898,7 @@ impl Client {
         let resumption = arg.cast::<Vec<ResumptionToken>>().as_mut().unwrap();
         let len = usize::try_from(len).unwrap();
         let mut v = Vec::with_capacity(len);
-        v.extend_from_slice(std::slice::from_raw_parts(token, len));
+        v.extend_from_slice(null_safe_slice(token, len));
         qinfo!(
             [format!("{fd:p}")],
             "Got resumption token {}",
@@ -1105,11 +1106,7 @@ impl Server {
         }
 
         let check_state = arg.cast::<ZeroRttCheckState>().as_mut().unwrap();
-        let token = if client_token.is_null() {
-            &[]
-        } else {
-            std::slice::from_raw_parts(client_token, usize::try_from(client_token_len).unwrap())
-        };
+        let token = null_safe_slice(client_token, usize::try_from(client_token_len).unwrap());
         match check_state.checker.check(token) {
             ZeroRttCheckResult::Accept => ssl::SSLHelloRetryRequestAction::ssl_hello_retry_accept,
             ZeroRttCheckResult::Fail => ssl::SSLHelloRetryRequestAction::ssl_hello_retry_fail,

neqo-crypto/src/agentio.rs

@@ -20,7 +20,7 @@ use neqo_common::{hex, hex_with_len, qtrace};
 use crate::{
     constants::{ContentType, Epoch},
     err::{nspr, Error, PR_SetError, Res},
-    prio, ssl,
+    null_safe_slice, prio, ssl,
 };
 
 // Alias common types.
@@ -100,7 +100,7 @@ impl RecordList {
     ) -> ssl::SECStatus {
         let records = arg.cast::<Self>().as_mut().unwrap();
 
-        let slice = std::slice::from_raw_parts(data, len as usize);
+        let slice = null_safe_slice(data, len);
         records.append(epoch, ContentType::try_from(ct).unwrap(), slice);
         ssl::SECSuccess
     }
@@ -178,6 +178,7 @@ impl AgentIoInput {
             return Err(Error::NoDataAvailable);
         }
 
+        #[allow(clippy::disallowed_methods)] // We just checked if this was empty.
         let src = unsafe { std::slice::from_raw_parts(self.input, amount) };
         qtrace!([self], "read {}", hex(src));
         let dst = unsafe { std::slice::from_raw_parts_mut(buf, amount) };
@@ -232,7 +233,7 @@ impl AgentIo {
 
     // Stage output from TLS into the output buffer.
     fn save_output(&mut self, buf: *const u8, count: usize) {
-        let slice = unsafe { std::slice::from_raw_parts(buf, count) };
+        let slice = unsafe { null_safe_slice(buf, count) };
         qtrace!([self], "save output {}", hex(slice));
         self.output.extend_from_slice(slice);
     }

neqo-crypto/src/cert.rs

@@ -7,13 +7,13 @@
 use std::{
     convert::TryFrom,
     ptr::{addr_of, NonNull},
-    slice,
 };
 
 use neqo_common::qerror;
 
 use crate::{
     err::secstatus_to_res,
+    null_safe_slice,
     p11::{CERTCertListNode, CERT_GetCertificateDer, CertList, Item, SECItem, SECItemArray},
     ssl::{
         PRFileDesc, SSL_PeerCertificateChain, SSL_PeerSignedCertTimestamps,
@@ -52,7 +52,7 @@ fn stapled_ocsp_responses(fd: *mut PRFileDesc) -> Option<Vec<Vec<u8>>> {
             };
             for idx in 0..len {
                 let itemp: *const SECItem = unsafe { ocsp_ptr.as_ref().items.offset(idx).cast() };
-                let item = unsafe { slice::from_raw_parts((*itemp).data, (*itemp).len as usize) };
+                let item = unsafe { null_safe_slice((*itemp).data, (*itemp).len) };
                 ocsp_helper.push(item.to_owned());
             }
             Some(ocsp_helper)
@@ -68,9 +68,8 @@ fn signed_cert_timestamp(fd: *mut PRFileDesc) -> Option<Vec<u8>> {
             if unsafe { sct_ptr.as_ref().len == 0 || sct_ptr.as_ref().data.is_null() } {
                 Some(Vec::new())
             } else {
-                let sct_slice = unsafe {
-                    slice::from_raw_parts(sct_ptr.as_ref().data, sct_ptr.as_ref().len as usize)
-                };
+                let sct_slice =
+                    unsafe { null_safe_slice(sct_ptr.as_ref().data, sct_ptr.as_ref().len) };
                 Some(sct_slice.to_owned())
             }
         }
@@ -105,7 +104,7 @@ impl<'a> Iterator for &'a mut CertificateInfo {
         let cert = unsafe { *self.cursor }.cert;
         secstatus_to_res(unsafe { CERT_GetCertificateDer(cert, &mut item) })
             .expect("getting DER from certificate should work");
-        Some(unsafe { std::slice::from_raw_parts(item.data, item.len as usize) })
+        Some(unsafe { null_safe_slice(item.data, item.len) })
     }
 }
 

neqo-crypto/src/ech.rs

@@ -15,7 +15,7 @@ use neqo_common::qtrace;
 
 use crate::{
     err::{ssl::SSL_ERROR_ECH_RETRY_WITH_ECH, Error, Res},
-    experimental_api,
+    experimental_api, null_safe_slice,
     p11::{
         self, Item, PrivateKey, PublicKey, SECITEM_FreeItem, SECItem, SECKEYPrivateKey,
         SECKEYPublicKey, Slot,
@@ -76,7 +76,7 @@ pub fn convert_ech_error(fd: *mut PRFileDesc, err: Error) -> Error {
             return Error::InternalError;
         }
         let buf = unsafe {
-            let slc = std::slice::from_raw_parts(item.data, usize::try_from(item.len).unwrap());
+            let slc = null_safe_slice(item.data, item.len);
             let buf = Vec::from(slc);
             SECITEM_FreeItem(&mut item, PRBool::from(false));
             buf
@@ -101,8 +101,7 @@ pub fn generate_keys() -> Res<(PrivateKey, PublicKey)> {
 
     let oid_data = unsafe { p11::SECOID_FindOIDByTag(p11::SECOidTag::SEC_OID_CURVE25519) };
     let oid = unsafe { oid_data.as_ref() }.ok_or(Error::InternalError)?;
-    let oid_slc =
-        unsafe { std::slice::from_raw_parts(oid.oid.data, usize::try_from(oid.oid.len).unwrap()) };
+    let oid_slc = unsafe { null_safe_slice(oid.oid.data, oid.oid.len) };
     let mut params: Vec<u8> = Vec::with_capacity(oid_slc.len() + 2);
     params.push(u8::try_from(p11::SEC_ASN1_OBJECT_ID).unwrap());
     params.push(u8::try_from(oid.oid.len).unwrap());

neqo-crypto/src/ext.rs

@@ -16,6 +16,7 @@ use crate::{
     agentio::as_c_void,
     constants::{Extension, HandshakeMessage, TLS_HS_CLIENT_HELLO, TLS_HS_ENCRYPTED_EXTENSIONS},
     err::Res,
+    null_safe_slice,
     ssl::{
         PRBool, PRFileDesc, SECFailure, SECStatus, SECSuccess, SSLAlertDescription,
         SSLExtensionHandler, SSLExtensionWriter, SSLHandshakeType,
@@ -105,7 +106,7 @@ impl ExtensionTracker {
         alert: *mut SSLAlertDescription,
         arg: *mut c_void,
     ) -> SECStatus {
-        let d = std::slice::from_raw_parts(data, len as usize);
+        let d = null_safe_slice(data, len);
         #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
         Self::wrap_handler_call(arg, |handler| {
             // Cast is safe here because the message type is always part of the enum

neqo-crypto/src/lib.rs

@@ -37,6 +37,7 @@ mod ssl;
 mod time;
 
 use std::{
+    convert::TryFrom,
     ffi::CString,
     path::{Path, PathBuf},
     ptr::null,
@@ -200,3 +201,27 @@ pub fn assert_initialized() {
         .get()
         .expect("NSS not initialized with init or init_db");
 }
+
+/// NSS tends to return empty "slices" with a null pointer, which will cause
+/// `std::slice::from_raw_parts` to panic if passed directly.  This wrapper avoids
+/// that issue.  It also performs conversion for lengths, as a convenience.
+///
+/// # Panics
+/// If the provided length doesn't fit into a `usize`.
+///
+/// # Safety
+/// The caller must adhere to the safety constraints of `std::slice::from_raw_parts`,
+/// except that this will accept a null value for `data`.
+unsafe fn null_safe_slice<'a, T>(data: *const u8, len: T) -> &'a [u8]
+where
+    usize: TryFrom<T>,
+{
+    if data.is_null() {
+        &[]
+    } else if let Ok(len) = usize::try_from(len) {
+        #[allow(clippy::disallowed_methods)]
+        std::slice::from_raw_parts(data, len)
+    } else {
+        panic!("null_safe_slice: size overflow");
+    }
+}

neqo-crypto/src/p11.rs

@@ -20,7 +20,10 @@ use std::{
 
 use neqo_common::hex_with_len;
 
-use crate::err::{secstatus_to_res, Error, Res};
+use crate::{
+    err::{secstatus_to_res, Error, Res},
+    null_safe_slice,
+};
 
 #[allow(clippy::upper_case_acronyms)]
 #[allow(clippy::unreadable_literal)]
@@ -148,9 +151,7 @@ impl PrivateKey {
                 &mut key_item,
             )
         })?;
-        let slc = unsafe {
-            std::slice::from_raw_parts(key_item.data, usize::try_from(key_item.len).unwrap())
-        };
+        let slc = unsafe { null_safe_slice(key_item.data, key_item.len) };
         let key = Vec::from(slc);
         // The data that `key_item` refers to needs to be freed, but we can't
         // use the scoped `Item` implementation.  This is OK as long as nothing
@@ -206,7 +207,7 @@ impl SymKey {
         // This is accessing a value attached to the key, so we can treat this as a borrow.
         match unsafe { key_item.as_mut() } {
             None => Err(Error::InternalError),
-            Some(key) => Ok(unsafe { std::slice::from_raw_parts(key.data, key.len as usize) }),
+            Some(key) => Ok(unsafe { null_safe_slice(key.data, key.len) }),
         }
     }
 }
@@ -285,7 +286,7 @@ impl Item {
         let b = self.ptr.as_ref().unwrap();
         // Sanity check the type, as some types don't count bytes in `Item::len`.
         assert_eq!(b.type_, SECItemType::siBuffer);
-        let slc = std::slice::from_raw_parts(b.data, usize::try_from(b.len).unwrap());
+        let slc = null_safe_slice(b.data, b.len);
         Vec::from(slc)
     }
 }