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

gh-119213: Fix getargs.c to store state in InterpreterState... #119195

Closed
wants to merge 1 commit into from

Conversation

1st1
Copy link
Member

@1st1 1st1 commented May 20, 2024

...as opposed to storing it in PyRuntime. Storing it in PyRuntime
is fundametally wrong, as its state contains references to Python
objects. Those objects (tuples and strings) can (and will) be
picked by various subinterpreter clean up code, leaving PyRuntime
with broken pointers.

#119194 is a backport to 3.12

@1st1 1st1 requested a review from ericsnowcurrently as a code owner May 20, 2024 01:31
@ericsnowcurrently ericsnowcurrently changed the title Fix getargs.c to store state in InterpreterState... gh-119213: Fix getargs.c to store state in InterpreterState... May 20, 2024
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; perhaps it would make sense to init getargs state just after _PyGC_Init, but I'm not sure it matters; I'll leave that kind of nitpicking to Eric :)

@colesbury
Copy link
Contributor

How does this work when the _PyArg_Parser instances are themselves declared as static variables (and are global to the process)? For example:

static _PyArg_Parser _parser = {
.keywords = _keywords,
.fname = "Struct",
.kwtuple = KWTUPLE,
};

@erlend-aasland erlend-aasland self-requested a review May 20, 2024 16:16
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka: Would you mind to review this change?

@serhiy-storchaka wrote this API.

@ericsnowcurrently
Copy link
Member

@colesbury, I'm looking into what's going on. Basically, the statically declared tuple is only for builtin modules. I have a solution that's different from Yury's but want to be sure it's correct before closing this one.

@ericsnowcurrently
Copy link
Member

superseded by gh-119331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants