fix: Replace bare static exception<T> with gil_safe_call_once_and_store.

[rwgk]

Description

This is to ensure that Py_DECREF() is not called after the Python interpreter was finalized already:

pybind11/include/pybind11/gil_safe_call_once.h

Line 19 in 3414c56

// 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and

(Bug noticed by chance while working on #4888.)

Suggested changelog entry:

Removes potential for Undefined Behavior during process teardown.

[rwgk]

Google global testing ID (passed): OCL:575710443:BASE:575809221:1698070476323:98dd25a0 (using this PR @ ca7bdac)

[rwgk]

@tkoeppe FYI

I never got to systematically looking if there are more bugs like this BTW.

[EthanSteinberg]

Looks good. Thanks for the fix!

I never got to systematically looking if there are more bugs like this BTW.

I bet we could set up some global variables to track this / throw assertions. Not sure if it would be worth the effort though.

[tkoeppe]

Does this default constructor call into CPython (and release/reacquire the GIL)?

The deadlock scenario can only arise if there's some non-trivial lock interaction in the initializer of the static variable.

I suppose this change would also deal with some end-of-program lifetime situations, though, e.g. if this object gets destroyed too early or too late.

[rwgk]

Does this default constructor call into CPython (and release/reacquire the GIL)?

No, it just copies nullptr to m_ptr (

pybind11/include/pybind11/pytypes.h

Line 297 in 3414c56

PyObject *m_ptr = nullptr;

).

The deadlock scenario can only arise if there's some non-trivial lock interaction in the initializer of the static variable.

Yes, that was on my mind.

I suppose this change would also deal with some end-of-program lifetime situations, though, e.g. if this object gets destroyed too early or too late.

That was the only reason I got into here.

I considered simply changing this to use new (with a boilerplate "intentional leak" comment), but then decided: the universally safe solution

1. is easy,
2. inexpensive,
3. expressive (call once is exactly what we want),
4. reads nicely,
5. future proof (in case the exception<T> implementation is changed),
6. and a great pattern for people to remember and follow,

let's use it!

[tkoeppe]

Yes, makes sense! Using the new facility just for the lifetime is certainly also a good move.

[rwgk]

Thanks @EthanSteinberg!

I bet we could set up some global variables to track this / throw assertions.

I don't know how we could do that (going completely blank tbh). Just disclosing & an indication of curiosity, but ...

Not sure if it would be worth the effort though.

... yeah, I'm assuming there is only very little left to be had, simply based on how widely used pybind11 is.

I'm slightly surprised that nobody needed this fix before. I'm guessing this bug bites very rarely, and only during process teardown, so it's just some occasional flakiness maybe that's easy to ignore.

[EthanSteinberg]

I don't know how we could do that (going completely blank tbh). Just disclosing & an indication of curiosity, but ...

I don't know exactly how to set this up (would need to do more research), but my initial guess would be to have a boolean flag that is flipped true by https://docs.python.org/3/library/atexit.html#atexit.register.

We could then flag on any calls to Py_DECREF or something of that nature ...

The timing here is really tricky. I'm not sure atexit would be at the point we need.

[rwgk]

The timing here is really tricky. I'm not sure atexit would be at the point we need.

I'm thinking, let's not go there (too uncertain, not a lot to gain for it).

But something else crossed my mind (~randomly): How does this static play with embed.h, specifically multiple calls to py::initialize_interpreter() - py::finalize_interpreter() or py::scoped_interpreter guard{}?

Is the stored Exception type still valid after py::finalize_interpreter()?

[tkoeppe]

I don' t know how the CPython API works for multiple interpreters, but just thinking about this on a high level, none of our static approaches can work with multiple interpreters. Instead, the "once" guard would have to be part of the interpreter state, and the once-initializers would need to work on that per-interpreter state, not globally.

[rwgk]

Thanks Thomas for confirming what I was suspecting! — Sounds like we (someone...) should make this warning much stronger:

pybind11/include/pybind11/embed.h

Lines 235 to 242 in fa27d2f

	.. warning::
	The interpreter can be restarted by calling `initialize_interpreter` again.
	Modules created using pybind11 can be safely re-initialized. However, Python
	itself cannot completely unload binary extension modules and there are several
	caveats with regard to interpreter restarting. All the details can be found
	in the CPython documentation. In short, not all interpreter memory may be
	freed, either due to reference cycles or user-created global data.