classification
Title: Fix possible parser/AST ref leaks
Type: behavior Stage: resolved
Components: Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: erlendaasland, pablogsal
Priority: normal Keywords: patch

Created on 2021-04-08 22:08 by erlendaasland, last changed 2021-04-09 17:47 by pablogsal. This issue is now closed.

Files
File name Uploaded Description Edit
patch.diff erlendaasland, 2021-04-08 22:08
parser.diff erlendaasland, 2021-04-08 22:32
Pull Requests
URL Status Linked Edit
PR 25289 merged erlendaasland, 2021-04-08 22:23
PR 25294 merged erlendaasland, 2021-04-09 05:10
Messages (8)
msg390559 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-08 22:08
_PyArena_AddPyObject() only steals the object reference if successful.

- In Parser/pegen.c, _PyPegen_fill_token(), the return value is not checked at all.
- In Parser/asdl_c.py, none of the (two) calls to _PyArena_AddPyObject() decref on failure
- Ditto for Python/Python-ast.c


Attached patch fixes all callers. I'll put up a PR if it looks ok.

Alternatively, one could make _PyArena_AddPyObject() _not_ steal the reference, but since it's an exposed API, that's probably not an option.
msg390563 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-08 22:20
Ah, I see that the Parser/asdl_c.py case is handled by the callers. So that leaves only Parser/pegen.c.
msg390564 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-08 22:20
Thanks Erlend. Mind creating a PR from the patch?
msg390565 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-08 22:21
Yes, I'll do that.
msg390566 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-08 22:32
We could also adjust Parser/asdl_c.py to decref right after a failed _PyArena_AddPyObject() call, instead of goto failure and Py_XDECREF. I'm not sure it's worth it though.
msg390567 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-08 22:33
> We could also adjust Parser/asdl_c.py to decref right after a failed _PyArena_AddPyObject() call, instead of goto failure and Py_XDECREF. I'm not sure it's worth it though.

I normally recommend having the least amount of exist paths possible, makes reasoning easily normally at the cost of some extra NULL checks for the XDECREF in failure scenarios
msg390569 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-08 22:36
+1
msg390653 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-09 17:46
New changeset 76d270ec2b776cc5331935cc58c2d63622f1c0e9 by Erlend Egeberg Aasland in branch '3.9':
[3.9] bpo-43779: Fix possible refleak involving _PyArena_AddPyObject (GH-25289). (GH-25294)
https://github.com/python/cpython/commit/76d270ec2b776cc5331935cc58c2d63622f1c0e9
History
Date User Action Args
2021-04-09 17:47:25pablogsalsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-04-09 17:46:40pablogsalsetmessages: + msg390653
2021-04-09 05:10:40erlendaaslandsetpull_requests: + pull_request24030
2021-04-08 22:36:14erlendaaslandsetmessages: + msg390569
2021-04-08 22:33:58pablogsalsetmessages: + msg390567
2021-04-08 22:32:21erlendaaslandsetfiles: + parser.diff

messages: + msg390566
2021-04-08 22:23:12erlendaaslandsetstage: patch review
pull_requests: + pull_request24024
2021-04-08 22:21:27erlendaaslandsetmessages: + msg390565
2021-04-08 22:20:48pablogsalsetmessages: + msg390564
2021-04-08 22:20:45erlendaaslandsetmessages: + msg390563
2021-04-08 22:10:55erlendaaslandsettitle: Fix possible _PyArena_AddPyObject ref leaks -> Fix possible parser/AST ref leaks
2021-04-08 22:08:01erlendaaslandcreate