gh-124160: Pass main_tstate to update_global_state_for_extension() #124164

luk1337 · 2024-09-17T09:39:57Z

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.

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

cpython-cla-bot · 2024-09-17T09:40:01Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-09-17T09:40:05Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

ZeroIntensity · 2024-09-18T15:05:16Z

I'll review this when I reproduce the issue later today. In the meantime, this needs a NEWS entry.

luk1337 · 2024-09-18T15:09:08Z

I'll review this when I reproduce the issue later today. In the meantime, this needs a NEWS entry.

Would you be so nice to write one for me?

luk1337 · 2024-09-18T15:10:19Z

I'll review this when I reproduce the issue later today. In the meantime, this needs a NEWS entry.

Would you be so nice to write one for me?

I left Allow edits by maintainers enabled so you can even push to my branch.

ZeroIntensity · 2024-09-18T15:13:01Z

Sure, I'll do that alongside the review.

ericsnowcurrently

The change is correct. In this entire section, the main tstate is always active and should always be targeted.

Before merging this, let's add a regression test that would have caught the reported failure.

bedevere-app · 2024-09-18T15:30:11Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

luk1337 · 2024-09-18T15:52:07Z

The change is correct. In this entire section, the main tstate is always active and should always be targeted.

Before merging this, let's add a regression test that would have caught the reported failure.

diff --git a/Programs/_testembed.c b/Programs/_testembed.c
index 342cc91cc58..5506ba21948 100644
--- a/Programs/_testembed.c
+++ b/Programs/_testembed.c
@@ -95,6 +95,14 @@ static void _testembed_Py_Initialize(void)
 }
 
 
+static int test_import_in_subinterpreters(void)
+{
+    _testembed_Py_InitializeFromConfig();
+    PyThreadState_Swap(Py_NewInterpreter());
+    PyRun_SimpleString("import readline"); // gh-124160
+}
+
+
 /*****************************************************
  * Test repeated initialisation and subinterpreters
  *****************************************************/
