Patchwork [7,of,7,RESEND] rust-cpython: mark all PyLeaked methods as unsafe

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 28, 2020, 11:50 p.m.
Message ID <a16f1f5096da829776a9.1580255433@mimosa>
Download mbox | patch
Permalink /patch/44729/
State Accepted
Headers show

Comments

Yuya Nishihara - Jan. 28, 2020, 11:50 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1571727874 -32400
#      Tue Oct 22 16:04:34 2019 +0900
# Node ID a16f1f5096da829776a94fea4e326fb1a15a4067
# Parent  e23256b750fd05002ad527e0705fba9c9030d272
rust-cpython: mark all PyLeaked methods as unsafe

Unfortunately, these methods can be abused to obtain the inner 'static
reference. The simplest (pseudo-code) example is:

  let leaked: PyLeaked<&'static _> = shared.leak_immutable();
  let static_ref: &'static _ = &*leaked.try_borrow(py)?;
      // PyLeakedRef::deref() tries to bound the lifetime to itself, but
      // the underlying data is a &'static reference, so the returned
      // reference can be &'static.

This problem can be easily fixed by coercing the lifetime, but there are
many other ways to achieve that, and there wouldn't be a generic solution:

  let leaked: PyLeaked<&'static [_]> = shared.leak_immutable();
  let leaked_iter: PyLeaked<slice::Iter<'static, _>>
      = unsafe { leaked.map(|v| v.iter()) };
  let static_slice: &'static [_] = leaked_iter.try_borrow(py)?.as_slice();

So basically I failed to design the safe borrowing interface. Maybe we'll
instead have to add much more restricted interface on top of the unsafe
PyLeaked methods? For instance, Iterator::next() could be implemented if
its Item type is not &'a (where 'a may be cheated.)

Anyway, this seems not an easy issue, so it's probably better to leave the
current interface as unsafe, and get broader comments while upstreaming this
feature.

Patch

diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -294,7 +294,13 @@  impl<T> PyLeaked<T> {
     /// Immutably borrows the wrapped value.
     ///
     /// Borrowing fails if the underlying reference has been invalidated.
-    pub fn try_borrow<'a>(
+    ///
+    /// # Safety
+    ///
+    /// The lifetime of the innermost object is cheated. Do not obtain and
+    /// copy it out of the borrow scope. See the example of `try_borrow_mut()`
+    /// for details.
+    pub unsafe fn try_borrow<'a>(
         &'a self,
         py: Python<'a>,
     ) -> PyResult<PyLeakedRef<'a, T>> {
@@ -311,7 +317,24 @@  impl<T> PyLeaked<T> {
     ///
     /// Typically `T` is an iterator. If `T` is an immutable reference,
     /// `get_mut()` is useless since the inner value can't be mutated.
-    pub fn try_borrow_mut<'a>(
+    ///
+    /// # Safety
+    ///
+    /// The lifetime of the innermost object is cheated. Do not obtain and
+    /// copy it out of the borrow scope. For example, the following code
+    /// is unsafe:
+    ///
+    /// ```compile_fail
+    /// let slice;
+    /// {
+    ///     let iter = leaked.try_borrow_mut(py);
+    ///     // slice can outlive since the iterator is of Iter<'static, T>
+    ///     // type, but it shouldn't.
+    ///     slice = iter.as_slice();
+    /// }
+    /// println!("{:?}", slice);
+    /// ```
+    pub unsafe fn try_borrow_mut<'a>(
         &'a mut self,
         py: Python<'a>,
     ) -> PyResult<PyLeakedRefMut<'a, T>> {
@@ -423,6 +446,11 @@  impl<T> DerefMut for PyLeakedRefMut<'_, 
 /// tuple on iteration success, turning it into something Python understands.
 /// * `$success_func` is the return type of `$success_func`
 ///
+/// # Safety
+///
+/// `$success_func` may take a reference, but it's lifetime may be cheated.
+/// Do not copy it out of the function call.
+///
 /// # Example
 ///
 /// ```
@@ -476,9 +504,10 @@  macro_rules! py_shared_iterator {
 
             def __next__(&self) -> PyResult<$success_type> {
                 let mut leaked = self.inner(py).borrow_mut();
-                let mut iter = leaked.try_borrow_mut(py)?;
+                let mut iter = unsafe { leaked.try_borrow_mut(py)? };
                 match iter.next() {
                     None => Ok(None),
+                    // res may be a reference of cheated 'static lifetime
                     Some(res) => $success_func(py, res),
                 }
             }
@@ -527,7 +556,7 @@  mod test {
         let (gil, owner) = prepare_env();
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
-        let leaked_ref = leaked.try_borrow(py).unwrap();
+        let leaked_ref = unsafe { leaked.try_borrow(py) }.unwrap();
         assert_eq!(*leaked_ref, "new");
     }
 
@@ -537,7 +566,8 @@  mod test {
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
         let mut leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
-        let mut leaked_ref = leaked_iter.try_borrow_mut(py).unwrap();
+        let mut leaked_ref =
+            unsafe { leaked_iter.try_borrow_mut(py) }.unwrap();
         assert_eq!(leaked_ref.next(), Some('n'));
         assert_eq!(leaked_ref.next(), Some('e'));
         assert_eq!(leaked_ref.next(), Some('w'));
@@ -550,7 +580,7 @@  mod test {
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
         owner.string_shared(py).borrow_mut().clear();
-        assert!(leaked.try_borrow(py).is_err());
+        assert!(unsafe { leaked.try_borrow(py) }.is_err());
     }
 
     #[test]
@@ -560,7 +590,7 @@  mod test {
         let leaked = owner.string_shared(py).leak_immutable();
         let mut leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
         owner.string_shared(py).borrow_mut().clear();
-        assert!(leaked_iter.try_borrow_mut(py).is_err());
+        assert!(unsafe { leaked_iter.try_borrow_mut(py) }.is_err());
     }
 
     #[test]
@@ -580,10 +610,10 @@  mod test {
         assert!(owner.string_shared(py).try_borrow_mut().is_ok());
         let leaked = owner.string_shared(py).leak_immutable();
         {
-            let _leaked_ref = leaked.try_borrow(py).unwrap();
+            let _leaked_ref = unsafe { leaked.try_borrow(py) }.unwrap();
             assert!(owner.string_shared(py).try_borrow_mut().is_err());
             {
-                let _leaked_ref2 = leaked.try_borrow(py).unwrap();
+                let _leaked_ref2 = unsafe { leaked.try_borrow(py) }.unwrap();
                 assert!(owner.string_shared(py).try_borrow_mut().is_err());
             }
             assert!(owner.string_shared(py).try_borrow_mut().is_err());
@@ -599,7 +629,8 @@  mod test {
         let leaked = owner.string_shared(py).leak_immutable();
         let mut leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
         {
-            let _leaked_ref = leaked_iter.try_borrow_mut(py).unwrap();
+            let _leaked_ref =
+                unsafe { leaked_iter.try_borrow_mut(py) }.unwrap();
             assert!(owner.string_shared(py).try_borrow_mut().is_err());
         }
         assert!(owner.string_shared(py).try_borrow_mut().is_ok());