Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault when trying to use PyRun_SimpleString() with some imports #124160

Closed
luk1337 opened this issue Sep 17, 2024 · 14 comments
Closed

Segfault when trying to use PyRun_SimpleString() with some imports #124160

luk1337 opened this issue Sep 17, 2024 · 14 comments
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes deferred-blocker topic-subinterpreters type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@luk1337
Copy link
Contributor

luk1337 commented Sep 17, 2024

Crash report

What happened?

I hit the segfault when doing the following thing:

$ docker run -ti fedora:41 bash
# dnf -y install gcc python-devel
# echo '#include <Python.h>

int main() {
    Py_Initialize();
    PyThreadState_Swap(Py_NewInterpreter());
    PyRun_SimpleString("import readline");
}' > test.c
# gcc test.c -I/usr/include/python3.13 -lpython3.13
# ./a.out 
Segmentation fault (core dumped)
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7a8c3cb in reload_singlephase_extension (tstate=tstate@entry=0x7ffff7e5a850, cached=cached@entry=0x0, 
    info=info@entry=0x7fffffff8c90) at /usr/src/debug/python3.13-3.13.0~rc2-1.fc41.x86_64/Python/import.c:1763
1763	    PyModuleDef *def = cached->def;

The same code doesn't crash on 3.12.

CPython versions tested on:

3.13

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

Python 3.13.0rc2 (main, Sep 7 2024, 00:00:00) [GCC 14.2.1 20240801 (Red Hat 14.2.1-1)]

Linked PRs

@luk1337 luk1337 added the type-crash A hard crash of the interpreter, possibly with a core dump label Sep 17, 2024
luk1337 added a commit to luk1337/cpython that referenced this issue Sep 17, 2024
Otherwise it'll always return NULL if tstate != main_tstate due to
_Py_IsMainInterpreter() check inside it.
@ZeroIntensity
Copy link
Member

I wasn't able to get an embedded interpreter working with requests, so I couldn't confirm the patch. (Embedding a locally compiled interpreter is still pretty painful.)

Though, I did try this directly on CPython (via _testcapi), and I wasn't able to reproduce it. It's odd that this only occurs when embedding.

(@encukou, for now, this needs topic-C-API and 3.13. This might be subinterpreter-related, but I don't know -- I'll look futher into it tomorrow.)

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 18, 2024

I wasn't able to get an embedded interpreter working with requests, so I couldn't confirm the patch. (Embedding a locally compiled interpreter is still pretty painful.)

Though, I did try this directly on CPython (via _testcapi), and I wasn't able to reproduce it. It's odd that this only occurs when embedding.

(@encukou, for now, this needs topic-C-API and 3.13. This might be subinterpreter-related, but I don't know -- I'll look futher into it tomorrow.)

Just fyi, you need both brotli and requests, it won't die w/o both of them installed. Unless importing brotl alone can trigger this crash too, but I haven't tried that myself.

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 18, 2024

I wasn't able to get an embedded interpreter working with requests, so I couldn't confirm the patch. (Embedding a locally compiled interpreter is still pretty painful.)
Though, I did try this directly on CPython (via _testcapi), and I wasn't able to reproduce it. It's odd that this only occurs when embedding.
(@encukou, for now, this needs topic-C-API and 3.13. This might be subinterpreter-related, but I don't know -- I'll look futher into it tomorrow.)

Just fyi, you need both brotli and requests, it won't die w/o both of them installed. Unless importing brotl alone can trigger this crash too, but I haven't tried that myself.

It does, I updated the op.

@encukou encukou added topic-C-API 3.13 bugs and security fixes labels Sep 18, 2024
@ZeroIntensity
Copy link
Member

Oh, I only tried this with requests, not brotli. Has this been reported to them yet? It could be something specific to their initialization function, rather than CPython.

With that being said, it doesn't look like brotli is doing anything complex -- it's just a simple single-phase init, as far as I can tell. But if this applied to all single-phase init extensions under subinterpreters, I would hope that it would have been reported much earlier in 3.13's development.

