Skip to content

getargs.c is Breaking Interpeter Isolation #119213

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.

Sign up for GitHub

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

Jump to bottom
Closed
ericsnowcurrently opened this issue May 20, 2024 · 4 comments
Closed

getargs.c is Breaking Interpeter Isolation #119213

ericsnowcurrently opened this issue May 20, 2024 · 4 comments
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented May 20, 2024 •
edited by bedevere-app bot
Loading

Bug report

Bug description:

@1st1 encountered some crashes due to this.

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

No response

Linked PRs
@ericsnowcurrently ericsnowcurrently added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters 3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes labels May 20, 2024
This was referenced May 20, 2024
Closed
Closed
@ericsnowcurrently
Copy link
Member Author

Here's what I've found:

  • there's a bug (in 3.13/main) where _PyRuntime.getargs.static_parsers is only ever set to the last node added by _parser_init()
  • for non-builtin modules (even for Py_BUILD_CORE_MODULE) _PyArg_Parser.kwtuple is allocated on the heap under the current interpreter

The first bug masks the problem on 3.13/main, so thankfully @1st1 was testing on 3.12.

The second bug is solvable by either always statically allocating the kwtuple or by always temporarily switching to the main interpreter when allocating from the heap.

Now that I can reliably reproduce the crash I'll have a fix up soon.

@ericsnowcurrently
Copy link
Member Author

One other note:

If I destroy the interpreter before runtime finalization I hit an assertion about negative refcount:

./Include/object.h:1040: _Py_NegativeRefcount: Assertion failed: object has negative ref count
<object at 0x7f6922fd5800 is freed>
Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: finalizing (tstate=0x000055d542ee5cb0)

Current thread 0x00007f692773b100 (most recent call first):
  <no Python frame>
Aborted (core dumped)

If I do not destroy the subinterpreter before runtime fini then it crashes due to the dangling pointer:

Debug memory block at address p=0x7f06a7ad4620: API ''
    71492436437630461 bytes originally requested
    The 7 pad bytes at p-7 are not all FORBIDDENBYTE (0xfd):
        at p-7: 0x00 *** OUCH
        at p-6: 0x00 *** OUCH
        at p-5: 0x00 *** OUCH
        at p-4: 0x00 *** OUCH
        at p-3: 0x00 *** OUCH
        at p-2: 0x00 *** OUCH
        at p-1: 0x00 *** OUCH
    Because memory is corrupted at the start, the count of bytes requested
       may be bogus, and checking the trailing pad bytes may segfault.
    The 8 pad bytes at tail=0xfe7d04a5ab441d are Segmentation fault (core dumped)

@bedevere-app bedevere-app bot mentioned this issue May 21, 2024
Merged
ericsnowcurrently added a commit that referenced this issue May 22, 2024
@ericsnowcurrently
gh-119213: Be More Careful About _PyArg_Parser.kwtuple Across Interpr…
8186500 
…eters (gh-119331)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 22, 2024
@ericsnowcurrently @miss-islington
pythongh-119213: Be More Careful About _PyArg_Parser.kwtuple Across I…
05a00aa 
…nterpreters (pythongh-119331)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.
(cherry picked from commit 8186500)

Co-authored-by: Eric Snow <[email protected]>
@bedevere-app bedevere-app bot mentioned this issue May 22, 2024
Merged
ericsnowcurrently added a commit that referenced this issue May 22, 2024
@miss-islington @ericsnowcurrently
[3.13] gh-119213: Be More Careful About _PyArg_Parser.kwtuple Across …
bfd9c3e 
…Interpreters (gh-119331) (gh-119410)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.

(cherry picked from commit 8186500)

Co-authored-by: Eric Snow <[email protected]>
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this issue May 22, 2024
@ericsnowcurrently
pythongh-119213: Be More Careful About _PyArg_Parser.kwtuple Across I…
c4bf08b 
…nterpreters (pythongh-119331)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.
@bedevere-app bedevere-app bot mentioned this issue May 22, 2024
Merged
@ericsnowcurrently
Copy link
Member Author

FTR, original change: gh-95860

ericsnowcurrently added a commit that referenced this issue May 22, 2024
@ericsnowcurrently
[3.12] gh-119213: Be More Careful About _PyArg_Parser.kwtuple Across …
0d5fe2c 
…Interpreters (gh-119331) (gh-119425)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.

(cherry picked from commit 8186500)
@ericsnowcurrently
Copy link
Member Author

Thanks for identifying the bug here, @1st1. Let me know if you notice any problems with the fix.

@github-project-automation github-project-automation bot moved this from Todo to Done in Subinterpreters May 23, 2024
@JelleZijlstra JelleZijlstra mentioned this issue May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@ericsnowcurrently @estyxx
pythongh-119213: Be More Careful About _PyArg_Parser.kwtuple Across I…
712aaf2 
…nterpreters (pythongh-119331)

_PyArg_Parser holds static global data generated for modules by Argument Clinic.  The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global.  In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters.  However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed.

This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes.  It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime.  The solution here is to temporarily switch to the main interpreter.  The alternative would be to always statically allocate the tuple.

This change also fixes a bug where only the most recent parser was added to the global linked list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Subinterpreters
Status: Done
Milestone
No milestone
Development

No branches or pull requests
1 participant
@ericsnowcurrently