Missing error / null pointer checks in `Parser/action_helpers.c`

[KowalskiThomas]

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_context does asdl_seq_SET(new_seq, i, _PyPegen_set_expr_context(p, e, ctx)); here
• _PyPegen_set_expr_context calls one of many functions, e.g. _set_name_context here
• _set_name_context calls into _PyAST_Name
• ... which may return NULL in 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

[picnixz]

_set_name_context is safe though here? we could add a bunch of asserts, but I don't think it makes sense to have eager checks.

[KowalskiThomas]

So _set_name_context calls _PyAST_Name, which can "fail" (return NULL) in the following cases:

1. id is falsy (outcome: PyErr_SetString + return NULL)
2. ctx is falsy (outcome: PyErr_SetString + return NULL)
3. _PyArena_Malloc(arena, sizeof(*p)) returns NULL (after calling PyErr_NoMemory) in which case NULL is 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_context because the parameters passed are known to be "correct".
I don't think anything guarantees 3. will not happen, though. In that case, the NULL will be bubbled up all the way to the asdl_seq_SET(new_seq, i, _PyPegen_set_expr_context(p, e, ctx));

[picnixz]

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.