I wonder if something else is going wrong here, because you'll lose some errors by omitting NULL checks. Does PyThreadState_Swap return anything iffy? What about Py_NewInterpreter?

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 18, 2024

Oh, I only tried this with requests, not brotli. Has this been reported to them yet? It could be something specific to their initialization function, rather than CPython.

With that being said, it doesn't look like brotli is doing anything complex -- it's just a simple single-phase init, as far as I can tell. But if this applied to all single-phase init extensions under subinterpreters, I would hope that it would have been reported much earlier in 3.13's development.

This happens with some other modules too, e.g.

(gdb) bt
#0  0x00007f14bca8c3cb in reload_singlephase_extension (tstate=tstate@entry=0x7f14bc28d850, 
    cached=cached@entry=0x0, info=info@entry=0x7ffdaa112e00)
    at /usr/src/debug/python3.13-3.13.0~rc2-1.fc41.x86_64/Python/import.c:1763
#1  0x00007f14bc82d42e in import_run_extension (tstate=tstate@entry=0x7f14bc28d850, 
    p0=p0@entry=0x7f14ba909330 <PyInit__rust>, info=info@entry=0x7ffdaa112e00, 
    spec=spec@entry=<ModuleSpec(name='cryptography.hazmat.bindings._rust', loader=<ExtensionFileLoader(name='cryptography.hazmat.bindings._rust', path='/usr/lib64/python3.13/site-packages/cryptography/hazmat/bindings/_rust.abi3.so') at remote 0x7f14bac5c910>, origin='/usr/lib64/python3.13/site-packages/cryptography/hazmat/bindings/_rust.abi3.so', loader_state=None, submodule_search_locations=None, _uninitialized_submodules=[], _set_fileattr=True, _cached=None) at remote 0x7f14bac547d0>, modules=<optimized out>)
    at /usr/src/debug/python3.13-3.13.0~rc2-1.fc41.x86_64/Python/import.c:2099

so I assumed this is a regression in CPython and that module maintainers shouldn't have to apply workarounds to fix 3.13 support.

I wonder if something else is going wrong here, because you'll lose some errors by omitting NULL checks. Does PyThreadState_Swap return anything iffy? What about Py_NewInterpreter?

[root@377a2e8f31b5 /]# cat test.c 
#include <Python.h>

int main() {
    Py_Initialize();
    PyThreadState *a = Py_NewInterpreter();
    printf("a=%p\n", a);
    PyThreadState *b = PyThreadState_Swap(a);
    printf("b=%p\n", b);
    PyRun_SimpleFile(fopen("test.py", "r"), "test.py");
}
[root@377a2e8f31b5 /]# ./a.out 
a=0x7ffb3c7ab850
b=0x7ffb3c7ab850
Segmentation fault (core dumped)

@ZeroIntensity
Copy link
Member

This happens with some other modules too, e.g.

Well that's not good :(

Are there any modules in the stdlib that this occurs with? (That makes it much easier to track down, so you don't have to deal with virtual environments when testing.)

Also, I'm assuming that the PyThreadState_Swap call is necessary for the reproducer. That runs the imports in a subinterpreter, rather than the main interpreter -- not all third party modules support that. Does this occur without that? (If not, that gives some good insight into what's going on.)

@vstinner
Copy link
Member

cc @ericsnowcurrently

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 18, 2024

This happens with some other modules too, e.g.

Well that's not good :(

Are there any modules in the stdlib that this occurs with? (That makes it much easier to track down, so you don't have to deal with virtual environments when testing.)

Also, I'm assuming that the PyThreadState_Swap call is necessary for the reproducer. That runs the imports in a subinterpreter, rather than the main interpreter -- not all third party modules support that. Does this occur without that? (If not, that gives some good insight into what's going on.)

