Check for sub interpreter support is not catching readline global state and crashing

[tonybaloney]

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}")
        continue

This 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

• gh-112292 : Catch import error conditions with readline hooks #112313

[tonybaloney]

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

[tonybaloney]

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

[tonybaloney]

@ericsnowcurrently I worked it out in the end 😖

[tonybaloney]

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]

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]

That said, fixing the error returns situation in Modules/readline.c is definitely worth doing.

[ericsnowcurrently]

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 here¹ 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

1. In this case the side effect is a registered library callback with the faulty expectation that PyState_FindModule() never returns NULL. ↩

[ericsnowcurrently]

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]

Related comment in #103092 #103092 (comment)

[tonybaloney]

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

1. 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

[encukou]

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]

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]

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]

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]

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]

This should be a non-issue since gh-121060.