Skip to content

bpo-1635741: _ast uses PyModule_AddObjectRef()#23146

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
Merged
vstinner merged 1 commit into from
Nov 4, 2020
Merged

bpo-1635741: _ast uses PyModule_AddObjectRef() #23146

vstinner merged 1 commit into from
Nov 4, 2020

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commented Nov 4, 2020•
edited by bedevere-bot
Loading

Replace PyModule_AddObject() with PyModule_AddObjectRef() in the _ast
module (Python-ast.c) to fix a reference leak on error.

https://bugs.python.org/issue1635741

@vstinner
bpo-1635741: _ast uses PyModule_AddObjectRef()
f001865
Replace PyModule_AddObject() with PyModule_AddObjectRef() in the _ast module (Python-ast.c) to fix a reference leak on error.
@pablogsal
Copy link
Member

pablogsal commented Nov 4, 2020

Are you sure this is a reference leak? PyModule_AddObject steals a reference so there is no extra incref that has happened that is not decreased later. What this would fix if I understand correctly is a possible crash due to the refcount being less than needed.

@vstinnervstinner merged commit 18ce7f1 into python:masterNov 4, 2020
@vstinner
Copy link
MemberAuthor

vstinner commented Nov 4, 2020

Are you sure this is a reference leak? PyModule_AddObject steals a reference so there is no extra incref that has happened that is not decreased later. What this would fix if I understand correctly is a possible crash due to the refcount being less than needed.

I was thinking about the error path, but you're right: my change doesn't fix any leak. I was confused by other code that I modified locally which have a bug (I didn't create PRs for them yet).

I removed the mention about a leak in the commit message and I merged my PR.

I prefer to use PyModule_AddObjectRef() because it looks really weird to be to decrement the refcount to immediately increment it. It would be dangerous if it would be a borrowed reference with a reference count of 1! (but it's not the case here)

@vstinnervstinner deleted the mod_addobjref_ast branch November 4, 2020 15:39
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Nov 5, 2020
@shihai1991
Merge branch 'master' into bpo_41832
c267e35
* master: bpo-42260: Add _PyInterpreterState_SetConfig() (pythonGH-23158) Disable peg generator tests when building with PGO (pythonGH-23141) bpo-1635741: _sqlite3 uses PyModule_AddObjectRef() (pythonGH-23148) bpo-1635741: Fix PyInit_pyexpat() error handling (pythonGH-22489) bpo-42260: Main init modify sys.flags in-place (pythonGH-23150) bpo-1635741: Fix ref leak in _PyWarnings_Init() error path (pythonGH-23151) bpo-1635741: _ast uses PyModule_AddObjectRef() (pythonGH-23146) bpo-1635741: _contextvars uses PyModule_AddType() (pythonGH-23147) bpo-42260: Reorganize PyConfig (pythonGH-23149) bpo-1635741: Add PyModule_AddObjectRef() function (pythonGH-23122) bpo-42236: os.device_encoding() respects UTF-8 Mode (pythonGH-23119) bpo-42251: Add gettrace and getprofile to threading (pythonGH-23125) Enable signing of nuget.org packages and update to supported timestamp server (pythonGH-23132) Fix incorrect links in ast docs (pythonGH-23017) Add _PyType_GetModuleByDef (pythonGH-22835) Post 3.10.0a2 bpo-41796: Call _PyAST_Fini() earlier to fix a leak (pythonGH-23131) bpo-42249: Fix writing binary Plist files larger than 4 GiB. (pythonGH-23121) bpo-40077: Convert mmap.mmap static type to a heap type (pythonGH-23108) Python 3.10.0a2
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@vstinner@adorilson
bpo-1635741: _ast uses PyModule_AddObjectRef() (pythonGH-23146)
d804003
Replace PyModule_AddObject() with PyModule_AddObjectRef() in the _ast module (Python-ast.c).
@kylotankylotanmannequin mentioned this pull request Sep 19, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lysnikolaoulysnikolaouAwaiting requested review from lysnikolaou

@pablogsalpablogsalAwaiting requested review from pablogsal

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner@pablogsal@the-knights-who-say-ni@bedevere-bot