Easy dangling PyObject with GILPool

[davidhewitt]

This sample code produces a dangling PyObject after the GILPool is dropped:

use pyo3::prelude::*;
use pyo3::{ffi, GILPool, AsPyPointer};
use pyo3::types::PyList;

fn main() {
    let gil = Python::acquire_gil();
    let py = gil.python();
    let obj;

    {
        let pool = GILPool::new(py);
        obj = py.eval("object()", None, None).unwrap();
        println!("Count before pool drop {}", unsafe { ffi::Py_REFCNT(obj.as_ptr()) });
    }

    println!("Count after pool drop {}", unsafe { ffi::Py_REFCNT(obj.as_ptr()) });
}

As seen by the output:

Count before pool drop 1
Count after pool drop 1919707401104

I think what must be going on is that obj has a lifetime tied to py, but it is actually only valid for the lifetime of pool. Because of the wrong lifetime semantics the Rust compiler lets us create the dangling pointer.

I'm not sure yet exactly what the solution should be. Maybe the change to GILPool / py relationship proposed in #800 will mitigate this issue.

I wonder whether this issue combined with #756 is telling us that the right design is really PyAny<'a> as proposed in #679

[davidhewitt]

FWIW after #869 then we will need one line of unsafe here (for GILPool::new), so the original title will no longer apply. But I still think the point stands whether PyAny<'a> is still the right design - because then the refcount drop can be done when the PyAny<'a> drops, not when the GILPool drops.

[kngwyu]

But I still think the point stands whether PyAny<'a> is still the right design

I'm sorry but I can't understand why this problem is related to PyAny<'a>...
It's just my own view, but PyAny<'a>'s Drop implementation would be

struct PyAny<'a>(...);
impl PyAny {
    fn from_borrowed(ptr: ...) -> Self {
        Py_INCREF(ptr);
        Self(ptr)
    }
    fn from_owned(ptr: ...) -> Self {
        Self(ptr)
    }
}

impl<'a> Drop for PyAny<'a> {
    fn drop(&mut self) {
        Py_DECREF(self.0)
    }
}

So its reference count is not affected by GILPool or GILGuard and thus cannot be negative.

[davidhewitt]

@kngwyu I agree with what what you say and how you propose PyAny<'a> would work. This is what I was trying to say 😃 . If we had PyAny<'a> then it would not have to worry about GILPool.

[kngwyu]

Ah, sorry, I just misread.

[davidhewitt]

FWIW; the change to GILPool to make GILPool::new unsafe has helped make the original example sound, but there is still the following example code using acquire_gil instead:

fn main() {
  use pyo3::prelude::*;
  use pyo3::{ffi, GILPool, AsPyPointer};
  use pyo3::types::PyList;

  let gil = Python::acquire_gil();
  let py = gil.python();
  let obj;

  {
    let gil2 = Python::acquire_gil();
    obj = py.eval("object()", None, None).unwrap();
    println!("Count before gil2 drop {}", unsafe { ffi::Py_REFCNT(obj.as_ptr()) });
  }

  println!("Count after gil2 drop {}", unsafe { ffi::Py_REFCNT(obj.as_ptr()) });
}

// Output:
// Count before gil2 drop 1
// Count after gil2 drop 1486953073552