No idea about stdlib also it doesn't happen w/o PyThreadState_Swap(). BTW I uploaded a simple PR that fixes it, if you didn't notice it yet ^^

@ZeroIntensity
Copy link
Member

I saw, I just don't know whether it's the correct fix because I can't reproduce it yet.

No idea about stdlib also it doesn't happen w/o PyThreadState_Swap()

Good to know, that means this is a subinterpreter problem. As far as I can see, brotli indicates that it supports subinterpreters via a module size of 0, but they are using global variables, so that might be incorrect on their end.

@luca020400
Copy link

luca020400 commented Sep 18, 2024

Wrote a quick script to traverse the whole support std modules and there's a few hits that make it crash

import sys
import importlib

for mod in sys.stdlib_module_names:
    try:
        print(mod)
        importlib.import_module(mod)
    except:
        pass

so far readline, rlcompleter, doctest, _tracemalloc make it crash on my machine like brotli.

Since the list isn't sorted you may be able to hit a few more entries I missed :)


Update to handle exceptions since some modules are listed but not loadable (like turtle)

@ZeroIntensity
Copy link
Member

Thanks! I don't have a chance to try and reproduce this at the moment, but I think the problem is with using a module state with single-phase init under a subinterpreter. The fix by @luk1337 makes sense to me.

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 18, 2024

Wrote a quick script to traverse the whole support std modules and there's a few hits that make it crash

import sys
import importlib

for mod in sys.stdlib_module_names:
    try:
        print(mod)
        importlib.import_module(mod)
    except:
        pass

so far readline, rlcompleter, doctest, _tracemalloc make it crash on my machine like brotli.

Since the list isn't sorted you may be able to hit a few more entries I missed :)

Update to handle exceptions since some modules are listed but not loadable (like turtle)

thx, I updated OP with readline instead of brotli

@ericsnowcurrently
Copy link
Member

FWIW, the fix in gh-124164 is correct. It should have been using main_tstate.

@luk1337 luk1337 changed the title Segfault when trying to use PyRun_SimpleFile() with some imports Segfault when trying to use PyRun_SimpleString() with some imports Sep 18, 2024
luk1337 added a commit to luk1337/cpython that referenced this issue Sep 18, 2024
Otherwise it'll always return NULL if tstate != main_tstate due to
_Py_IsMainInterpreter() check inside it.

Also, add a regression test that makes sure that `readline` is
importable in case that triggered the crash before.
luk1337 added a commit to luk1337/cpython that referenced this issue Sep 18, 2024
Otherwise it'll always return NULL if tstate != main_tstate due to
_Py_IsMainInterpreter() check inside it.

Also, add a regression test that makes sure that `readline` is
importable in case that triggered the crash before.
luk1337 added a commit to luk1337/cpython that referenced this issue Sep 18, 2024
Otherwise it'll always return NULL if tstate != main_tstate due to
_Py_IsMainInterpreter() check inside it.

Also, add a regression test that makes sure that `readline` is
importable in case that triggered the crash before.
@ZeroIntensity
Copy link
Member

Alas, a reproducer that does not require C code:

import _interpreters

interp = _interpreters.create()
_interpreters.run_string(interp, "import readline")

@kumaraditya303 kumaraditya303 added the 3.14 new features, bugs and security fixes label Sep 19, 2024
luk1337 added a commit to luk1337/cpython that referenced this issue Sep 19, 2024
hroncok pushed a commit to fedora-python/cpython that referenced this issue Sep 19, 2024
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this issue Sep 22, 2024
Yhg1s pushed a commit that referenced this issue Sep 23, 2024
…on() (GH-124164) (#124250)

gh-124160: Pass main_tstate to update_global_state_for_extension() (GH-124164)
(cherry picked from commit 7331d0f)

Co-authored-by: luk1337 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes deferred-blocker topic-subinterpreters type-crash A hard crash of the interpreter, possibly with a core dump
Projects
Status: Done
Development

No branches or pull requests

7 participants