From 5a275c83b04d05115a232c0828024e1bb2fdedab Mon Sep 17 00:00:00 2001 From: Joe Jevnik Date: Mon, 9 Jul 2018 12:02:12 -0400 Subject: [PATCH 1/9] Fix handling for removed PyArg_ParseTuple formatters --- Lib/test/test_capi.py | 3 +- .../2018-07-09-12-00-21.bpo-23926.W7R_lI.rst | 2 + Modules/_testcapimodule.c | 71 +++++++++++++++++++ Python/getargs.c | 9 ++- 4 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 2a6de3c5aa9780..6f075cb9ac0c13 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -458,7 +458,8 @@ def test_skipitem(self): # skip parentheses, the error reporting is inconsistent about them # skip 'e', it's always a two-character code # skip '|' and '$', they don't represent arguments anyway - if c in '()e|$': + # skip 'w' because it is invalid without a suffix. + if c in 'w()e|$': continue # test the format unit when not skipped diff --git a/Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst b/Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst new file mode 100644 index 00000000000000..fa09d2a0a72df7 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst @@ -0,0 +1,2 @@ +Fixed skipitem()'s handling of the old 'w' and 'w#' formatters. These are +no longer supported and now raise an exception if used. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 014c2f325af3e8..2b9ad26f8c4180 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -1437,6 +1437,77 @@ getargs_Z(PyObject *self, PyObject *args) Py_RETURN_NONE; } +/* Test the old w and w# codes that no longer work */ +static PyObject * +test_w_code_invalid(PyObject *self) +{ + static const char * const keywords[] = {"a", "b", "c", "d", NULL}; + char *formats_3[] = {"O|w#$O", + "O|w$O", + "O|w#O", + "O|wO", + NULL}; + char *formats_4[] = {"O|w#O$O", + "O|wO$O", + "O|Ow#O", + "O|OwO", + "O|Ow#$O", + "O|Ow$O", + NULL}; + size_t n; + PyObject *args; + PyObject *kwargs; + PyObject *tmp; + + if (!(args = PyTuple_Pack(1, Py_None))) { + return NULL; + } + + if (!(kwargs = PyDict_New()) || PyDict_SetItemString(kwargs, "c", Py_None)) { + Py_DECREF(args); + Py_XDECREF(kwargs); + return NULL; + } + + for (n = 0;formats_3[n];++n) { + if (PyArg_ParseTupleAndKeywords(args, kwargs, formats_3[n], + (char**) keywords, + &tmp, &tmp, &tmp)) { + Py_DECREF(args); + return raiseTestError("test_w_code_invalid_suffix", + formats_3[n]); + } + else { + PyErr_Clear(); + } + } + + if (PyDict_DelItemString(kwargs, "c") || + PyDict_SetItemString(kwargs, "d", Py_None)) { + + Py_DECREF(kwargs); + Py_DECREF(args); + return NULL; + } + + for (n = 0;formats_4[n];++n) { + if (PyArg_ParseTupleAndKeywords(args, kwargs, formats_4[n], + (char**) keywords, + &tmp, &tmp, &tmp, &tmp)) { + Py_DECREF(args); + return raiseTestError("test_w_code_invalid_suffix", + formats_4[n]); + } + else { + PyErr_Clear(); + } + } + + Py_DECREF(args); + Py_DECREF(kwargs); + Py_RETURN_NONE; +} + static PyObject * getargs_Z_hash(PyObject *self, PyObject *args) { diff --git a/Python/getargs.c b/Python/getargs.c index 97c1fe8f4c9da9..bb0475cdd584f7 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -2303,6 +2303,14 @@ skipitem(const char **p_format, va_list *p_va, int flags) /* string codes */ + case 'w': /* buffer, read-write */ + if (*format != '*') { + /* after 'w', only '*' is allowed */ + goto err; + } + format++; + break; + case 'e': /* string with encoding */ { if (p_va != NULL) { @@ -2320,7 +2328,6 @@ skipitem(const char **p_format, va_list *p_va, int flags) case 'y': /* bytes */ case 'u': /* unicode string */ case 'Z': /* unicode string or None */ - case 'w': /* buffer, read-write */ { if (p_va != NULL) { (void) va_arg(*p_va, char **); From 65aec546c08c7de09f43a349ded670289f569f6c Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 17:30:33 +0100 Subject: [PATCH 2/9] Merge comments --- Lib/test/test_capi/test_getargs.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_capi/test_getargs.py b/Lib/test/test_capi/test_getargs.py index d51abed6289bce..115b964742ea69 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -1112,10 +1112,9 @@ def test_skipitem(self): c = chr(i) # skip parentheses, the error reporting is inconsistent about them - # skip 'e', it's always a two-character code + # skip 'e' and 'w', they're always two-character codes # skip '|' and '$', they don't represent arguments anyway - # skip 'w' because it is invalid without a suffix. - if c in 'w()e|$': + if c in '()ew|$': continue # test the format unit when not skipped From 6c948fbde332d29eefa8fbf7b0c09cab096bcd47 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 17:32:43 +0100 Subject: [PATCH 3/9] Recreate the blurb --- ...6.W7R_lI.rst => 2024-03-18-17-29-52.gh-issue-68114.W7R_lI.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Misc/NEWS.d/next/C API/{2018-07-09-12-00-21.bpo-23926.W7R_lI.rst => 2024-03-18-17-29-52.gh-issue-68114.W7R_lI.rst} (100%) diff --git a/Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst b/Misc/NEWS.d/next/C API/2024-03-18-17-29-52.gh-issue-68114.W7R_lI.rst similarity index 100% rename from Misc/NEWS.d/next/C API/2018-07-09-12-00-21.bpo-23926.W7R_lI.rst rename to Misc/NEWS.d/next/C API/2024-03-18-17-29-52.gh-issue-68114.W7R_lI.rst From c2a63a2beeb25dcd3ad17eadfa8aeb11c463f258 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 17:36:59 +0100 Subject: [PATCH 4/9] Fix the test - Don't use removed trivial helper raiseTestError - Decref kwargs if it's created - Export the test so that it is run --- Modules/_testcapi/getargs.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/Modules/_testcapi/getargs.c b/Modules/_testcapi/getargs.c index 91bc9a3fdf71c5..9bc8f7f1553117 100644 --- a/Modules/_testcapi/getargs.c +++ b/Modules/_testcapi/getargs.c @@ -143,7 +143,7 @@ getargs_w_star(PyObject *self, PyObject *args) /* Test the old w and w# codes that no longer work */ static PyObject * -test_w_code_invalid(PyObject *self) +test_w_code_invalid(PyObject *, PyObject *) { static const char * const keywords[] = {"a", "b", "c", "d", NULL}; char *formats_3[] = {"O|w#$O", @@ -167,7 +167,13 @@ test_w_code_invalid(PyObject *self) return NULL; } - if (!(kwargs = PyDict_New()) || PyDict_SetItemString(kwargs, "c", Py_None)) { + kwargs = PyDict_New(); + if (!kwargs) { + Py_DECREF(args); + return NULL; + } + + if (PyDict_SetItemString(kwargs, "c", Py_None)) { Py_DECREF(args); Py_XDECREF(kwargs); return NULL; @@ -178,8 +184,11 @@ test_w_code_invalid(PyObject *self) (char**) keywords, &tmp, &tmp, &tmp)) { Py_DECREF(args); - return raiseTestError("test_w_code_invalid_suffix", - formats_3[n]); + Py_DECREF(kwargs); + PyErr_Format(PyExc_AssertionError, + "test_w_code_invalid_suffix: %s", + formats_3[n]); + return NULL; } else { PyErr_Clear(); @@ -199,8 +208,11 @@ test_w_code_invalid(PyObject *self) (char**) keywords, &tmp, &tmp, &tmp, &tmp)) { Py_DECREF(args); - return raiseTestError("test_w_code_invalid_suffix", - formats_4[n]); + Py_DECREF(kwargs); + PyErr_Format(PyExc_AssertionError, + "test_w_code_invalid_suffix: %s", + formats_4[n]); + return NULL; } else { PyErr_Clear(); @@ -764,6 +776,7 @@ static PyMethodDef test_methods[] = { {"getargs_z_star", getargs_z_star, METH_VARARGS}, {"parse_tuple_and_keywords", parse_tuple_and_keywords, METH_VARARGS}, {"gh_99240_clear_args", gh_99240_clear_args, METH_VARARGS}, + {"test_w_code_invalid", test_w_code_invalid, METH_NOARGS}, {NULL}, }; From 4fbed1550642da4cdfe3575ca27d0234c63abcbb Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 17:40:20 +0100 Subject: [PATCH 5/9] Fix style --- Modules/_testcapi/getargs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_testcapi/getargs.c b/Modules/_testcapi/getargs.c index 9bc8f7f1553117..b5d77b8a7838bb 100644 --- a/Modules/_testcapi/getargs.c +++ b/Modules/_testcapi/getargs.c @@ -179,7 +179,7 @@ test_w_code_invalid(PyObject *, PyObject *) return NULL; } - for (n = 0;formats_3[n];++n) { + for (n = 0; formats_3[n]; ++n) { if (PyArg_ParseTupleAndKeywords(args, kwargs, formats_3[n], (char**) keywords, &tmp, &tmp, &tmp)) { @@ -203,7 +203,7 @@ test_w_code_invalid(PyObject *, PyObject *) return NULL; } - for (n = 0;formats_4[n];++n) { + for (n = 0; formats_4[n]; ++n) { if (PyArg_ParseTupleAndKeywords(args, kwargs, formats_4[n], (char**) keywords, &tmp, &tmp, &tmp, &tmp)) { From 3aff04966c5e0849b2c02e6a7bb90ca7c1c50a41 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 17:46:21 +0100 Subject: [PATCH 6/9] Ensure the invalid args result in SystemError --- Modules/_testcapi/getargs.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Modules/_testcapi/getargs.c b/Modules/_testcapi/getargs.c index b5d77b8a7838bb..c83c1e4cb3e4ad 100644 --- a/Modules/_testcapi/getargs.c +++ b/Modules/_testcapi/getargs.c @@ -191,6 +191,11 @@ test_w_code_invalid(PyObject *, PyObject *) return NULL; } else { + if (!PyErr_ExceptionMatches(PyExc_SystemError)) { + Py_DECREF(args); + Py_DECREF(kwargs); + return NULL; + } PyErr_Clear(); } } @@ -215,6 +220,11 @@ test_w_code_invalid(PyObject *, PyObject *) return NULL; } else { + if (!PyErr_ExceptionMatches(PyExc_SystemError)) { + Py_DECREF(args); + Py_DECREF(kwargs); + return NULL; + } PyErr_Clear(); } } From ae9b1e281134353752653d7b8f77fea58419aff9 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 18 Mar 2024 18:05:25 +0100 Subject: [PATCH 7/9] Fix functionality, and add additional test The original patch didn't call va_arg, resulting in corruption when 'w*' was used. --- Lib/test/test_capi/test_getargs.py | 32 +++++++++++++++++------------- Modules/_testcapi/getargs.c | 24 ++++++++++++++++++++++ Python/getargs.c | 14 ++++++------- 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_capi/test_getargs.py b/Lib/test/test_capi/test_getargs.py index 115b964742ea69..1e92eef05c3b02 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -856,20 +856,24 @@ def test_y_hash(self): def test_w_star(self): # getargs_w_star() modifies first and last byte - from _testcapi import getargs_w_star - self.assertRaises(TypeError, getargs_w_star, 'abc\xe9') - self.assertRaises(TypeError, getargs_w_star, b'bytes') - self.assertRaises(TypeError, getargs_w_star, b'nul:\0') - self.assertRaises(TypeError, getargs_w_star, memoryview(b'bytes')) - buf = bytearray(b'bytearray') - self.assertEqual(getargs_w_star(buf), b'[ytearra]') - self.assertEqual(buf, bytearray(b'[ytearra]')) - buf = bytearray(b'memoryview') - self.assertEqual(getargs_w_star(memoryview(buf)), b'[emoryvie]') - self.assertEqual(buf, bytearray(b'[emoryvie]')) - self.assertRaises(TypeError, getargs_w_star, None) - self.assertRaises(TypeError, getargs_w_star, NONCONTIG_WRITABLE) - self.assertRaises(TypeError, getargs_w_star, NONCONTIG_READONLY) + # getargs_w_star_opt() takes additional optional args: with one + # argument it should behave the same as getargs_w_star + from _testcapi import getargs_w_star, getargs_w_star_opt + for func in (getargs_w_star, getargs_w_star_opt): + with self.subTest(func=func): + self.assertRaises(TypeError, func, 'abc\xe9') + self.assertRaises(TypeError, func, b'bytes') + self.assertRaises(TypeError, func, b'nul:\0') + self.assertRaises(TypeError, func, memoryview(b'bytes')) + buf = bytearray(b'bytearray') + self.assertEqual(func(buf), b'[ytearra]') + self.assertEqual(buf, bytearray(b'[ytearra]')) + buf = bytearray(b'memoryview') + self.assertEqual(func(memoryview(buf)), b'[emoryvie]') + self.assertEqual(buf, bytearray(b'[emoryvie]')) + self.assertRaises(TypeError, func, None) + self.assertRaises(TypeError, func, NONCONTIG_WRITABLE) + self.assertRaises(TypeError, func, NONCONTIG_READONLY) def test_getargs_empty(self): from _testcapi import getargs_empty diff --git a/Modules/_testcapi/getargs.c b/Modules/_testcapi/getargs.c index c83c1e4cb3e4ad..ac63c5de2d9389 100644 --- a/Modules/_testcapi/getargs.c +++ b/Modules/_testcapi/getargs.c @@ -141,6 +141,29 @@ getargs_w_star(PyObject *self, PyObject *args) return result; } +static PyObject * +getargs_w_star_opt(PyObject *self, PyObject *args) +{ + Py_buffer buffer; + Py_buffer buf2; + int number = 1; + + if (!PyArg_ParseTuple(args, "w*|w*i:getargs_w_star", + &buffer, &buf2, &number)) { + return NULL; + } + + if (2 <= buffer.len) { + char *str = buffer.buf; + str[0] = '['; + str[buffer.len-1] = ']'; + } + + PyObject *result = PyBytes_FromStringAndSize(buffer.buf, buffer.len); + PyBuffer_Release(&buffer); + return result; +} + /* Test the old w and w# codes that no longer work */ static PyObject * test_w_code_invalid(PyObject *, PyObject *) @@ -777,6 +800,7 @@ static PyMethodDef test_methods[] = { {"getargs_s_star", getargs_s_star, METH_VARARGS}, {"getargs_tuple", getargs_tuple, METH_VARARGS}, {"getargs_w_star", getargs_w_star, METH_VARARGS}, + {"getargs_w_star_opt", getargs_w_star_opt, METH_VARARGS}, {"getargs_empty", _PyCFunction_CAST(getargs_empty), METH_VARARGS|METH_KEYWORDS}, {"getargs_y", getargs_y, METH_VARARGS}, {"getargs_y_hash", getargs_y_hash, METH_VARARGS}, diff --git a/Python/getargs.c b/Python/getargs.c index e271cd394934e1..539925e471f54c 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -2621,14 +2621,6 @@ skipitem(const char **p_format, va_list *p_va, int flags) /* string codes */ - case 'w': /* buffer, read-write */ - if (*format != '*') { - /* after 'w', only '*' is allowed */ - goto err; - } - format++; - break; - case 'e': /* string with encoding */ { if (p_va != NULL) { @@ -2644,10 +2636,16 @@ skipitem(const char **p_format, va_list *p_va, int flags) case 's': /* string */ case 'z': /* string or None */ case 'y': /* bytes */ + case 'w': /* buffer, read-write */ { if (p_va != NULL) { (void) va_arg(*p_va, char **); } + if (c == 'w' && *format != '*') + { + /* after 'w', only '*' is allowed */ + goto err; + } if (*format == '#') { if (p_va != NULL) { (void) va_arg(*p_va, Py_ssize_t *); From 30e7def3d84f35504afb222e58b7a7826409738a Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 19 Mar 2024 09:57:18 +0100 Subject: [PATCH 8/9] Unmark 'w#' as supported in test_skipitem_with_suffix --- Lib/test/test_capi/test_getargs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_capi/test_getargs.py b/Lib/test/test_capi/test_getargs.py index 1e92eef05c3b02..e710400f75c235 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -1156,7 +1156,7 @@ def test_skipitem_with_suffix(self): dict_b = {'b':1} keywords = ["a", "b"] - supported = ('s#', 's*', 'z#', 'z*', 'y#', 'y*', 'w#', 'w*') + supported = ('s#', 's*', 'z#', 'z*', 'y#', 'y*', 'w*') for c in string.ascii_letters: for c2 in '#*': f = c + c2 From 2aee71cf301dca3ecee9127f13fbf7c0a763a27a Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 19 Mar 2024 10:11:44 +0100 Subject: [PATCH 9/9] Don't omit parameter names --- Modules/_testcapi/getargs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_testcapi/getargs.c b/Modules/_testcapi/getargs.c index ac63c5de2d9389..ee04c760d27213 100644 --- a/Modules/_testcapi/getargs.c +++ b/Modules/_testcapi/getargs.c @@ -166,7 +166,7 @@ getargs_w_star_opt(PyObject *self, PyObject *args) /* Test the old w and w# codes that no longer work */ static PyObject * -test_w_code_invalid(PyObject *, PyObject *) +test_w_code_invalid(PyObject *self, PyObject *arg) { static const char * const keywords[] = {"a", "b", "c", "d", NULL}; char *formats_3[] = {"O|w#$O",