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

[Raphaël Gomès]

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 at 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),
>>                   }