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

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 22, 2019, 8:11 a.m.
Message ID <779af7c0979780d03451.1571731918@mimosa>
Download mbox | patch
Permalink /patch/42527/
State New
Headers show

Comments

Yuya Nishihara - Oct. 22, 2019, 8:11 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1571727874 -32400
#      Tue Oct 22 16:04:34 2019 +0900
# Node ID 779af7c0979780d03451dd7cfe303e8dc64c6576
# Parent  29399437267567957a97dc9d924704ca748e3cec
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.
Yuya Nishihara - Oct. 22, 2019, 8:17 a.m.
On Tue, 22 Oct 2019 17:11:58 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1571727874 -32400
> #      Tue Oct 22 16:04:34 2019 +0900
> # Node ID 779af7c0979780d03451dd7cfe303e8dc64c6576
> # Parent  29399437267567957a97dc9d924704ca748e3cec
> rust-cpython: mark all PyLeaked methods as unsafe

CC: Raphaël, Georges

This might be interesting. The other patches in this series are refactoring
stuff.

> 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.
> 
> 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),
>                  }
Raphaël Gomès - Oct. 22, 2019, 12:43 p.m.
On 10/22/19 10:11 AM, Yuya Nishihara wrote:
> 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.
Yes, this is unfortunate indeed. I'm all for starting the upstreaming 
process soon as I eluded to in your last series, because we might find a 
nice solution while discussing it and prevent too much API drift.
> 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

Nit: I feel like "cheated" is not the right term. I think "artificial" 
might be better suited? I don't care enough if it gets in however
Raphaël Gomès - Oct. 22, 2019, 12:45 p.m.
Nice series as always, everything makes sense to me. I will try to 
dogfood the new API a little bit more in the near future.

On 10/22/19 10:17 AM, Yuya Nishihara wrote:
> On Tue, 22 Oct 2019 17:11:58 +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1571727874 -32400
>> #      Tue Oct 22 16:04:34 2019 +0900
>> # Node ID 779af7c0979780d03451dd7cfe303e8dc64c6576
>> # Parent  29399437267567957a97dc9d924704ca748e3cec
>> rust-cpython: mark all PyLeaked methods as unsafe
> CC: Raphaël, Georges
>
> This might be interesting. The other patches in this series are refactoring
> stuff.
>
>> 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.
>>
>> 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),
>>                   }
Yuya Nishihara - Oct. 22, 2019, 12:53 p.m.
On Tue, 22 Oct 2019 14:43:53 +0200, Raphaël Gomès wrote:
> On 10/22/19 10:11 AM, Yuya Nishihara wrote:
> > 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
> 
> Nit: I feel like "cheated" is not the right term. I think "artificial"
> might be better suited? I don't care enough if it gets in however

Perhaps. I'll revise the docstring when sending pullreq. Thanks.
Raphaël Gomès - Oct. 23, 2019, 7:49 a.m.
FYI, Georges is currently on vacation, don't expect an answer for at 
least a week.

On 10/22/19 2:53 PM, Yuya Nishihara wrote:
> On Tue, 22 Oct 2019 14:43:53 +0200, Raphaël Gomès wrote:
>> On 10/22/19 10:11 AM, Yuya Nishihara wrote:
>>> 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
>> Nit: I feel like "cheated" is not the right term. I think "artificial"
>> might be better suited? I don't care enough if it gets in however
> Perhaps. I'll revise the docstring when sending pullreq. Thanks.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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());