Repository navigation
Check for sub interpreter support is not catching readline global state and crashing #112292
Description
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_builtin is the first one)
from test.libregrtest.findtests import findtests
import _xxsubinterpreters as interpreters
# Get a list of tests
test_names = findtests()
skip_tests = [
# "test_builtin" # Crashes
]
def run_test():
import unittest
test_cases = unittest.defaultTestLoader.loadTestsFromName(f"test.{test_name}")
reasons = ""
pass_count = 0
fail_count = 0
for case in test_cases:
r = unittest.result.TestResult()
case.run(r)
if r.wasSuccessful():
pass_count += r.testsRun
else:
for failedcase, reason in r.failures:
reasons += "---------------------------------------------------------------\n"
reasons += f"Test case {failedcase} failed:\n"
reasons += reason
reasons += "\n---------------------------------------------------------------\n"
fail_count += 1
for failedcase, reason in r.errors:
reasons += (
"---------------------------------------------------------------\n"
)
reasons += f"Test case {failedcase} failed with errors:\n"
reasons += reason
reasons += "\n---------------------------------------------------------------\n"
fail_count += 1
interp = interpreters.create()
for test in test_names:
# Run the test suite
if test in skip_tests:
print(f"Skipping test {test}")
continue
print(f"Running test {test}")
try:
result = interpreters.run_func(interp, run_test, shared={"test_name": test})
except Exception as e:
print(f"Test {test} failed with exception {e}")
continueThis crashes during the test_builtin suite and any others which dynamically load the readline module.
> ./python.exe -X dev test_in_interp.py
....
Running test test_bufio
Running test test_builtin
Fatal Python error: Segmentation fault
Current thread 0x00007ff857472700 (most recent call first):
File "<frozen importlib._bootstrap>", line 488 in _call_with_frames_removed
File "<frozen importlib._bootstrap_external>", line 1304 in create_module
File "<frozen importlib._bootstrap>", line 813 in module_from_spec
File "<frozen importlib._bootstrap>", line 915 in _load_unlocked
File "<frozen importlib._bootstrap>", line 1325 in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 1354 in _find_and_load
File "/Users/anthonyshaw/projects/cpython/Lib/pdb.py", line 239 in __init__
File "/Users/anthonyshaw/projects/cpython/Lib/doctest.py", line 386 in __init__
File "/Users/anthonyshaw/projects/cpython/Lib/doctest.py", line 1527 in run
File "/Users/anthonyshaw/projects/cpython/Lib/doctest.py", line 2261 in runTest
File "/Users/anthonyshaw/projects/cpython/Lib/unittest/case.py", line 589 in _callTestMethod
File "/Users/anthonyshaw/projects/cpython/Lib/unittest/case.py", line 636 in run
File "/Users/anthonyshaw/projects/cpython/Lib/unittest/case.py", line 692 in __call__
File "/Users/anthonyshaw/projects/cpython/Lib/unittest/suite.py", line 122 in run
File "/Users/anthonyshaw/projects/cpython/test_in_interp.py", line 19 in run_test
Extension modules: _testcapi, _xxsubinterpreters (total: 2)
zsh: segmentation fault ./python.exe -X dev test_in_interp.pyStack trace:
python.exe!Py_TYPE (/Users/anthonyshaw/projects/cpython/Include/object.h:297)
python.exe!Py_IS_TYPE (/Users/anthonyshaw/projects/cpython/Include/object.h:330)
python.exe!PyObject_TypeCheck (/Users/anthonyshaw/projects/cpython/Include/object.h:487)
python.exe!PyModule_GetState (/Users/anthonyshaw/projects/cpython/Objects/moduleobject.c:608)
readline.cpython-313td-darwin.so!on_startup_hook (/Users/anthonyshaw/projects/cpython/Modules/readline.c:1029)
libedit.3.dylib!rl_initialize (Unknown Source:0)
readline.cpython-313td-darwin.so!setup_readline (/Users/anthonyshaw/projects/cpython/Modules/readline.c:1219)
readline.cpython-313td-darwin.so!PyInit_readline (/Users/anthonyshaw/projects/cpython/Modules/readline.c:1515)
python.exe!_PyImport_LoadDynamicModuleWithSpec (/Users/anthonyshaw/projects/cpython/Python/importdl.c:170)
python.exe!_imp_create_dynamic_impl (/Users/anthonyshaw/projects/cpython/Python/import.c:3750)
python.exe!_imp_create_dynamic (/Users/anthonyshaw/projects/cpython/Python/clinic/import.c.h:485)
python.exe!cfunction_vectorcall_FASTCALL (/Users/anthonyshaw/projects/cpython/Objects/methodobject.c:425)
python.exe!_PyVectorcall_Call (/Users/anthonyshaw/projects/cpython/Objects/call.c:273)
python.exe!_PyObject_Call (/Users/anthonyshaw/projects/cpython/Objects/call.c:348)
python.exe!PyObject_Call (/Users/anthonyshaw/projects/cpython/Objects/call.c:373)
python.exe!_PyEval_EvalFrameDefault (/Users/anthonyshaw/projects/cpython/Python/generated_cases.c.h:5382)
python.exe!_PyEval_EvalFrame (/Users/anthonyshaw/projects/cpython/Include/internal/pycore_ceval.h:115)
python.exe!_PyEval_Vector (/Users/anthonyshaw/projects/cpython/Python/ceval.c:1783)
python.exe!_PyFunction_Vectorcall (Unknown Source:0)
python.exe!_PyObject_VectorcallTstate (/Users/anthonyshaw/projects/cpython/Include/internal/pycore_call.h:168)
CPython versions tested on:
CPython main branch
Operating systems tested on:
macOS
Activity
tonybaloney commentedon Nov 22, 2023
After some investigation, this is what I've established:
on_startupwhich uses a macro to read the module state.readlinestate_globalmacro doesn't check if the result ofPyState_FindModuleis NULL and segfaultsPyState_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.tonybaloney commentedon Nov 22, 2023
Furthermore:
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.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
tonybaloney commentedon Nov 22, 2023
@ericsnowcurrently I worked it out in the end 😖
tonybaloney commentedon Nov 22, 2023
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.ericsnowcurrently commentedon Nov 22, 2023
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.
ericsnowcurrently commentedon Nov 22, 2023
That said, fixing the error returns situation in Modules/readline.c is definitely worth doing.
ericsnowcurrently commentedon Nov 22, 2023
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. ↩ericsnowcurrently commentedon Nov 22, 2023
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.
tonybaloney commentedon Nov 23, 2023
Related comment in #103092 #103092 (comment)
tonybaloney commentedon Nov 23, 2023
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 issueencukou commentedon Nov 27, 2023
Keep a memo of all single-phase
PyInit_*functions that were called (successfully or not), and refuse to call one a second time?ericsnowcurrently commentedon Nov 27, 2023
That seems like it would do the job. Good idea.
tonybaloney commentedon Nov 27, 2023
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?tonybaloney commentedon Nov 28, 2023
I tried that, it didn't seem to fix this specific issue
gh-112292 : Catch import error conditions with readline hooks (gh-112313
pythongh-112292 : Catch import error conditions with readline hooks (p…
stefanholek commentedon Jun 15, 2024
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!
ericsnowcurrently commentedon Jul 8, 2024
This should be a non-issue since gh-121060.
pythongh-112292 : Catch import error conditions with readline hooks (p…