Skip to content

gh-146355: Fix main_module ref leak in _PyRun_SimpleStringFlagsWithName#146356

Open
A0su wants to merge 2 commits intopython:mainfrom
A0su:Fix-SimpleStringFlagsWithName-main-refleak
Open

gh-146355: Fix main_module ref leak in _PyRun_SimpleStringFlagsWithName#146356
A0su wants to merge 2 commits intopython:mainfrom
A0su:Fix-SimpleStringFlagsWithName-main-refleak

Conversation

@A0su
Copy link

@A0su A0su commented Mar 24, 2026

gh-146355: Fix main_module ref leak in _PyRun_SimpleStringFlagsWithName

This is a sub-issue for #146102 and original gist details can be found here

When the_name is NULL we'll fail to call Py_DECREF on main_module. I used the same goto pattern as _PyRun_SimpleFileObject to fix this.

I omitted a new test entry as we have indirect coverage of functionality in test_embed.py. Not sure if it makes sense for the scope of fixing the ref leak to include the additional code for a test in test_run.py (which has a TODO for this function).

@A0su A0su requested a review from ericsnowcurrently as a code owner March 24, 2026 03:03
@bedevere-app
Copy link

bedevere-app bot commented Mar 24, 2026

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.

@python-cla-bot
Copy link

python-cla-bot bot commented Mar 24, 2026

All commit authors signed the Contributor License Agreement.

CLA signed

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.

I dislike this change. IMO it makes the code harder to follow and more complex. I would prefer just that to fix the leak:

diff --git a/Python/pythonrun.c b/Python/pythonrun.c
index a21f494dc69..1e327848e65 100644
--- a/Python/pythonrun.c
+++ b/Python/pythonrun.c
@@ -567,6 +567,7 @@ _PyRun_SimpleStringFlagsWithName(const char *command, const char* name, PyCompil
         PyObject* the_name = PyUnicode_FromString(name);
         if (!the_name) {
             PyErr_Print();
+            Py_DECREF(main_module);
             return -1;
         }
         res = _PyRun_StringFlagsWithName(command, the_name, Py_file_input, dict, dict, flags, 0);

It's a single line to fix the leak.

@bedevere-app
Copy link

bedevere-app bot commented Mar 24, 2026

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.

@bedevere-app
Copy link

bedevere-app bot commented Mar 24, 2026

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.

@A0su
Copy link
Author

A0su commented Mar 24, 2026

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Mar 24, 2026

Thanks for making the requested changes!

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

@bedevere-app bedevere-app bot requested a review from vstinner March 24, 2026 15:42
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.

2 participants