Skip to content

Potential NULL dereference of kw_defaults in has_kwonlydefaults #135302

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
Closed
rialbat opened this issue Jun 9, 2025 · 5 comments
Closed

Potential NULL dereference of kw_defaults in has_kwonlydefaults #135302

rialbat opened this issue Jun 9, 2025 · 5 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided

Comments

@rialbat
Copy link
Contributor

rialbat commented Jun 9, 2025 •
edited by bedevere-app bot
Loading

The pointer s->v.AsyncFunctionDef.args->kw_defaults is explicitly checked for NULL:

cpython/Python/symtable.c

Lines 2193 to 2195 in aaad2e8

if (s->v.AsyncFunctionDef.args->kw_defaults)
VISIT_SEQ_WITH_NULL(st, expr,
s->v.AsyncFunctionDef.args->kw_defaults);

However, a few lines later, the same pointer is passed unconditionally to the has_kwonlydefaults function:

cpython/Python/symtable.c

Lines 2203 to 2204 in aaad2e8

has_kwonlydefaults(s->v.AsyncFunctionDef.args->kwonlyargs,
s->v.AsyncFunctionDef.args->kw_defaults),

Inside has_kwonlydefaults, the kw_defaults parameter is not checked for NULL before being dereferenced:

cpython/Python/symtable.c

Lines 1783 to 1787 in aaad2e8

for (int i = 0; i < asdl_seq_LEN(kwonlyargs); i++) {
expr_ty default_ = asdl_seq_GET(kw_defaults, i);
if (default_) {
return 1;
}

Linked PRs
rialbat added a commit to rialbat/cpython that referenced this issue Jun 9, 2025
@rialbat
pythongh-135302: Fix NULL pointer dereference in has_kwonlydefaults f…
6bfa6ad 
…unction
@bedevere-app bedevere-app bot mentioned this issue Jun 9, 2025
Closed
@JelleZijlstra
Copy link
Member

This isn't necessary because we access kw_defaults only if kwonlyargs is nonempty.

@picnixz picnixz added interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided labels Jun 9, 2025
@picnixz
Copy link
Member

picnixz commented Jun 9, 2025 •
edited
Loading

@JelleZijlstra I suggested adding an assertion #135303 (comment) but I don't know if you wouldn't prefer a PR that adds assertions also elsewhere rather than just modifiying this single place.

@JelleZijlstra
Copy link
Member

I don't really have a preference. The code is fine as is; an assertion would be harmless but not help much. Feel free to merge a PR adding assertions if you feel it's useful.

@picnixz
Copy link
Member

picnixz commented Jun 9, 2025

I think it's only useful if there is a chance to call the function by mistake. If it's not the case, then let's keep the code as is (I don't know if there are other places that would benefit having an assertion)

@rialbat
Copy link
Contributor Author

rialbat commented Jun 10, 2025

This isn't necessary because we access kw_defaults only if kwonlyargs is nonempty.

Got it. Thanks for your answer. I'll close the issue and PR.

@rialbat rialbat closed this as completed Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@JelleZijlstra @picnixz @rialbat