Repository navigation
Check for sub interpreter support is not catching readline global state and crashing #112292
Description
Activity
After some investigation, this is what I've established:
- Readline init runs successfully the first time inside a sub interpreter
- Readline has a concurrent initialization callback hook called
on_startupwhich uses a macro to read the module state. - The
readlinestate_globalmacro doesn't check if the result ofPyState_FindModuleis NULL and segfaults PyState_FindModuleis returning NULL (but not setting an exception) because the module state list in the interpreter state is smaller than the index (for example,m_indexis 54 and the list is only 4 long).m_indexis set based on a runtime state global, but the module cache index is an interpreter state list.- Readline's initialisation code doesn't check return values in lots of places, so even if it did return an error, without further changes it will crash a second time
Reacted by Eric Snow and Erlend E. AaslandFurthermore:
- The first time,
readlinesuccessfully imports, but within_PyImport_LoadDynamicModuleWithSpecit calls_PyImport_CheckSubinterpIncompatibleExtensionAllowedwhich returns and error, goes to theerrorcondition and skips_PyImport_FixupExtensionObject, so even thoughm_indexis set, the module is never added to the module cache index. - Then because readline was registered with a callback (
on_startup) function which assumes that the module was imported successfully, it crashes.
That's it.
To fix this, the readline hooks need to be more robust to at least not segfault and raise an import error or something else
Reacted by Eric Snow and Erlend E. Aasland- The first time,
@ericsnowcurrently I worked it out in the end 😖
Reacted by Eric Snow and Erlend E. AaslandAn alternative and more universal fix would be to somehow have a check like
_PyImport_CheckSubinterpIncompatibleExtensionAllowedearlier in_PyImport_LoadDynamicModuleWithSpecso that the init function is never called in the first place.An alternative and more universal fix would be to somehow have a check like
_PyImport_CheckSubinterpIncompatibleExtensionAllowedearlier in_PyImport_LoadDynamicModuleWithSpecso that the init function is never called in the first place.I agree with your line of thinking, but there isn't much we can actually do about it. The reason the check happens where it does is that we don't know if the module implements multi-phase init until after we call the init function. The function returns either a module object or a module def object. The former indicates a single-phase init module while that latter a multi-phase init module. We only do the check for single-phase init. Thus we can't check before calling the init function.
That said, fixing the error returns situation in Modules/readline.c is definitely worth doing.
Reacted by Erlend E. AaslandFWIW, I'm at least a little concerned about the potential failure mode here, where a single-phase init module is loaded and then import fails in a subinterpreter, but none of the module's side effects are reverted, leading to a crash. There isn't much we can do about that given how we must always call the init func.
My main concern is that some third-party extensions might have the same potential failure mode, so importing them in a subinterpreter would fail (as expected) but then crash. The compatibility check is meant to keep things safe but falls short in this case. 😞 I suppose the specific underlying cause of the crash here1 might be unusual and the risk of crash generally less likely, but even if a small number of such extensions relies on a successful import (or that the import is happening in the main interpreter), the subsequent crashes would be a real problem. It would be nice if we could make things at least a little safer. Again, I'm not sure there's much we can do, though. Our only option might be education (i.e. documentation, "What's New" entries, etc.).
@encukou, any ideas?
Footnotes
-
In this case the side effect is a registered library callback with the faulty expectation that
PyState_FindModule()never returnsNULL. ↩
-
As to the readline module, I think it would be better if we could remove the library callbacks that the init function registers, or, better, avoid registering them in the first place. Either way, that would likely mean implementing multi-phase init.
Reacted by Petr Viktorin and Erlend E. AaslandRelated comment in #103092 #103092 (comment)
FWIW, I'm at least a little concerned about the potential failure mode here, where a single-phase init module is loaded and then import fails in a subinterpreter, but none of the module's side effects are reverted, leading to a crash. There isn't much we can do about that given how we must always call the init func.
My main concern is that some third-party extensions might have the same potential failure mode, so importing them in a subinterpreter would fail (as expected) but then crash. The compatibility check is meant to keep things safe but falls short in this case. 😞 I suppose the specific underlying cause of the crash here1 might be unusual and the risk of crash generally less likely, but even if a small number of such extensions relies on a successful import (or that the import is happening in the main interpreter), the subsequent crashes would be a real problem. It would be nice if we could make things at least a little safer. Again, I'm not sure there's much we can do, though. Our only option might be education (i.e. documentation, "What's New" entries, etc.).
@encukou, any ideas?
Footnotes
- In this case the side effect is a registered library callback with the faulty expectation that
PyState_FindModule()never returnsNULL. ↩
The good news is I've managed to run most of the CPython test suite in a sub interpreter and this is the only module I've found that crashes like this. There are lots of failures, mostly because
_testcapiisn't supported, but I'll look into them in another issueReacted by Eric Snow and Erlend E. Aasland- In this case the side effect is a registered library callback with the faulty expectation that
FWIW, I'm at least a little concerned about the potential failure mode here, where a single-phase init module is loaded and then import fails in a subinterpreter, but none of the module's side effects are reverted, leading to a crash. There isn't much we can do about that given how we must always call the init func.
@encukou, any ideas?
Keep a memo of all single-phase
PyInit_*functions that were called (successfully or not), and refuse to call one a second time?Reacted by Eric SnowKeep a memo of all single-phase
PyInit_*functions that were called (successfully or not), and refuse to call one a second time?That seems like it would do the job. Good idea.
I think you might get the same result if you call
_PyImport_ClearExtension/clear_singlephase_extensionif_PyImport_CheckSubinterpIncompatibleExtensionAllowed()returns false along with raising an exception?I think you might get the same result if you call
_PyImport_ClearExtension/clear_singlephase_extensionif_PyImport_CheckSubinterpIncompatibleExtensionAllowed()returns false along with raising an exception?I tried that, it didn't seem to fix this specific issue
I am interested in reproducing those segfaults. I went back to the commit before #112313 was merged (2e632fa) and ran
test_in_interp.pyas described above. But no crashes.Unfortunately the fork behind the PR has ben deleted so I don't know where to go from here. Anyone have a commit hash that shows the segfaults?
Thank you!
This should be a non-issue since gh-121060.
Reacted by Stefan H. Holek
Bug report
Bug description:
Sub interpreters have a check (
_PyImport_CheckSubinterpIncompatibleExtensionAllowed) for extensions which aren't compatible.One example of an incompatible extension is
readline. It has a global state and shouldn't be imported from a sub interpreter. The issue is that it can be imported dynamically and the crash happens in the module init mechanism, but the check is after (https://github.com/python/cpython/blob/main/Python/importdl.c#L208) meaning it doesn't have the chance to stop the import before the segmentation fault.I discovered this by writing a simple test harness that goes through the Python test suite and tries to run each test in a sub interpreter. It segfaults on a number of test suites (
test_builtinis the first one)This crashes during the
test_builtinsuite and any others which dynamically load the readline module.Stack trace:
CPython versions tested on:
CPython main branch
Operating systems tested on:
macOS
Linked PRs