Introduce CsMatBase::proper_indptr for ffi

This API is safer than the previously introduced
IndPtrBase::to_proper_ffi, which could very easily lead to use after
free in unsafe code, as pointed out by @mulimoen. This new API still can
be misused, but that's a common pitfall for methods returning Cow. The
documentation thus warns against this pitfall, and illustrates the
proper usage.

Author: vbarrielle

sprs-benches/src/main.rs

@@ -52,12 +52,10 @@ fn eigen_prod(a: sprs::CsMatView<f64>, b: sprs::CsMatView<f64>) -> usize {
     assert!(b.is_csr());
     assert!(b.rows() <= isize::MAX as usize);
     assert!(b.indptr().nnz() <= isize::MAX as usize);
-    let a_indptr = a.indptr();
-    let (_a_indptr_proper, a_indptr_ptr) = a_indptr.to_proper_ffi();
+    let a_indptr_proper = a.proper_indptr();
     let a_indices = a.indices().as_ptr() as *const isize;
     let a_data = a.data().as_ptr();
-    let b_indptr = b.indptr();
-    let (_b_indptr_proper, b_indptr_ptr) = b_indptr.to_proper_ffi();
+    let b_indptr_proper = b.proper_indptr();
     let b_indices = b.indices().as_ptr() as *const isize;
     let b_data = b.data().as_ptr();
     // Safety: sprs guarantees the validity of these pointers, and our wrapping
@@ -73,10 +71,10 @@ fn eigen_prod(a: sprs::CsMatView<f64>, b: sprs::CsMatView<f64>) -> usize {
             a_rows,
             a_cols,
             b_cols,
-            a_indptr_ptr as *const isize,
+            a_indptr_proper.as_ptr() as *const isize,
             a_indices,
             a_data,
-            b_indptr_ptr as *const isize,
+            b_indptr_proper.as_ptr() as *const isize,
             b_indices,
             b_data,
         )

src/sparse/csmat.rs

@@ -810,6 +810,33 @@ where
         crate::IndPtrView::new_trusted(self.indptr.raw_storage())
     }
 
