From 2b370bb5802ad56a5b489a1ec4c4fa5628b75424 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 19 Jul 2022 09:41:59 +0300 Subject: [PATCH] gh-94938: Fix errror detection of unexpected keyword arguments When keyword argument name is an instance of a str subclass with overloaded methods __eq__ and __hash__, the former code could not find the name of an extraneous keyword argument to report an error, and _PyArg_UnpackKeywords() returned success without setting the corresponding cell in linearized arguments array. But since the number of expected initialized cells is determined as total number of passed arguments, this lead to reading NULL as keyword parameter value, that caused SysTemError or crash or other undesired behavior. --- Lib/test/test_call.py | 25 +++ Lib/test/test_getargs2.py | 27 ++++ ...2-07-19-09-41-55.gh-issue-94938.xYBlM7.rst | 3 + Python/getargs.c | 142 +++++++----------- 4 files changed, 112 insertions(+), 85 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-07-19-09-41-55.gh-issue-94938.xYBlM7.rst diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index 07355e8fa0616c..a2eec419ab5b41 100644 --- a/Lib/test/test_call.py +++ b/Lib/test/test_call.py @@ -11,6 +11,19 @@ import contextlib +class BadStr(str): + def __eq__(self, other): + return True + def __hash__(self): + # Guaranteed different hash + return str.__hash__(self) ^ 3 + + def __eq__(self, other): + return False + def __hash__(self): + return str.__hash__(self) + + class FunctionCalls(unittest.TestCase): def test_kwargs_order(self): @@ -145,6 +158,18 @@ def test_varargs17_kw(self): self.assertRaisesRegex(TypeError, msg, print, 0, sep=1, end=2, file=3, flush=4, foo=5) + def test_varargs18_kw(self): + # _PyArg_UnpackKeywordsWithVararg() + msg = r"invalid keyword argument for print\(\)$" + with self.assertRaisesRegex(TypeError, msg): + print(0, 1, **{BadStr('foo'): ','}) + + def test_varargs19_kw(self): + # _PyArg_UnpackKeywords() + msg = r"invalid keyword argument for round\(\)$" + with self.assertRaisesRegex(TypeError, msg): + round(1.75, **{BadStr('foo'): 1}) + def test_oldargs0_1(self): msg = r"keys\(\) takes no arguments \(1 given\)" self.assertRaisesRegex(TypeError, msg, {}.keys, 0) diff --git a/Lib/test/test_getargs2.py b/Lib/test/test_getargs2.py index 7c11c6cd33a84d..d8d0b216523bcc 100644 --- a/Lib/test/test_getargs2.py +++ b/Lib/test/test_getargs2.py @@ -746,6 +746,33 @@ def test_surrogate_keyword(self): "'\udc80' is an invalid keyword argument for this function"): getargs_keyword_only(1, 2, **{'\uDC80': 10}) + def test_weird_str_subclass(self): + class BadStr(str): + def __eq__(self, other): + return True + def __hash__(self): + # Guaranteed different hash + return str.__hash__(self) ^ 3 + with self.assertRaisesRegex(TypeError, + "invalid keyword argument for this function"): + getargs_keyword_only(1, 2, **{BadStr("keyword_only"): 3}) + with self.assertRaisesRegex(TypeError, + "invalid keyword argument for this function"): + getargs_keyword_only(1, 2, **{BadStr("monster"): 666}) + + def test_weird_str_subclass2(self): + class BadStr(str): + def __eq__(self, other): + return False + def __hash__(self): + return str.__hash__(self) + with self.assertRaisesRegex(TypeError, + "invalid keyword argument for this function"): + getargs_keyword_only(1, 2, **{BadStr("keyword_only"): 3}) + with self.assertRaisesRegex(TypeError, + "invalid keyword argument for this function"): + getargs_keyword_only(1, 2, **{BadStr("monster"): 666}) + class PositionalOnlyAndKeywords_TestCase(unittest.TestCase): from _testcapi import getargs_positional_only_and_keywords as getargs diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-07-19-09-41-55.gh-issue-94938.xYBlM7.rst b/Misc/NEWS.d/next/Core and Builtins/2022-07-19-09-41-55.gh-issue-94938.xYBlM7.rst new file mode 100644 index 00000000000000..cc4feae685f23c --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-07-19-09-41-55.gh-issue-94938.xYBlM7.rst @@ -0,0 +1,3 @@ +Fix error detection in some builtin functions when keyword argument name is +an instance of a str subclass with overloaded ``__eq__`` and ``__hash__``. +Previously it could cause SystemError or other undesired behavior. diff --git a/Python/getargs.c b/Python/getargs.c index fb4a5124beab8a..89aaf7f09dd16f 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -1502,6 +1502,50 @@ _PyArg_VaParseTupleAndKeywordsFast_SizeT(PyObject *args, PyObject *keywords, return retval; } +static void +error_unexpected_keyword_arg(PyObject *kwargs, PyObject *kwnames, PyObject *kwtuple, const char *fname) +{ + /* make sure there are no extraneous keyword arguments */ + Py_ssize_t j = 0; + while (1) { + PyObject *keyword; + if (kwargs != NULL) { + if (!PyDict_Next(kwargs, &j, &keyword, NULL)) + break; + } + else { + if (j >= PyTuple_GET_SIZE(kwnames)) + break; + keyword = PyTuple_GET_ITEM(kwnames, j); + j++; + } + if (!PyUnicode_Check(keyword)) { + PyErr_SetString(PyExc_TypeError, + "keywords must be strings"); + return; + } + + int match = PySequence_Contains(kwtuple, keyword); + if (match <= 0) { + if (!match) { + PyErr_Format(PyExc_TypeError, + "'%S' is an invalid keyword " + "argument for %.200s%s", + keyword, + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()"); + } + return; + } + } + /* Something wrong happened. There are extraneous keyword arguments, + * but we don't know what. And we don't bother. */ + PyErr_Format(PyExc_TypeError, + "invalid keyword argument for %.200s%s", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()"); +} + int PyArg_ValidateKeywordArguments(PyObject *kwargs) { @@ -1790,6 +1834,13 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format, return cleanreturn(0, &freelist); } } + /* Something wrong happened. There are extraneous keyword arguments, + * but we don't know what. And we don't bother. */ + PyErr_Format(PyExc_TypeError, + "invalid keyword argument for %.200s%s", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()"); + return cleanreturn(0, &freelist); } return cleanreturn(1, &freelist); @@ -2132,7 +2183,6 @@ vgetargskeywordsfast_impl(PyObject *const *args, Py_ssize_t nargs, assert(IS_END_OF_FORMAT(*format) || (*format == '|') || (*format == '$')); if (nkwargs > 0) { - Py_ssize_t j; /* make sure there are no arguments given by name and position */ for (i = pos; i < nargs; i++) { keyword = PyTuple_GET_ITEM(kwtuple, i - pos); @@ -2156,34 +2206,9 @@ vgetargskeywordsfast_impl(PyObject *const *args, Py_ssize_t nargs, return cleanreturn(0, &freelist); } } - /* make sure there are no extraneous keyword arguments */ - j = 0; - while (1) { - int match; - if (kwargs != NULL) { - if (!PyDict_Next(kwargs, &j, &keyword, NULL)) - break; - } - else { - if (j >= PyTuple_GET_SIZE(kwnames)) - break; - keyword = PyTuple_GET_ITEM(kwnames, j); - j++; - } - match = PySequence_Contains(kwtuple, keyword); - if (match <= 0) { - if (!match) { - PyErr_Format(PyExc_TypeError, - "'%S' is an invalid keyword " - "argument for %.200s%s", - keyword, - (parser->fname == NULL) ? "this function" : parser->fname, - (parser->fname == NULL) ? "" : "()"); - } - return cleanreturn(0, &freelist); - } - } + error_unexpected_keyword_arg(kwargs, kwnames, kwtuple, parser->fname); + return cleanreturn(0, &freelist); } return cleanreturn(1, &freelist); @@ -2357,7 +2382,6 @@ _PyArg_UnpackKeywords(PyObject *const *args, Py_ssize_t nargs, } if (nkwargs > 0) { - Py_ssize_t j; /* make sure there are no arguments given by name and position */ for (i = posonly; i < nargs; i++) { keyword = PyTuple_GET_ITEM(kwtuple, i - posonly); @@ -2381,34 +2405,9 @@ _PyArg_UnpackKeywords(PyObject *const *args, Py_ssize_t nargs, return NULL; } } - /* make sure there are no extraneous keyword arguments */ - j = 0; - while (1) { - int match; - if (kwargs != NULL) { - if (!PyDict_Next(kwargs, &j, &keyword, NULL)) - break; - } - else { - if (j >= PyTuple_GET_SIZE(kwnames)) - break; - keyword = PyTuple_GET_ITEM(kwnames, j); - j++; - } - match = PySequence_Contains(kwtuple, keyword); - if (match <= 0) { - if (!match) { - PyErr_Format(PyExc_TypeError, - "'%S' is an invalid keyword " - "argument for %.200s%s", - keyword, - (parser->fname == NULL) ? "this function" : parser->fname, - (parser->fname == NULL) ? "" : "()"); - } - return NULL; - } - } + error_unexpected_keyword_arg(kwargs, kwnames, kwtuple, parser->fname); + return NULL; } return buf; @@ -2537,35 +2536,8 @@ _PyArg_UnpackKeywordsWithVararg(PyObject *const *args, Py_ssize_t nargs, } if (nkwargs > 0) { - Py_ssize_t j; - /* make sure there are no extraneous keyword arguments */ - j = 0; - while (1) { - int match; - if (kwargs != NULL) { - if (!PyDict_Next(kwargs, &j, &keyword, NULL)) - break; - } - else { - if (j >= PyTuple_GET_SIZE(kwnames)) - break; - keyword = PyTuple_GET_ITEM(kwnames, j); - j++; - } - - match = PySequence_Contains(kwtuple, keyword); - if (match <= 0) { - if (!match) { - PyErr_Format(PyExc_TypeError, - "'%S' is an invalid keyword " - "argument for %.200s%s", - keyword, - (parser->fname == NULL) ? "this function" : parser->fname, - (parser->fname == NULL) ? "" : "()"); - } - goto exit; - } - } + error_unexpected_keyword_arg(kwargs, kwnames, kwtuple, parser->fname); + goto exit; } return buf;