Do not let an invalid external pointer crash the R session (#95)

CHANGELOG.md

@@ -3,6 +3,10 @@
 <!-- next-header -->
 ## [Unreleased] (ReleaseDate)
 
+### Fixed bugs
+
+* Reject invalid external pointers so that the R session doesn't crash.
+
 ## [v0.2.14] (2024-02-14)
 
 ### Fixed bugs

R-package/tests/testthat/test-invalid_pointer.R

@@ -0,0 +1,14 @@
+test_that("invalid pointer doesn't clash the session", {
+  dir.create("data", showWarnings = FALSE)
+  rds_file <- file.path("data", "person.rds")
+
+  if (file.exists(rds_file)) {
+    x <- readRDS(rds_file)
+    expect_error(x$name(), "Invalid external pointer")
+  } else {
+    x <- Person()
+    saveRDS(x, rds_file)
+
+    skip("This test is for the first run. Please rerun later.")
+  }
+})

savvy-bindgen/src/gen/rust.rs

@@ -33,7 +33,7 @@ impl SavvyFn {
                 #(#attrs)*
                 #[allow(non_snake_case)]
                 unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
-                    let self__ = savvy::get_external_pointer_addr(self__) as *mut #ty;
+                    let self__ = savvy::get_external_pointer_addr(self__)? as *mut #ty;
                     #(#stmts_additional)*
 
                     (*self__).#fn_name_orig(#(#args_pat),*)

src/error.rs

@@ -5,6 +5,7 @@ pub enum Error {
     UnexpectedType(String),
     NotScalar,
     Aborted(SEXP),
+    InvalidPointer,
     GeneralError(String),
 }
 
@@ -22,6 +23,7 @@ impl std::fmt::Display for Error {
             Error::UnexpectedType(type_name) => write!(f, "Unexpected type: {}", type_name),
             Error::NotScalar => write!(f, "Must be length 1 of non-missing value"),
             Error::Aborted(_) => write!(f, "Aborted due to some error"),
+            Error::InvalidPointer => write!(f, "Invalid external pointer"),
             Error::GeneralError(msg) => write!(f, "{}", msg),
         }
     }

src/sexp/external_pointer.rs

@@ -45,7 +45,12 @@ pub trait IntoExtPtrSexp: Sized {
         unsafe extern "C" fn finalizer<T>(x: SEXP) {
             // bring back the ownership to Rust's side so that Rust will drop
             // after this block ends.
-            let _ = Box::from_raw(R_ExternalPtrAddr(x) as *mut T);
+            let ptr = R_ExternalPtrAddr(x);
+
+            // the pointer can be null (e.g. https://github.com/pola-rs/r-polars/issues/851)
+            if !ptr.is_null() {
+                drop(Box::from_raw(ptr as *mut T));
+            }
 
             R_ClearExternalPtr(x);
         }
@@ -73,8 +78,16 @@ pub trait IntoExtPtrSexp: Sized {
 ///
 /// ## Safety
 /// This is intended to be used only in savvy-bindgen
-pub unsafe fn get_external_pointer_addr(x: SEXP) -> *mut std::os::raw::c_void {
-    R_ExternalPtrAddr(x)
+pub unsafe fn get_external_pointer_addr(
+    x: SEXP,
+) -> crate::error::Result<*mut std::os::raw::c_void> {
+    let ptr = R_ExternalPtrAddr(x);
+
+    if ptr.is_null() {
+        return Err(crate::error::Error::InvalidPointer);
+    }
+
+    Ok(ptr)
 }
 
 /// An **external** external pointer.