Fix soundness of ContextWithCallbacks.

Change to use `NonNull` instead of `Box` due to the requirement that boxed values are not aliased. See rust-lang/unsafe-code-guidelines#326.
gpg-rs · Jan 5, 2024 · 4528d56 · 4528d56
1 parent 4b016f7
commit 4528d56
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 30 deletions.
diff --git a/src/bin/pinentry.rs b/src/bin/pinentry.rs
@@ -7,7 +7,7 @@ fn main() {
     let stdin = io::stdin();
     let mut lines = stdin.lock().lines();
     while let Some(Ok(cmd)) = lines.next() {
-        match cmd.split(' ').nth(0) {
+        match cmd.split(' ').next() {
             Some("GETPIN") => {
                 println!("D abc");
                 println!("OK");

diff --git a/src/context.rs b/src/context.rs
@@ -5,7 +5,7 @@ use std::{
     iter::FusedIterator,
     mem::ManuallyDrop,
     ops::{Deref, DerefMut},
-    ptr,
+    ptr::{self, NonNull as CoreNonNull},
     str::Utf8Error,
     time::Duration,
 };
@@ -1875,7 +1875,7 @@ impl Context {
         }
     }
 
-    fn get_result<R: crate::OpResult>(&self) -> Option<R> {
+    fn get_result<R: results::OpResult>(&self) -> Option<R> {
         R::from_context(self)
     }
 }
@@ -2028,9 +2028,9 @@ impl fmt::Debug for Signers<'_> {
 /// [`gpgme_ctx_t`](https://www.gnupg.org/documentation/manuals/gpgme/Contexts.html#Contexts)
 pub struct ContextWithCallbacks<'a> {
     inner: Context,
-    passphrase_hook: Option<Box<dyn Send + 'a>>,
-    progress_hook: Option<Box<dyn Send + 'a>>,
-    status_hook: Option<Box<dyn Send + 'a>>,
+    passphrase_hook: Option<CoreNonNull<dyn Send + 'a>>,
+    progress_hook: Option<CoreNonNull<dyn Send + 'a>>,
+    status_hook: Option<CoreNonNull<dyn Send + 'a>>,
 }
 
 impl Drop for ContextWithCallbacks<'_> {
@@ -2046,7 +2046,11 @@ impl<'a> ContextWithCallbacks<'a> {
     /// [`gpgme_set_passphrase_cb`](https://www.gnupg.org/documentation/manuals/gpgme/Passphrase-Callback.html#index-gpgme_005fset_005fpassphrase_005fcb)
     pub fn clear_passphrase_provider(&mut self) {
         (**self).clear_passphrase_provider();
-        self.passphrase_hook.take();
+        if let Some(hook) = self.passphrase_hook.take() {
+            unsafe {
+                drop(Box::from_raw(hook.as_ptr()));
+            }
+        }
     }
 
     /// Upstream documentation:
@@ -2062,15 +2066,19 @@ impl<'a> ContextWithCallbacks<'a> {
                 Some(callbacks::passphrase_cb::<P>),
                 ptr::addr_of_mut!(*hook).cast(),
             );
+            self.passphrase_hook = Some(CoreNonNull::new_unchecked(Box::into_raw(hook)));
         }
-        self.passphrase_hook = Some(hook);
     }
 
     /// Upstream documentation:
     /// [`gpgme_set_progress_cb`](https://www.gnupg.org/documentation/manuals/gpgme/Progress-Meter-Callback.html#index-gpgme_005fset_005fprogress_005fcb)
     pub fn clear_progress_reporter(&mut self) {
         (**self).clear_progress_reporter();
-        self.progress_hook.take();
+        if let Some(hook) = self.progress_hook.take() {
+            unsafe {
+                drop(Box::from_raw(hook.as_ptr()));
+            }
+        }
     }
 
     /// Upstream documentation:
@@ -2086,15 +2094,19 @@ impl<'a> ContextWithCallbacks<'a> {
                 Some(callbacks::progress_cb::<H>),
                 ptr::addr_of_mut!(*hook).cast(),
             );
+            self.progress_hook = Some(CoreNonNull::new_unchecked(Box::into_raw(hook)));
         }
-        self.progress_hook = Some(hook);
     }
 
     /// Upstream documentation:
     /// [`gpgme_set_status_cb`](https://www.gnupg.org/documentation/manuals/gpgme/Status-Message-Callback.html#index-gpgme_005fset_005fstatus_005fcb)
     pub fn clear_status_handler(&mut self) {
         (**self).clear_status_handler();
-        self.status_hook.take();
+        if let Some(hook) = self.status_hook.take() {
+            unsafe {
+                drop(Box::from_raw(hook.as_ptr()));
+            }
+        }
     }
 
     /// Upstream documentation:
@@ -2110,8 +2122,8 @@ impl<'a> ContextWithCallbacks<'a> {
                 Some(callbacks::status_cb::<H>),
                 ptr::addr_of_mut!(*hook).cast(),
             );
