Overhaul many parts of unsafe semantics using zerocopy and NonNulls (messense#112)

* Overhaul many parts of unsafe semantics using zerocopy and NonNulls

* fmt

* Mark FFI analogues as repr(C) to ensure consistent representation

* fix double-free when calling PdfPage::try_from(Page)

* Fix up from_raw and from_non_null fns for Page to be more accurate to names

* Add new FzArray type to ensure fz-allocated Arrays are dropped correctly

* Make fzarray fields non-pub

* Fix some doc links

* Remove unused import

* Finally fix ASAN issues

* fmt

* Fix compilation on older targets

* Actually fix compilation this time lmao

* Make FzArray default impl invariant over T

* Make build error clearer by reporting src and dst

* Don't try to recursively copy .git dir when building mupdf-sys

* fmt

* *Actually* don't copy over .git directory was building

Author: itsjunetime

.github/workflows/CI.yml

@@ -101,6 +101,7 @@ jobs:
           cargo test -Zbuild-std --target x86_64-unknown-linux-gnu --features serde
         env:
           RUSTFLAGS: -Zsanitizer=address
+          LSAN_OPTIONS: report_objects=1
 
   valgrind:
     name: Valgrind

Cargo.toml

@@ -50,6 +50,7 @@ once_cell = "1.3.1"
 num_enum = "0.7.0"
 bitflags = "2.0.2"
 serde = { version = "1.0.201", features = ["derive"], optional = true }
+zerocopy = { version = "0.8.17", features = ["derive"] }
 
 [dependencies.font-kit]
 version = "0.14.1"

mupdf-sys/Cargo.toml

@@ -93,3 +93,4 @@ pkg-config = "0.3"
 regex = "1.11"
 
 [dependencies]
+zerocopy = { version = "0.8.17", features = ["derive"] }

mupdf-sys/build.rs

@@ -1,4 +1,5 @@
 use std::env;
+use std::ffi::OsStr;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::process::Stdio;
@@ -15,24 +16,29 @@ const SKIP_FONTS: [&str; 6] = [
 ];
 
 macro_rules! t {
-    ($e:expr) => {
+    ($e:expr ) => {
         match $e {
             Ok(n) => n,
             Err(e) => panic!("\n{} failed with {}\n", stringify!($e), e),
         }
     };
 }
 
-fn cp_r(dir: &Path, dest: &Path) {
+fn cp_r(dir: &Path, dest: &Path, excluding_dir_names: &'static [&'static str]) {
     for entry in t!(fs::read_dir(dir)) {
         let entry = t!(entry);
         let path = entry.path();
         let dst = dest.join(path.file_name().expect("Failed to get filename of path"));
         if t!(fs::metadata(&path)).is_file() {
-            t!(fs::copy(path, dst));
-        } else {
+            fs::copy(&path, &dst)
+                .unwrap_or_else(|e| panic!("Couldn't fs::copy {path:?} to {dst:?}: {e}"));
+        } else if path
+            .file_name()
+            .and_then(OsStr::to_str)
+            .is_none_or(|dst| !excluding_dir_names.contains(&dst))
+        {
             t!(fs::create_dir_all(&dst));
-            cp_r(&path, &dst);
+            cp_r(&path, &dst, excluding_dir_names);
         }
     }
 }
@@ -54,7 +60,7 @@ fn build_libmupdf() {
 
     let current_dir = env::current_dir().unwrap();
     let mupdf_src_dir = current_dir.join("mupdf");
-    cp_r(&mupdf_src_dir, &build_dir);
+    cp_r(&mupdf_src_dir, &build_dir, &[".git"]);
 
     let mut build = cc::Build::new();
     #[cfg(not(feature = "xps"))]
@@ -219,7 +225,7 @@ fn build_libmupdf() {
 
     let current_dir = env::current_dir().unwrap();
     let mupdf_src_dir = current_dir.join("mupdf");
-    cp_r(&mupdf_src_dir, &build_dir);
+    cp_r(&mupdf_src_dir, &build_dir, &[".git"]);
 
     let msbuild = cc::windows_registry::find(target.as_str(), "msbuild.exe");
     if let Some(mut msbuild) = msbuild {
@@ -432,6 +438,23 @@ impl bindgen::callbacks::ParseCallbacks for Callback {
         }
         Some(output)
     }
+
+    fn add_derives(&self, info: &bindgen::callbacks::DeriveInfo<'_>) -> Vec<String> {
+        static ZEROCOPY_TYPES: [&str; 2] = ["fz_point", "fz_quad"];
+
+        if ZEROCOPY_TYPES.contains(&info.name) {
+            [
+                "zerocopy::FromBytes",
+                "zerocopy::IntoBytes",
+                "zerocopy::Immutable",
+            ]
+            .into_iter()
+            .map(ToString::to_string)
+            .collect()
+        } else {
+            vec![]
+        }
+    }
 }
 
 fn main() {

src/array.rs

@@ -0,0 +1,123 @@
+use std::{
+    ffi::c_void,
+    ops::{Deref, DerefMut},
+    ptr::{self, NonNull},
+    slice,
+};
+
+use mupdf_sys::fz_free;
+
+use crate::context;
+
+/// Essentially a [`Box`]`<[T], A>` with `fz_calloc` as the allocator. Necessary until the
+/// allocator API is stable, as we need to be able to free this with `fz_free` instead of the
+/// system allocator.
+// An important note about `fz_calloc`: If a function which allocates from `fz_calloc` gives you a
+// pointer, allocated from `fz_calloc`, but says the length of items behind it is 0, you still have
+// to `fz_free` that pointer. It's probably lying about the length behind it being 0 - it was
+// probably part of an allocation bigger than 0, but of which 0 bytes were actually written to.
+pub struct FzArray<T> {
+    ptr: Option<NonNull<T>>,
+    len: usize,
+}
+
+impl<T> Default for FzArray<T> {
+    fn default() -> Self {
+        Self { ptr: None, len: 0 }
+    }
+}
+
+impl<T> FzArray<T> {
+    /// # Safety
+    ///
+    /// * `ptr` must point to an array of at least `len` instances of `T`. There may be more than
+    ///   `len` instances, but only `len` will be accessed by this struct.
+    ///
+    /// * If `len > 0`, the memory it points to also must be allocated by `fz_calloc` inside a
+    ///   mupdf FFI call. `ptr` may be dangling or not well-aligned if `len == 0`
+    pub(crate) unsafe fn from_parts(ptr: NonNull<T>, len: usize) -> Self {
+        Self {
+            ptr: Some(ptr),
+            len,
+        }
+    }
+
+    /// # Safety
+    ///
+    /// * It must be valid to call [`FzArray::from_parts`] with the ptr stored inside `self` and
+    ///   the `len` specified in this call
+    pub(crate) unsafe fn set_len(&mut self, len: usize) {
+        self.len = len;
+    }
+}
+
+impl<T> Deref for FzArray<T> {
+    type Target = [T];
+    fn deref(&self) -> &Self::Target {
+        match self.ptr {
+            // SAFETY: `self.ptr.as_ptr()` is not non-null (as it's a NonNull) and the creator has
+            // promised us that it does point to a valid slice. Also, if it does point to a
+            Some(ptr) => unsafe { slice::from_raw_parts(ptr.as_ptr(), self.len) },
+            None => &[],
+        }
+    }
+}
+
+impl<T> DerefMut for FzArray<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        match self.ptr.as_mut() {
+            Some(ptr) => {
+                let ptr = unsafe { ptr.as_mut() };
+                unsafe { slice::from_raw_parts_mut(ptr, self.len) }
+            }
+            None => &mut [],
+        }
+    }
+}
+
+impl<T> Drop for FzArray<T> {
+    fn drop(&mut self) {
+        if let Some(ptr) = self.ptr {
+            // SAFETY: Upheld by constructor - this must point to something allocated by fz_calloc
+            unsafe { fz_free(context(), ptr.as_ptr() as *mut c_void) };
+        }
+    }
+}
+
+impl<T> IntoIterator for FzArray<T> {
+    type IntoIter = FzIter<T>;
+    type Item = T;
+    fn into_iter(self) -> Self::IntoIter {
+        let next_item_and_end = self.ptr.map(|p| {
+            // Would be nice to use `.addr()` but it seems CI doesn't have 1.84 yet, and that's
+            // what stabilized the strict provenance APIs
+            let end = unsafe { p.add(self.len) }.as_ptr() as usize;
+            (p, end)
+        });
+        FzIter {
+            _kept_to_be_dropped: self,
+            next_item_and_end,
+        }
+    }
+}
+
+pub struct FzIter<T> {
+    _kept_to_be_dropped: FzArray<T>,
+    next_item_and_end: Option<(NonNull<T>, usize)>,
+}
+
+impl<T> Iterator for FzIter<T> {
+    type Item = T;
+    fn next(&mut self) -> Option<Self::Item> {
+        let (next_item, end_addr) = self.next_item_and_end.as_mut()?;
+
+        // Same thing as above with `.addr()`
+        if next_item.as_ptr() as usize == *end_addr {
+            return None;
+        }
+
+        let ret = unsafe { ptr::read(next_item.as_ptr()) };
+        *next_item = unsafe { next_item.add(1) };
+        Some(ret)
+    }
+}

src/document.rs

@@ -214,10 +214,9 @@ impl Document {
     }
 
     pub fn load_page(&self, page_no: i32) -> Result<Page, Error> {
-        unsafe {
-            let inner = ffi_try!(mupdf_load_page(context(), self.inner, page_no));
-            Ok(Page::from_raw(inner))
-        }
+        let fz_page = unsafe { ffi_try!(mupdf_load_page(context(), self.inner, page_no)) };
+        // SAFETY: We're trusting the FFI layer here to provide a valid pointer
+        unsafe { Page::from_raw(fz_page) }
     }
 
     pub fn pages(&self) -> Result<PageIter, Error> {

src/error.rs

@@ -1,6 +1,8 @@
 use std::fmt;
 use std::io;
 use std::ffi::NulError;
+use std::num::TryFromIntError;
+use std::ptr::NonNull;
 
 use mupdf_sys::*;
 
@@ -24,48 +26,54 @@ impl std::error::Error for MuPdfError {}
 
 /// # Safety
 ///
-/// * `error` must point to a non-null, well-aligned initialized instance of `mupdf_error_t`.
+/// * `ptr` must point to a valid, well-aligned instance of [`mupdf_error_t`].
 ///
-/// * `(*error).message` must point to a null-terminated c-string.
-pub unsafe fn ffi_error(err: *mut mupdf_error_t) -> MuPdfError {
+/// * The pointers stored in this [`mupdf_error_t`] must also be non-null, well-aligned, and point
+///   to valid instances of what they claim to represent.
+///
+/// * The [`field@mupdf_error_t::message`] ptr in `ptr` must point to a null-terminated c-string
+pub unsafe fn ffi_error(ptr: NonNull<mupdf_error_t>) -> MuPdfError {
     use std::ffi::CStr;
 
-    let code = (*err).type_;
-    let c_msg = (*err).message;
-    let c_str = CStr::from_ptr(c_msg);
-    let message = format!("{}", c_str.to_string_lossy());
-    mupdf_drop_error(err);
+    // SAFETY: Upheld by caller
+    let err = unsafe { *ptr.as_ptr() };
+    let code = err.type_;
+    let c_msg = err.message;
+
+    // SAFETY: Upheld by caller
+    let c_str = unsafe { CStr::from_ptr(c_msg) };
+    let message = c_str.to_string_lossy().to_string();
+
+    // SAFETY: Upheld by caller; if it's pointing to a valid instance then it can be dropped
+    unsafe { mupdf_drop_error(ptr.as_ptr()) };
     MuPdfError { code, message }
 }
 
 macro_rules! ffi_try {
     ($func:ident($($arg:expr),+)) => ({
         use std::ptr;
         let mut err = ptr::null_mut();
-        let res = $func($($arg),+, &mut err);
-        if !err.is_null() {
+        // SAFETY: Upheld by the caller of the macro
+        let res = $func($($arg),+, (&mut err) as *mut *mut ::mupdf_sys::mupdf_error_t);
+        if let Some(err) = ::core::ptr::NonNull::new(err) {
+            // SAFETY: We're trusting the FFI call to provide us with a valid ptr if it is not
+            // null.
             return Err($crate::ffi_error(err).into());
         }
         res
     });
-    ($func:ident()) => ({
-        use std::ptr;
-        let mut err = ptr::null_mut();
-        let res = $func(&mut err);
-        if !err.is_null() {
-            return Err($crate::ffi_error(err).into());
-        }
-        res
-    })
 }
 
 #[derive(Debug)]
+#[non_exhaustive]
 pub enum Error {
     Io(io::Error),
     InvalidLanguage(String),
     InvalidPdfDocument,
     MuPdf(MuPdfError),
     Nul(NulError),
+    IntConversion(TryFromIntError),
+    UnexpectedNullPtr
 }
 
 impl fmt::Display for Error {
@@ -76,6 +84,8 @@ impl fmt::Display for Error {
             Error::InvalidPdfDocument => write!(f, "invalid pdf document"),
             Error::MuPdf(ref err) => err.fmt(f),
             Error::Nul(ref err) => err.fmt(f),
+            Error::IntConversion(ref err) => err.fmt(f),
+            Error::UnexpectedNullPtr => write!(f, "An FFI function call returned a null ptr when we expected a non-null ptr")
         }
     }
 }
@@ -99,3 +109,9 @@ impl From<NulError> for Error {
         Self::Nul(err)
     }
 }
+
+impl From<TryFromIntError> for Error {
+    fn from(value: TryFromIntError) -> Self {
+        Self::IntConversion(value)
+    }
+}