Close soundness hole with acquire_gil

[davidhewitt]

After thinking on #864 some more, I realised we now have an easy route to make this API sound.

If a GILPool already exists when Python::acquire_gil() is called, we should not create another GILPool.

This ensures that users cannot in safe code accidentally create a GILPool and allow references to outlast it. For those that still really want to clear memory manually, they can still use GILPool::new() and drop that pool when they want.

Fixes #864
Fixes #311

[kngwyu]

I have a mixed feeling about this PR.

I can understand we can avoid what you call 'dangling' by this trick.
However, this delays the release of owned objects and thus users cannot control the timing of the release.

I feel this demerit surpasses the merit it offers.
I don't think this situation occurs in real use cases.

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

[davidhewitt]

However, this delays the release of owned objects and thus users cannot control the timing of the release.

This is not true - users can still control timing of the release, just they must now use the unsafe GILPool::new to control timing of the release.

I think it is very important we make the manual memory control an unsafe API. Because if we continue to tell people to use acquire_gil to
manually manage memory, some users somewhere out there will make this mistake and create segfaults / security holes / UB.

[davidhewitt]

Idea: we could perhaps make a safe API similar to allow_threads to control memory like this:

// The below sample would fail to pass the borrow checker because returning a reference to py

let obj: &PyAny = pyo3::with_memory_pool(|py| {
  // py comes from the new GILPool we create, so no objects can accidentally be made
  // with too long lifetime
  py.eval("object()", None, None).unwrap()
});

// The below sample would compile successfully

let obj: PyObject = pyo3::with_memory_pool(|py| {
  py.eval("object()", None, None).unwrap().to_object(py)
});

Like allow_threads this would use the Sync bound to ensure that the user cannot access the wrong Python GIL token.

Not sure what I think of the name with_memory_pool. Maybe cleanup_pointers ? Or another better name?

[kngwyu]

Idea: we could perhaps make a safe API similar to allow_threads to control memory like this:

Hmm 🤔
I don't want to our API such complex.

As an experiment, I created #894 witch uses Vec<Vec<NonNull<ffi::PyObject>>> for owned storage, but it makes list_get_item 1.5x slower.

So..., since (now) we cannot find a better solution than this PR, now I feel it OK to delay the object release.
I think it cannot be problematic in most cases.

[davidhewitt]

If you like, as part of this PR I can later today add more documentation to address #311

[kngwyu]

I can later today add more documentation to address

How about adding

impl Python {
    unsafe fn acruiqe_gil_with_memorypool(self) -> GILGuard { 
        GILGuard::acquie_with_gil_pool()  // name your own
    }
}

?
Then we can place the document on it.

[kngwyu]

One thing I dislike about this PR is the lifetime 'py can be incorrect if we have nested GILGuards.
So here's another idea: just panics if we detect invalid Python is used. #902

But it results in 1.2x slowdown... 😰

[davidhewitt]

Pushed some more docs, and I want to push some more docs to advanced.md for this PR later today.

[davidhewitt]

I am unsure if #913 will show a better way to solve this problem? Not sure yet, I haven't thought too hard, feel like there might be a way but haven't seen one yet.

[davidhewitt]

Okay, I've had another go at this. I also added some documentation to advanced.md.

Based on the feedback you gave, I moved the function to Python::new_pool(). This is still unsafe because of the reasons we discuss in this PR regarding dangling references, but at least it takes Python so guarantees that the GIL is held.

This function still returns GILPool though. I prefer this to the proposal Python::acquire_gil_with_memorypool because I think if the user needs to acquire GIL they should use the existing acquire_gil.

I'm happy to keep tweaking this PR until we find something which feels good though. So let me know what you think of this version 😄

[kngwyu]

👍

[kngwyu]

I like the new Python::new_pool.
The only concern I have is GILPool::python should not be marked unsafe to encourage users to use it?

[davidhewitt]

The only concern I have is GILPool::python should not be marked unsafe to encourage users to use it?

Yes I wondered about this too. If we add an extra safety note to new_pool saying "the pool must be dropped before the GIL is released", then GILPool::python can be deemed safe.

And I guess this requirement is true anyway, else when the GILPool drops we will be calling Py_DECREF without the GIL.

[davidhewitt]

I've made GILPool::python safe as discussed 👍

[kngwyu]

Thank you!