+    /// Get an indptr representation suitable for ffi, cloning if necessary to
+    /// get a compatible representation.
+    ///
+    /// # Warning
+    ///
+    /// For ffi usage, one needs to call `Cow::as_ptr`, but it's important
+    /// to keep the `Cow` alive during the lifetime of the pointer. Example
+    /// of a correct and incorrect ffi usage:
+    ///
+    /// ```rust
+    /// let mat: sprs::CsMat<f64> = sprs::CsMat::eye(5);
+    /// let mid = mat.view().middle_outer_views(1, 2);
+    /// let ptr = {
+    ///     let indptr_proper = mid.proper_indptr();
+    ///     println!(
+    ///         "ptr {:?} is valid as long as _indptr_proper_owned is in scope",
+    ///         indptr_proper.as_ptr()
+    ///     );
+    ///     indptr_proper.as_ptr()
+    /// };
+    /// // This line is UB.
+    /// // println!("ptr deref: {}", *ptr);
+    /// ```
+    pub fn proper_indptr(&self) -> std::borrow::Cow<[Iptr]> {
+        self.indptr.to_proper()
+    }
+
     /// The inner dimension location for each non-zero value. See
     /// the documentation of indptr() for more explanations.
     pub fn indices(&self) -> &[I] {

src/sparse/indptr.rs

@@ -149,6 +149,46 @@ where
 
     /// Returns a proper indptr representation, cloning if we do not have
     /// a proper indptr.
+    ///
+    /// # Warning
+    ///
+    /// For ffi usage, one needs to call `Cow::as_ptr`, but it's important
+    /// to keep the `Cow` alive during the lifetime of the pointer. Example
+    /// of a correct and incorrect ffi usage:
+    ///
+    /// ```rust
+    /// let mat: sprs::CsMat<f64> = sprs::CsMat::eye(5);
+    /// let mid = mat.view().middle_outer_views(1, 2);
+    /// let ptr = {
+    ///     let indptr = mid.indptr(); // needed for lifetime
+    ///     let indptr_proper = indptr.to_proper();
+    ///     println!(
+    ///         "ptr {:?} is valid as long as _indptr_proper_owned is in scope",
+    ///         indptr_proper.as_ptr()
+    ///     );
+    ///     indptr_proper.as_ptr()
+    /// };
+    /// // This line is UB.
+    /// // println!("ptr deref: {}", *ptr);
+    /// ```
+    ///
+    /// It is much easier to directly use the `proper_indptr` method of
+    /// `CsMatBase` directly:
+    ///
+    /// ```rust
+    /// let mat: sprs::CsMat<f64> = sprs::CsMat::eye(5);
+    /// let mid = mat.view().middle_outer_views(1, 2);
+    /// let ptr = {
+    ///     let indptr_proper = mid.proper_indptr();
+    ///     println!(
+    ///         "ptr {:?} is valid as long as _indptr_proper_owned is in scope",
+    ///         indptr_proper.as_ptr()
+    ///     );
+    ///     indptr_proper.as_ptr()
+    /// };
+    /// // This line is UB.
+    /// // println!("ptr deref: {}", *ptr);
+    /// ```
     pub fn to_proper(&self) -> std::borrow::Cow<[Iptr]> {
         if self.is_proper() {
             std::borrow::Cow::Borrowed(&self.storage[..])
@@ -159,31 +199,6 @@ where
         }
     }
 
-    /// Returns a proper indptr representation, cloning if we do not have
-    /// a proper indptr, and a pointer to this proper representation for use
-    /// in ffi. The returned pointer has thus the same lifetime as the
-    /// proper representation.
-    ///
-    /// # Example
-    ///
-    /// ```rust
-    /// let mat: sprs::CsMat<f64> = sprs::CsMat::eye(5);
-    /// let mid = mat.view().middle_outer_views(1, 2);
-    /// let (_indptr_proper_owned, ptr) = mid.indptr().to_proper_ffi();
-    /// println!(
-    ///     "ptr {:?} is valid as long as _indptr_proper_owned is in scope",
-    ///     ptr
-    /// );
-    /// unsafe {
-    ///     assert_eq!(*ptr, 0)
-    /// };
-    /// ```
-    pub fn to_proper_ffi(&self) -> (std::borrow::Cow<[Iptr]>, *const Iptr) {
-        let proper = self.to_proper();
-        let ptr = proper.as_ptr();
-        (proper, ptr)
-    }
-
     fn offset(&self) -> Iptr {
         let zero = Iptr::zero();
         self.storage.get(0).cloned().unwrap_or(zero)

suitesparse_bindings/sprs_suitesparse_camd/src/lib.rs

@@ -38,10 +38,10 @@ where
             mat.to_other_types();
         let mut perm: Vec<SuiteSparseInt> = vec![0; n];
         let camd_res = unsafe {
-            let (_indptr_proper, indptr_ptr) = mat.indptr().to_proper_ffi();
+            let indptr_proper = mat.proper_indptr();
             camd_order(
                 n as SuiteSparseInt,
-                indptr_ptr,
+                indptr_proper.as_ptr(),
                 mat.indices().as_ptr(),
                 perm.as_mut_ptr(),
                 control.as_mut_ptr(),
@@ -57,10 +57,10 @@ where
             mat.to_other_types();
         let mut perm: Vec<SuiteSparseLong> = vec![0; n];
         let camd_res = unsafe {
-            let (_indptr_proper, indptr_ptr) = mat.indptr().to_proper_ffi();
+            let indptr_proper = mat.proper_indptr();
             camd_l_order(
                 n as SuiteSparseLong,
-                indptr_ptr,
+                indptr_proper.as_ptr(),
                 mat.indices().as_ptr(),
                 perm.as_mut_ptr(),
                 control.as_mut_ptr(),