gh-155733: Validate keyword argument keys in operator.methodcaller and functools.partial - #155779
gh-155733: Validate keyword argument keys in operator.methodcaller and functools.partial#155779pranavchoudhary-tech wants to merge 5 commits into
Conversation
…s-155733 # Conflicts: # Modules/_functoolsmodule.c
|
I think there are some cases that you are missing (like |
|
@brijkapadia Good point. I've added string key validation to partial_setstate in C and setstate in functools.py, updated test_non_string_keywords to cover setstate, and rebased with main. |
| @@ -0,0 +1 @@ | |||
| Validate keyword argument keys in ``operator.methodcaller`` and ``functools.partial`` to raise ``TypeError`` instead of crashing when non-string keys are passed. | |||
There was a problem hiding this comment.
Small nitpick:
| Validate keyword argument keys in ``operator.methodcaller`` and ``functools.partial`` to raise ``TypeError`` instead of crashing when non-string keys are passed. | |
| Validate keyword arguments to :func:`operator.methodcaller` and :func:`functools.partial` to raise a :exc:`TypeError` instead of crashing with non-string keys. |
| * Note, tail is already coppied. */ | ||
| Py_ssize_t pos = 0, i = 0; | ||
| PyObject *keyword_dict = n_merges ? pto_kw_merged : partial_keywords; | ||
| int valid_keys = 1; |
There was a problem hiding this comment.
You could also set valid_keys = 0 here instead (and update other occurrences to valid_keys). It doesn't really matter, but this is more consistent with other places. Maybe rename to something like error too?
There was a problem hiding this comment.
I don't understand why we should do it. It's more likely to have valid keys than not so it's less likely to hit error paths. As for renaming to error it's too broad. However, I would rename it to valid_kwargs
| /* Copy pto_keywords with overlapping call keywords merged | ||
| * Note, tail is already coppied. */ |
There was a problem hiding this comment.
Is there a reason why we can remove this comment?
| if kwds is not None and any(not isinstance(k, str) for k in kwds): | ||
| raise TypeError("keywords must be strings") |
There was a problem hiding this comment.
Not sure if you really need this check. You'll get the error when calling the function anyway. For instance, we don't check that Placeholder isn't passed as a keyword value either.
3 participants
gh-155733: Validate keyword argument keys in
operator.methodcallerandfunctools.partialSummary
Fixes gh-155733.
Previously, passing non-string keys as
**kwargsintooperator.methodcallerorfunctools.partialbypassed kwarg type validation, leading to vectorcall assertion failures (PyUnicode_Check) or segmentation faults in CPython.This PR adds keyword type checks (
PyUnicode_Check) tomethodcaller_new,partial_new, andpartial_vectorcall, raisingTypeError: keywords must be stringscleanly.