Repository navigation
Missing error / null pointer checks in Parser/action_helpers.c #149689
Description
I'm opening this issue to discuss something I've noticed in Parser/action_helpers.c. There are multiple places where functions are called and their result is not checked, meaning if they error, Python will continue to try and execute its logic even though it would only be reasonable to bail. I initially thought those call sites skipped the return code / null pointer checks because they were redundant (i.e. that there was some assumption that some function calls can never fail) but upon further scrutiny that seems to be the case for none of them.
To take one random example:
_set_seq_contextdoesasdl_seq_SET(new_seq, i, _PyPegen_set_expr_context(p, e, ctx));here_PyPegen_set_expr_contextcalls one of many functions, e.g._set_name_contexthere_set_name_contextcalls into_PyAST_Name- ... which may return
NULLin several cases
The other cases are similar.
I have a fix branch here which also serves as a list of all the places in this specific file that would be worth fixing.
Let me know what you think.
Linked PRs
- gh-149689: add missing error checks in Parser/action_helpers.c #149710
- [3.15] gh-149689: add missing error checks in Parser/action_helpers.c (GH-149710) #152640
- [3.14] gh-149689: add missing error checks in Parser/action_helpers.c (GH-149710) #152641
- [3.13] gh-149689: add missing error checks in
Parser/action_helpers.c#153763
Activity
picnixz commentedon May 11, 2026
_set_name_contextis safe though here? we could add a bunch of asserts, but I don't think it makes sense to have eager checks.KowalskiThomas commentedon May 11, 2026
So
_set_name_contextcalls_PyAST_Name, which can "fail" (returnNULL) in the following cases:idis falsy (outcome:PyErr_SetString+return NULL)ctxis falsy (outcome:PyErr_SetString+return NULL)_PyArena_Malloc(arena, sizeof(*p))returnsNULL(after callingPyErr_NoMemory) in which caseNULLis returned directly by_set_name_context.If I understand correctly what your comment refers to is that we know 1. and 2. will not happen in the context in which we call
_set_name_contextbecause the parameters passed are known to be "correct".I don't think anything guarantees 3. will not happen, though. In that case, the
NULLwill be bubbled up all the way to theasdl_seq_SET(new_seq, i, _PyPegen_set_expr_context(p, e, ctx));picnixz commentedon May 12, 2026
Sorry, I overlooked that allocation so I guess we need to add checks. I did add checks myself and it would be better to keep them.
gh-149689: add missing error checks in Parser/action_helpers.c (#149710)
[3.14] gh-149689: add missing error checks in Parser/action_helpers.c (…
[3.15] gh-149689: add missing error checks in Parser/action_helpers.c (…
[3.13] gh-149689: add missing error checks in `Parser/action_helpers.…