Fix thread-safety of own_back<T>() when use lender feature, closing issue #14

Author: jhg

src/lender.rs

@@ -27,6 +27,15 @@ pub(super) fn lent_type_of<T>(pointer: *const T) -> Option<TypeId> {
     lent_pointers.get(&(pointer as usize)).copied()
 }
 
+fn writable_lent_pointers() -> RwLockWriteGuard<'static, HashMap<usize, TypeId>> {
+    let Ok(lent_pointers) = LENT_POINTERS.write() else {
+        log::error!("RwLock poisoned, it is not possible to add or remove pointers");
+        unreachable!("RwLock poisoned, it is not possible to add or remove pointers");
+    };
+
+    lent_pointers
+}
+
 /// Use only when lend memory as a [`raw`](crate::raw) pointer.
 ///
 /// # Panics
@@ -53,15 +62,18 @@ pub(super) fn lend<T: 'static>(pointer: *const T) -> Result<(), PointerError> {
 /// If the [`RwLock`] used is poisoned, but it only happens if a panic happens
 /// while holding it. And it's specially reviewed and in a small module to
 /// avoid panics while holding it.
-pub(super) fn retrieve<T>(pointer: *const T) {
-    writable_lent_pointers().remove(&(pointer as usize));
-}
-
-fn writable_lent_pointers() -> RwLockWriteGuard<'static, HashMap<usize, TypeId>> {
-    let Ok(lent_pointers) = LENT_POINTERS.write() else {
-        log::error!("RwLock poisoned, it is not possible to add or remove pointers");
-        unreachable!();
-    };
-
-    lent_pointers
+pub(super) fn retrieve<T: 'static>(pointer: *const T) -> Result<(), PointerError> {
+    match writable_lent_pointers().remove(&(pointer as usize)) {
+        Some(type_id) if type_id != TypeId::of::<T>() => {
+            log::error!(
+                "Using a pointer with a different type as an opaque pointer to Rust's data"
+            );
+            Err(PointerError::InvalidType)
+        }
+        None => {
+            log::error!("Using an invalid pointer as an opaque pointer to Rust's data");
+            Err(PointerError::Invalid)
+        }
+        _ => Ok(()),
+    }
 }

src/lib.rs

@@ -63,14 +63,14 @@ pub fn raw<T: 'static>(data: T) -> Result<*mut T, PointerError> {
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub unsafe fn own_back<T: 'static>(pointer: *mut T) -> Result<T, PointerError> {
     validation::not_null_pointer(pointer)?;
+
+    // TODO: Simplify and optimize this.
     #[cfg(all(feature = "std", feature = "lender"))]
     validation::lent_pointer(pointer)?;
-    let boxed = { Box::from_raw(pointer) };
-
     #[cfg(all(feature = "std", feature = "lender"))]
-    lender::retrieve(pointer);
+    lender::retrieve(pointer)?;
 
-    Ok(*boxed)
+    Ok(*Box::from_raw(pointer))
 }
 
 /// Reference to an object but without to own it.

tests/pointer.rs

@@ -43,6 +43,16 @@ fn own_back_invalid_type() {
     unsafe { opaque_pointer::own_back(pointer as *mut TestIt).unwrap() };
 }
 
+#[cfg(all(feature = "std", feature = "lender"))]
+#[test]
+fn own_back_twice_error() {
+    let pointer = opaque_pointer::raw(TestIt::new(2)).unwrap();
+    unsafe { opaque_pointer::own_back(pointer).unwrap() };
+
+    let error = unsafe { opaque_pointer::own_back(pointer).unwrap_err() };
+    assert_eq!(error, PointerError::Invalid);
+}
+
 #[test]
 fn immutable_reference() {
     let pointer = opaque_pointer::raw(TestIt::new(2)).unwrap();

tests/thread_safety.rs

@@ -0,0 +1,29 @@
+use std::thread;
+use std::time::Duration;
+use opaque_pointer;
+
+#[cfg(all(feature = "std", feature = "lender"))]
+#[test]
+fn own_back() {
+    let for_test = 0;
+    let pointer = opaque_pointer::raw(for_test).unwrap();
+
+    let mut threads = Vec::new();
+    for _ in 0..1000 {
+        let pointer = pointer as usize;
+        threads.push(thread::spawn(move || {
+            thread::sleep(Duration::from_millis(5));
+            unsafe { opaque_pointer::own_back(pointer as *mut i32).is_ok() }
+        }));
+    }
+
+    // If all works well, only one thread will be able to own_back the pointer.
+    let mut counter = 0;
+    for thread in threads {
+        if let Ok(true) = thread.join() {
+            counter += 1;
+        }
+    }
+
+    assert_eq!(1, counter);
+}