changeset 42851:64e28b891796

rust-cpython: mark unsafe functions as such It wasn't trivial to fix leak_immutable() to be safe since we have to allow immutable operations (e.g. iter()) on the leaked reference. So let's mark it unsafe for now. Callers must take care of the returned object to guarantee the memory safety. I'll revisit this later. I think $leaked<T: 'static> could have a function that converts itself into $leaked<U: 'static> with a given FnOnce(&T) -> &U, where T is $inner_struct, and U is $iterator_type for example.
author Yuya Nishihara <yuya@tcha.org>
date Sun, 01 Sep 2019 18:06:31 +0900
parents 8f549c46bc64
children 58f73e9ccfff
files rust/hg-cpython/src/dirstate/dirs_multiset.rs rust/hg-cpython/src/dirstate/dirstate_map.rs rust/hg-cpython/src/ref_sharing.rs
diffstat 3 files changed, 32 insertions(+), 16 deletions(-) [+]
line wrap: on
line diff
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Sun Sep 01 17:48:24 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Sun Sep 01 18:06:31 2019 +0900
@@ -86,7 +86,7 @@
             })
     }
     def __iter__(&self) -> PyResult<DirsMultisetKeysIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         DirsMultisetKeysIterator::create_instance(
             py,
             RefCell::new(Some(leak_handle)),
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sun Sep 01 17:48:24 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sun Sep 01 18:06:31 2019 +0900
@@ -319,7 +319,7 @@
     }
 
     def keys(&self) -> PyResult<DirstateMapKeysIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         DirstateMapKeysIterator::from_inner(
             py,
             Some(leak_handle),
@@ -328,7 +328,7 @@
     }
 
     def items(&self) -> PyResult<DirstateMapItemsIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         DirstateMapItemsIterator::from_inner(
             py,
             Some(leak_handle),
@@ -337,7 +337,7 @@
     }
 
     def __iter__(&self) -> PyResult<DirstateMapKeysIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         DirstateMapKeysIterator::from_inner(
             py,
             Some(leak_handle),
@@ -434,7 +434,7 @@
     }
 
     def copymapiter(&self) -> PyResult<CopyMapKeysIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         CopyMapKeysIterator::from_inner(
             py,
             Some(leak_handle),
@@ -443,7 +443,7 @@
     }
 
     def copymapitemsiter(&self) -> PyResult<CopyMapItemsIterator> {
-        let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+        let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
         CopyMapItemsIterator::from_inner(
             py,
             Some(leak_handle),
--- a/rust/hg-cpython/src/ref_sharing.rs	Sun Sep 01 17:48:24 2019 +0900
+++ b/rust/hg-cpython/src/ref_sharing.rs	Sun Sep 01 18:06:31 2019 +0900
@@ -58,7 +58,12 @@
     /// Return a reference to the wrapped data with an artificial static
     /// lifetime.
     /// We need to be protected by the GIL for thread-safety.
-    pub fn leak_immutable<T>(
+    ///
+    /// # Safety
+    ///
+    /// This is highly unsafe since the lifetime of the given data can be
+    /// extended. Do not call this function directly.
+    pub unsafe fn leak_immutable<T>(
         &self,
         py: Python,
         data: &PySharedRefCell<T>,
@@ -72,10 +77,14 @@
         }
         let ptr = data.as_ptr();
         self.leak_count.replace(self.leak_count.get() + 1);
-        unsafe { Ok(&*ptr) }
+        Ok(&*ptr)
     }
 
-    pub fn decrease_leak_count(&self, _py: Python, mutable: bool) {
+    /// # Safety
+    ///
+    /// It's unsafe to update the reference count without knowing the
+    /// reference is deleted. Do not call this function directly.
+    pub unsafe fn decrease_leak_count(&self, _py: Python, mutable: bool) {
         self.leak_count
             .replace(self.leak_count.get().saturating_sub(1));
         if mutable {
@@ -123,6 +132,8 @@
 }
 
 impl<'a, T> PyRefMut<'a, T> {
+    // Must be constructed by PySharedState after checking its leak_count.
+    // Otherwise, drop() would incorrectly update the state.
     fn new(
         _py: Python<'a>,
         inner: RefMut<'a, T>,
@@ -152,7 +163,9 @@
     fn drop(&mut self) {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        self.py_shared_state.decrease_leak_count(py, true);
+        unsafe {
+            self.py_shared_state.decrease_leak_count(py, true);
+        }
     }
 }
 
@@ -223,7 +236,7 @@
             /// It's up to you to make sure that the management object lives
             /// longer than the leaked reference. Otherwise, you'll get a
             /// dangling reference.
-            fn leak_immutable<'a>(
+            unsafe fn leak_immutable<'a>(
                 &'a self,
                 py: Python<'a>,
             ) -> PyResult<($leaked, &'static $inner_struct)> {
@@ -247,7 +260,9 @@
         }
 
         impl $leaked {
-            fn new(py: Python, inner: &$name) -> Self {
+            // Marked as unsafe so client code wouldn't construct $leaked
+            // struct by mistake. Its drop() is unsafe.
+            unsafe fn new(py: Python, inner: &$name) -> Self {
                 Self {
                     inner: inner.clone_ref(py),
                 }
@@ -258,9 +273,10 @@
             fn drop(&mut self) {
                 let gil = Python::acquire_gil();
                 let py = gil.python();
-                self.inner
-                    .py_shared_state(py)
-                    .decrease_leak_count(py, false);
+                let state = self.inner.py_shared_state(py);
+                unsafe {
+                    state.decrease_leak_count(py, false);
+                }
             }
         }
     };
@@ -346,7 +362,7 @@
 ///     data py_shared_state: PySharedState;
 ///
 ///     def __iter__(&self) -> PyResult<MyTypeItemsIterator> {
-///         let (leak_handle, leaked_ref) = self.leak_immutable(py)?;
+///         let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };
 ///         MyTypeItemsIterator::create_instance(
 ///             py,
 ///             RefCell::new(Some(leak_handle)),