Require the GIL to be held in ReleasePool::drain

[andersk]

This adds a Python marker to GILPool, to prevent the caller from misusing it to drain the ReleasePool and release Python objects without the GIL held.

[kngwyu]

Thank you for your PR.
As I commented in #311 I don't think we need this.
But, since I might be wrong about this(yeah everyone makes mistake), could you please explain more why we need this ?

[konstin]

IMHO we don't generally need this as GILPool is a #[doc(hidden)] internal api, but it makes upholding the invariants easier. Specifically, dropping the GILPool calls ffi::Py_DECREF, which requires the GIL to outlive the GILPool.

For my understanding misusing GILPool::new and GILPool::new_no_pointers will result in use-after-free, so IMO we should also mark those functions as unsafe. We might also want to inline and remove impl Default for GILPool, but that's a separate task.

[andersk]

We should never expose functions that can be used to break the invariants of safe Rust without marking them unsafe, even in #[doc(hidden)]. A user who accidentally (or intentionally!) relies on a #[doc(hidden)] API, or an auditor scanning for unsafe blocks, will receive no indication that this code might be dangerous.

If the functions can still be misused even with the GIL held, then yeah, we should mark them unsafe too. Either way, requiring a Python does make them less dangerous.

(The impl Default was removed because there’s no way for that to require a Python. As far as I can tell, it isn’t used anywhere, and was only added to satisfy a clippy rule that now no longer applies because new now takes an argument.)

[konstin]

(The impl Default was removed because there’s no way for that to require a Python. As far as I can tell, it isn’t used anywhere, and was only added to satisfy a clippy rule that now no longer applies because new now takes an argument.)

Oh sorry I had totally missed that you had already done that.

[kngwyu]

IMHO we don't generally need this as GILPool is a #[doc(hidden)] internal api, but it makes upholding the invariants easier.

OK, I understand.

[konstin]

Thank you!