@@ -2398,6 +2406,7 @@ static struct TestCase TestCases[] = {
     {"test_repeated_init_exec", test_repeated_init_exec},
     {"test_repeated_simple_init", test_repeated_simple_init},
     {"test_forced_io_encoding", test_forced_io_encoding},
+    {"test_import_in_subinterpreters", test_import_in_subinterpreters},
     {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters},
     {"test_repeated_init_and_inittab", test_repeated_init_and_inittab},
     {"test_pre_initialization_api", test_pre_initialization_api},

does this look ok?

ZeroIntensity · 2024-09-18T16:02:21Z

Looks good, but we should be using NULL checks in tests.

luk1337 · 2024-09-18T16:04:42Z

Looks good, but we should be using NULL checks in tests.

There are no null checks for Py_NewInterpreter(), PyThreadState_Swap(), PyRun_SimpleString() in the whole file and this test is just crashing w/o this PR.

ZeroIntensity · 2024-09-18T16:08:01Z

Yeah, functions like PyRun_SimpleString can fail. In that case, we don't want to segfault the tests, we should just return -1 and let the error propagate.

luk1337 · 2024-09-18T16:12:05Z

Yeah, functions like PyRun_SimpleString can fail. In that case, we don't want to segfault the tests, we should just return -1 and let the error propagate.

ah indeed, I forgot about the return anyway.

bedevere-app · 2024-09-18T16:15:15Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

luk1337 · 2024-09-18T16:15:25Z

I have made the requested changes; please review again

bedevere-app · 2024-09-18T16:15:31Z

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

luk1337 · 2024-09-18T16:31:28Z

Added short news entry

Misc/NEWS.d/next/C_API/2024-09-18-18-30-16.gh-issue-124160.wFuEVx.rst

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.

ericsnowcurrently

LGTM, with one caveat about the test using the readline module

ericsnowcurrently · 2024-09-18T17:25:14Z

Programs/_testembed.c

+static int test_import_in_subinterpreters(void)
+{
+    _testembed_Py_InitializeFromConfig();
+    PyThreadState_Swap(Py_NewInterpreter());
+    return PyRun_SimpleString("import readline"); // gh-124160
+}


This works, but only as long as the readline module fits the relevant criteria. That's probably fine.

Ideally we would add a new test module for this dedicated case, though we could probably reuse _testsinglephase_with_reinit (in Modules/_testsinglephase.c). If you think that would take much time, I'm fine with addressing that separately, so we can get this PR merged sooner.

Would prefer if someone else figured out how to set up a more robust test case for this issue in a separate PR.

ZeroIntensity · 2024-09-18T17:33:05Z

I've added the 3.13 backport label, since this is a regression from 3.12. Unfortunately, I don't think this can go in until 3.13.1, as the branch is locked right now.

ZeroIntensity

I've gone through and confirmed that this fixes importing for all stateful single-phase modules. Thank you for the fix, @luk1337!

miss-islington-app · 2024-09-19T16:05:24Z

Thanks @luk1337 for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…n() (pythonGH-124164) (cherry picked from commit 7331d0f) Co-authored-by: luk1337 <priv.luk@gmail.com>

bedevere-app · 2024-09-19T16:05:34Z

GH-124250 is a backport of this pull request to the 3.13 branch.

…xtension() (python#124164)

…n() (python#124164)

…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 <priv.luk@gmail.com>

luk1337 requested a review from kumaraditya303 as a code owner September 17, 2024 09:39

bedevere-app bot added the awaiting review label Sep 17, 2024

bedevere-app bot mentioned this pull request Sep 17, 2024

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

Closed

ericsnowcurrently requested changes Sep 18, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Sep 18, 2024

luk1337 force-pushed the luk/import branch from 878d596 to 09a486b Compare September 18, 2024 16:15

bedevere-app bot added awaiting change review and removed awaiting changes labels Sep 18, 2024

bedevere-app bot requested a review from ericsnowcurrently September 18, 2024 16:15

luk1337 force-pushed the luk/import branch from 09a486b to 1e781f7 Compare September 18, 2024 16:31

ZeroIntensity reviewed Sep 18, 2024

View reviewed changes

Misc/NEWS.d/next/C_API/2024-09-18-18-30-16.gh-issue-124160.wFuEVx.rst Outdated Show resolved Hide resolved

luk1337 force-pushed the luk/import branch from 1e781f7 to 679b5b6 Compare September 18, 2024 16:40

ericsnowcurrently approved these changes Sep 18, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Sep 18, 2024

ZeroIntensity added the needs backport to 3.13 bugs and security fixes label Sep 18, 2024

ZeroIntensity approved these changes Sep 18, 2024

View reviewed changes

kumaraditya303 approved these changes Sep 19, 2024

View reviewed changes

kumaraditya303 merged commit 7331d0f into python:main Sep 19, 2024
39 of 40 checks passed

bedevere-app bot removed the awaiting merge label Sep 19, 2024

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 19, 2024

pythongh-124160: Pass main_tstate to update_global_state_for_extensio…

de74ce9

…n() (pythonGH-124164) (cherry picked from commit 7331d0f) Co-authored-by: luk1337 <priv.luk@gmail.com>

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 19, 2024

luk1337 deleted the luk/import branch September 19, 2024 16:06

luk1337 added a commit to luk1337/cpython that referenced this pull request Sep 19, 2024

00440: pythongh-124160: Pass main_tstate to update_global_state_for_e…

5abec04

…xtension() (python#124164)

savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024

pythongh-124160: Pass main_tstate to update_global_state_for_extensio…

6802f71

…n() (python#124164)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-124160: Pass main_tstate to update_global_state_for_extension() #124164

gh-124160: Pass main_tstate to update_global_state_for_extension() #124164

luk1337 commented Sep 17, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 17, 2024 •

edited

Loading

bedevere-app bot commented Sep 17, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

ericsnowcurrently left a comment

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ericsnowcurrently left a comment

ericsnowcurrently Sep 18, 2024

luk1337 Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

ZeroIntensity left a comment

miss-islington-app bot commented Sep 19, 2024

bedevere-app bot commented Sep 19, 2024

gh-124160: Pass main_tstate to update_global_state_for_extension() #124164

gh-124160: Pass main_tstate to update_global_state_for_extension() #124164

Conversation

luk1337 commented Sep 17, 2024 • edited Loading

cpython-cla-bot bot commented Sep 17, 2024 • edited Loading

bedevere-app bot commented Sep 17, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

ericsnowcurrently left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ZeroIntensity commented Sep 18, 2024

luk1337 commented Sep 18, 2024

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

bedevere-app bot commented Sep 18, 2024

luk1337 commented Sep 18, 2024

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently Sep 18, 2024

Choose a reason for hiding this comment

luk1337 Sep 18, 2024

Choose a reason for hiding this comment

ZeroIntensity commented Sep 18, 2024

ZeroIntensity left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Sep 19, 2024

bedevere-app bot commented Sep 19, 2024

luk1337 commented Sep 17, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 17, 2024 •

edited

Loading