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 7 commits into
Conversation
Sorry, something went wrong.
…s-155733 # Conflicts: # Modules/_functoolsmodule.c
|
I think there are some cases that you are missing (like __setstate__). Could you add what is in gh-145960 but not in this PR? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
| @@ -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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Applied, thanks!
Sorry, something went wrong.
| * 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?
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Updated the variable name to valid_kwargs, thanks!
Sorry, something went wrong.
| /* 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?
Sorry, something went wrong.
There was a problem hiding this comment.
Restored, thanks for catching that!
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Makes sense. Should I remove the explicit check from Lib/functools.py and rely on the standard unpacking error?
Sorry, something went wrong.
gh-155733: Validate keyword argument keys in operator.methodcaller and functools.partial
Summary
Fixes gh-155733.
Previously, passing non-string keys as **kwargs into operator.methodcaller or functools.partial bypassed kwarg type validation, leading to vectorcall assertion failures (PyUnicode_Check) or segmentation faults in CPython.
This PR adds keyword type checks (PyUnicode_Check) to methodcaller_new, partial_new, and partial_vectorcall, raising TypeError: keywords must be strings cleanly.