Fix segfault when reloading interpreter with external modules #1092
Conversation
When embedding the interpreter and loading external modules in that embedded interpreter, the external module correctly shares its internals_ptr with the one in the embedded interpreter. When the interpreter is shut down, however, only the `internals_ptr` local to the embedded code is actually reset to nullptr: the external module remains set. The result is that loading an external pybind11 module, letting the interpreter go through a finalize/initialize, then attempting to use something in the external module fails because this external module is still trying to use the old (destroyed) internals. This causes undefined behaviour (typically a segfault). This commit fixes it by adding a level of indirection in the internals path, converting the local internals variable to `internals **` instead of `internals *`. With this change, we can detect a stale internals pointer and reload the internals pointer (either from a capsule or by creating a new internals instance). (No issue number: this was reported on gitter by @henryiii and @aoloe).
`cmake -E env` was added in 3.2. This drops the built external module in the source directory instead. (This is a bit ugly, but it's the same ugliness we already do for the `pytest` tests).
|
bot the test code and the original code that raised the issue work correctly with this pull request. thanks! |
|
Marking this for v2.2.2; this doesn't change what gets stored in the builtins, and so should be perfectly compatible with v2.2.[01] modules. |
|
Cc @dean0x7d for comments. What is here works, but maybe you have a nicer idea than the double pointer. |
|
I feel like I'm missing important to understand this PR. When the interpreter shuts down, how does the internals pointer get marked as stale? I just see two leaking memory allocations (one for internals, one for the pointer to it) but no changes to shutdown/GC-related code. |
|
The critical change for that is in Basically, there is now only one pointer to the internals data, where previously there were |
|
Aha, understood now. Thank you for the clarification. |
…#1092) * Fix segfault when reloading interpreter with external modules When embedding the interpreter and loading external modules in that embedded interpreter, the external module correctly shares its internals_ptr with the one in the embedded interpreter. When the interpreter is shut down, however, only the `internals_ptr` local to the embedded code is actually reset to nullptr: the external module remains set. The result is that loading an external pybind11 module, letting the interpreter go through a finalize/initialize, then attempting to use something in the external module fails because this external module is still trying to use the old (destroyed) internals. This causes undefined behaviour (typically a segfault). This commit fixes it by adding a level of indirection in the internals path, converting the local internals variable to `internals **` instead of `internals *`. With this change, we can detect a stale internals pointer and reload the internals pointer (either from a capsule or by creating a new internals instance). (No issue number: this was reported on gitter by @henryiii and @aoloe).
* Fix segfault when reloading interpreter with external modules When embedding the interpreter and loading external modules in that embedded interpreter, the external module correctly shares its internals_ptr with the one in the embedded interpreter. When the interpreter is shut down, however, only the `internals_ptr` local to the embedded code is actually reset to nullptr: the external module remains set. The result is that loading an external pybind11 module, letting the interpreter go through a finalize/initialize, then attempting to use something in the external module fails because this external module is still trying to use the old (destroyed) internals. This causes undefined behaviour (typically a segfault). This commit fixes it by adding a level of indirection in the internals path, converting the local internals variable to `internals **` instead of `internals *`. With this change, we can detect a stale internals pointer and reload the internals pointer (either from a capsule or by creating a new internals instance). (No issue number: this was reported on gitter by @henryiii and @aoloe).
When embedding the interpreter and loading external modules in that
embedded interpreter, the external module correctly shares its
internals_ptr with the one in the embedded interpreter. When the
interpreter is shut down, however, only the
internals_ptrlocal tothe embedded code is actually reset to nullptr: the external module
remains set.
The result is that loading an external pybind11 module, letting the
interpreter go through a finalize/initialize, then attempting to use
something in the external module fails because this external module is
still trying to use the old (destroyed) internals. This causes
undefined behaviour (typically a segfault).
This commit fixes it by adding a level of indirection in the internals
path, converting the local internals variable to
internals **insteadof
internals *. With this change, we can detect a stale internalspointer and reload the internals pointer (either from a capsule or by
creating a new internals instance).
(No issue number: this was reported on gitter by @henryiii and @aoloe; thanks to both for the issue and reproducible test case).