+            self.status_hook = Some(CoreNonNull::new_unchecked(Box::into_raw(hook)));
         }
-        self.status_hook = Some(hook);
     }
 
     /// Returns the inner `Context` object.

diff --git a/src/data.rs b/src/data.rs
@@ -228,6 +228,10 @@ impl<'data> Data<'data> {
 
     /// Upstream documentation:
     /// [`gpgme_data_new_from_stream`](https://www.gnupg.org/documentation/manuals/gpgme/File-Based-Data-Buffers.html#index-gpgme_005fdata_005fnew_005ffrom_005fstream)
+    ///
+    /// # Safety
+    ///
+    /// The provided `FILE` object must be valid.
     #[inline]
     pub unsafe fn from_raw_file(file: *mut libc::FILE) -> Result<Self> {
         crate::init();

diff --git a/src/engine.rs b/src/engine.rs
@@ -1,15 +1,8 @@
-use std::{
-    ffi::CStr,
-    fmt,
-    marker::PhantomData,
-    ptr,
-    str::Utf8Error,
-    sync::{RwLock, RwLockReadGuard},
-};
+use std::{ffi::CStr, fmt, marker::PhantomData, str::Utf8Error};
 
 use ffi;
 
-use crate::{utils::convert_err, Context, NonNull, Protocol, Result};
+use crate::{Context, NonNull, Protocol, Result};
 
 /// Upstream documentation:
 /// [`gpgme_engine_info_t`](https://www.gnupg.org/documentation/manuals/gpgme/Engine-Information.html#index-gpgme_005fengine_005finfo_005ft)

diff --git a/src/lib.rs b/src/lib.rs
@@ -345,8 +345,4 @@ impl Gpgme {
     }
 }
 
-unsafe trait OpResult: Clone {
-    fn from_context(ctx: &Context) -> Option<Self>;
-}
-
 type NonNull<T> = ptr::NonNull<<T as utils::Ptr>::Inner>;
diff --git a/src/results.rs b/src/results.rs
@@ -13,10 +13,13 @@ use libc;
 
 use crate::{
     notation::SignatureNotations, utils::convert_err, Context, Error, HashAlgorithm, ImportFlags,
-    KeyAlgorithm, NonNull, OpResult, Result, SignMode, SignatureNotation, SignatureSummary,
-    Validity,
+    KeyAlgorithm, NonNull, Result, SignMode, SignatureSummary, Validity,
 };
 
+pub(crate) trait OpResult: Clone {
+    fn from_context(ctx: &Context) -> Option<Self>;
+}
+
 macro_rules! impl_result {
     ($(#[$Attr:meta])* $Name:ident : $T:ty = $Constructor:expr) => {
         $(#[$Attr])*
@@ -44,7 +47,7 @@ macro_rules! impl_result {
             }
         }
 
-        unsafe impl OpResult for $Name {
+        impl OpResult for $Name {
             fn from_context(ctx: &Context) -> Option<Self> {
                 unsafe {
                     $Constructor(ctx.as_raw()).as_mut().map(|r| {

diff --git a/src/utils.rs b/src/utils.rs
@@ -3,10 +3,12 @@ use std::io::{self, prelude::*};
 use crate::Error;
 
 pub use cstr_argument::CStrArgument;
-pub type SmallVec<T> = ::smallvec::SmallVec<[T; 4]>;
+pub type SmallVec<T> = ::smallvec::SmallVec<[T; 8]>;
 
 macro_rules! impl_wrapper {
     ($T:ty$(, $Args:expr)*) => {
+        /// # Safety
+        /// The provided instance must be valid.
         #[inline]
         pub unsafe fn from_raw(raw: $T) -> Self {
             Self(NonNull::<$T>::new(raw).unwrap()$(, $Args)*)
@@ -30,6 +32,8 @@ macro_rules! impl_list_iterator {
         $Vis struct $Name<'a>(Option<$Item<'a>>);
 
         impl $Name<'_> {
+            /// # Safety
+            /// The provided instance must be valid.
             #[inline]
             pub unsafe fn from_list(first: $Raw) -> Self {
                 $Name(first.as_mut().map(|r| $Item::from_raw(r)))

diff --git a/tests/verify.rs b/tests/verify.rs
@@ -11,7 +11,7 @@ const TEST_MSG1: &[u8] = b"-----BEGIN PGP MESSAGE-----\n\
                            =Crq6\n\
                            -----END PGP MESSAGE-----\n";
 
-#[sealed_test(before = common::setup(), after = common::teardown())]
+#[sealed_test(before = common::setup())]
 fn test_signature_key() {
     let mut ctx = common::create_context();
     let mut output = Vec::new();
@@ -25,5 +25,6 @@ fn test_signature_key() {
             return;
         }
     }
+    common::teardown();
     panic!();
 }