Issue 43779: Fix possible parser/AST ref leaks
Created on 2021-04-08 22:08 by erlendaasland, last changed 2022-04-11 14:59 by admin. 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) *  |
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) * |
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) *  |
Date: 2021-04-08 22:20 | |
Thanks Erlend. Mind creating a PR from the patch? |
|||
| msg390565 - (view) | Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-04-08 22:21 | |
Yes, I'll do that. |
|||
| msg390566 - (view) | Author: Erlend E. Aasland (erlendaasland) * |
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) * |
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) * |
Date: 2021-04-08 22:36 | |
+1 |
|||
| msg390653 - (view) | Author: Pablo Galindo Salgado (pablogsal) * |
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 |
| 2022-04-11 14:59:44 | admin | set | github: 87945 |
| 2021-04-09 17:47:25 | pablogsal | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2021-04-09 17:46:40 | pablogsal | set | messages: + msg390653 |
| 2021-04-09 05:10:40 | erlendaasland | set | pull_requests: + pull_request24030 |
| 2021-04-08 22:36:14 | erlendaasland | set | messages: + msg390569 |
| 2021-04-08 22:33:58 | pablogsal | set | messages: + msg390567 |
| 2021-04-08 22:32:21 | erlendaasland | set | files:
+ parser.diff messages: + msg390566 |
| 2021-04-08 22:23:12 | erlendaasland | set | stage: patch review pull_requests: + pull_request24024 |
| 2021-04-08 22:21:27 | erlendaasland | set | messages: + msg390565 |
| 2021-04-08 22:20:48 | pablogsal | set | messages: + msg390564 |
| 2021-04-08 22:20:45 | erlendaasland | set | messages: + msg390563 |
| 2021-04-08 22:10:55 | erlendaasland | set | title: Fix possible _PyArena_AddPyObject ref leaks -> Fix possible parser/AST ref leaks |
| 2021-04-08 22:08:01 | erlendaasland | create | |