GeezerVison is showing github.com — Check for sub interpreter support is not catching readline global state and crashing · Issue #112292 · python/cpython
Check for sub interpreter support is not catching readline global state and crashing #112292DescriptionBug 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.py Stack 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
Linked PRs
Activitytonybaloney commented on Nov 22, 2023 ContributorAuthor More actions 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_startup which uses a macro to read the module state.
- The
readlinestate_global macro doesn't check if the result of PyState_FindModule is NULL and segfaults
PyState_FindModule is returning NULL (but not setting an exception) because the module state list in the interpreter state is smaller than the index (for example, m_index is 54 and the list is only 4 long). m_index is 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. Aasland tonybaloney commented on Nov 22, 2023 ContributorAuthor More actions Furthermore:
- The first time,
readline successfully imports, but within _PyImport_LoadDynamicModuleWithSpec it calls _PyImport_CheckSubinterpIncompatibleExtensionAllowed which returns and error, goes to the error condition and skips _PyImport_FixupExtensionObject, so even though m_index is 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 tonybaloney commented on Nov 22, 2023 ContributorAuthor More actions Reacted by Eric Snow and Erlend E. Aasland tonybaloney commented on Nov 22, 2023 ContributorAuthor More actions An alternative and more universal fix would be to somehow have a check like _PyImport_CheckSubinterpIncompatibleExtensionAllowed earlier in _PyImport_LoadDynamicModuleWithSpec so that the init function is never called in the first place. ericsnowcurrently commented on Nov 22, 2023
An alternative and more universal fix would be to somehow have a check like _PyImport_CheckSubinterpIncompatibleExtensionAllowed earlier in _PyImport_LoadDynamicModuleWithSpec so 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. ericsnowcurrently commented on Nov 22, 2023 That said, fixing the error returns situation in Modules/readline.c is definitely worth doing. Reacted by Erlend E. Aasland ericsnowcurrently commented on 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 returns NULL. ↩
ericsnowcurrently commented on 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. Reacted by Petr Viktorin and Erlend E. Aasland tonybaloney commented on Nov 23, 2023 ContributorAuthor More actions tonybaloney commented on Nov 23, 2023 ContributorAuthor More actions
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 returns NULL. ↩
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 _testcapi isn't supported, but I'll look into them in another issue Reacted by Eric Snow and Erlend E. Aasland encukou commented on Nov 27, 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.
@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? ericsnowcurrently commented on 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?
That seems like it would do the job. Good idea. tonybaloney commented on Nov 27, 2023 ContributorAuthor More actions I think you might get the same result if you call _PyImport_ClearExtension / clear_singlephase_extension if _PyImport_CheckSubinterpIncompatibleExtensionAllowed() returns false along with raising an exception? tonybaloney commented on Nov 28, 2023 ContributorAuthor More actions
I think you might get the same result if you call _PyImport_ClearExtension / clear_singlephase_extension if _PyImport_CheckSubinterpIncompatibleExtensionAllowed() returns false along with raising an exception?
I tried that, it didn't seem to fix this specific issue stefanholek commented on 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.py as 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 commented on Jul 8, 2024 Reacted by Stefan H. Holek
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|