From 03c905c1476d94c3d1ad7c5c032bfaba53af12a9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 16:06:05 -0700 Subject: [PATCH 1/6] Add failing regression tests --- Lib/test/test_bytes.py | 17 +++++++++++++++++ Modules/_testcapimodule.c | 15 +++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/Lib/test/test_bytes.py b/Lib/test/test_bytes.py index b457ff6ca849c5d..07249a33e4faa8b 100644 --- a/Lib/test/test_bytes.py +++ b/Lib/test/test_bytes.py @@ -1710,6 +1710,23 @@ def test_repeat_after_setslice(self): self.assertEqual(b1, b) self.assertEqual(b3, b'xcxcxc') + def test_mutating_index(self): + class Boom: + def __index__(self): + b.clear() + return 0 + + with self.subTest("tp_as_mapping"): + b = bytearray(b'Now you see me...') + with self.assertRaises(IndexError): + b[0] = Boom() + + with self.subTest("tp_as_sequence"): + _testcapi = import_helper.import_module('_testcapi') + b = bytearray(b'Now you see me...') + with self.assertRaises(IndexError): + _testcapi.sequence_setitem(b, 0, Boom()) + class AssortedBytesTest(unittest.TestCase): # diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 436c701a7a3faf2..3ebe7fa4a349761 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5484,6 +5484,20 @@ sequence_getitem(PyObject *self, PyObject *args) } +static PyObject * +sequence_setitem(PyObject *self, PyObject *args) +{ + Py_ssize_t i; + PyObject *seq, *val; + if (!PyArg_ParseTuple(args, "OnO", &seq, &i, &val)) { + return NULL; + } + if (PySequence_SetItem(seq, i, val)) { + return NULL; + } + Py_RETURN_NONE; +} + /* Functions for testing C calling conventions (METH_*) are named meth_*, * e.g. "meth_varargs" for METH_VARARGS. * @@ -6300,6 +6314,7 @@ static PyMethodDef TestMethods[] = { #endif {"write_unraisable_exc", test_write_unraisable_exc, METH_VARARGS}, {"sequence_getitem", sequence_getitem, METH_VARARGS}, + {"sequence_setitem", sequence_setitem, METH_VARARGS}, {"meth_varargs", meth_varargs, METH_VARARGS}, {"meth_varargs_keywords", _PyCFunction_CAST(meth_varargs_keywords), METH_VARARGS|METH_KEYWORDS}, {"meth_o", meth_o, METH_O}, From f98f23bdd80b6fb249533eb2da77797c9d932683 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 16:08:02 -0700 Subject: [PATCH 2/6] Get byte value before performing bounds checks --- Objects/bytearrayobject.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/Objects/bytearrayobject.c b/Objects/bytearrayobject.c index d0179626414874e..7d7c18a54f8a9f0 100644 --- a/Objects/bytearrayobject.c +++ b/Objects/bytearrayobject.c @@ -565,19 +565,24 @@ bytearray_setitem(PyByteArrayObject *self, Py_ssize_t i, PyObject *value) { int ival; - if (i < 0) + // GH-91153: We need to do this *before* the size check, in case value has a + // nasty __index__ method that changes the size of the bytearray: + if (value && !_getbytevalue(value, &ival)) { + return -1; + } + + if (i < 0) { i += Py_SIZE(self); + } if (i < 0 || i >= Py_SIZE(self)) { PyErr_SetString(PyExc_IndexError, "bytearray index out of range"); return -1; } - if (value == NULL) + if (value == NULL) { return bytearray_setslice(self, i, i+1, NULL); - - if (!_getbytevalue(value, &ival)) - return -1; + } PyByteArray_AS_STRING(self)[i] = ival; return 0; @@ -593,11 +598,21 @@ bytearray_ass_subscript(PyByteArrayObject *self, PyObject *index, PyObject *valu if (_PyIndex_Check(index)) { Py_ssize_t i = PyNumber_AsSsize_t(index, PyExc_IndexError); - if (i == -1 && PyErr_Occurred()) + if (i == -1 && PyErr_Occurred()) { return -1; + } - if (i < 0) + int ival; + + // GH-91153: We need to do this *before* the size check, in case values + // has a nasty __index__ method that changes the size of the bytearray: + if (values && !_getbytevalue(values, &ival)) { + return -1; + } + + if (i < 0) { i += PyByteArray_GET_SIZE(self); + } if (i < 0 || i >= Py_SIZE(self)) { PyErr_SetString(PyExc_IndexError, "bytearray index out of range"); @@ -612,9 +627,6 @@ bytearray_ass_subscript(PyByteArrayObject *self, PyObject *index, PyObject *valu slicelen = 1; } else { - int ival; - if (!_getbytevalue(values, &ival)) - return -1; buf[i] = (char)ival; return 0; } From 94fb01005911ea7f7a5edb613eb8ca48d5680e14 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 16:08:30 -0700 Subject: [PATCH 3/6] make patchcheck --- Lib/test/test_bytes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_bytes.py b/Lib/test/test_bytes.py index 07249a33e4faa8b..521e391b8d5f9db 100644 --- a/Lib/test/test_bytes.py +++ b/Lib/test/test_bytes.py @@ -1720,13 +1720,13 @@ def __index__(self): b = bytearray(b'Now you see me...') with self.assertRaises(IndexError): b[0] = Boom() - + with self.subTest("tp_as_sequence"): _testcapi = import_helper.import_module('_testcapi') b = bytearray(b'Now you see me...') with self.assertRaises(IndexError): _testcapi.sequence_setitem(b, 0, Boom()) - + class AssortedBytesTest(unittest.TestCase): # From f0af2c5f767e5a0122cf7a45cbe2dabd9ae6c4bf Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 16:15:32 -0700 Subject: [PATCH 4/6] blurb add --- .../2022-07-15-16-15-04.gh-issue-91153.HiBmtt.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-07-15-16-15-04.gh-issue-91153.HiBmtt.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-07-15-16-15-04.gh-issue-91153.HiBmtt.rst b/Misc/NEWS.d/next/Core and Builtins/2022-07-15-16-15-04.gh-issue-91153.HiBmtt.rst new file mode 100644 index 000000000000000..2caa0170f75bcaf --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-07-15-16-15-04.gh-issue-91153.HiBmtt.rst @@ -0,0 +1,2 @@ +Fix an issue where a :class:`bytearray` item assignment could crash if it's +resized by the new value's :meth:`__index__` method. From 2c494e7df96e054637dc1897d43f170df5d46f85 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 16:21:34 -0700 Subject: [PATCH 5/6] Fixup --- Modules/_testcapimodule.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 3ebe7fa4a349761..3a73941845e3227 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5498,6 +5498,7 @@ sequence_setitem(PyObject *self, PyObject *args) Py_RETURN_NONE; } + /* Functions for testing C calling conventions (METH_*) are named meth_*, * e.g. "meth_varargs" for METH_VARARGS. * From b346277c7aad1c2b9841fec622decc52dd7d680d Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 15 Jul 2022 22:31:16 -0700 Subject: [PATCH 6/6] Fix compiler warnings. --- Objects/bytearrayobject.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Objects/bytearrayobject.c b/Objects/bytearrayobject.c index 7d7c18a54f8a9f0..b2962fd137d93ef 100644 --- a/Objects/bytearrayobject.c +++ b/Objects/bytearrayobject.c @@ -563,7 +563,7 @@ bytearray_setslice(PyByteArrayObject *self, Py_ssize_t lo, Py_ssize_t hi, static int bytearray_setitem(PyByteArrayObject *self, Py_ssize_t i, PyObject *value) { - int ival; + int ival = -1; // GH-91153: We need to do this *before* the size check, in case value has a // nasty __index__ method that changes the size of the bytearray: @@ -584,6 +584,7 @@ bytearray_setitem(PyByteArrayObject *self, Py_ssize_t i, PyObject *value) return bytearray_setslice(self, i, i+1, NULL); } + assert(0 <= ival && ival < 256); PyByteArray_AS_STRING(self)[i] = ival; return 0; } @@ -602,7 +603,7 @@ bytearray_ass_subscript(PyByteArrayObject *self, PyObject *index, PyObject *valu return -1; } - int ival; + int ival = -1; // GH-91153: We need to do this *before* the size check, in case values // has a nasty __index__ method that changes the size of the bytearray: @@ -627,6 +628,7 @@ bytearray_ass_subscript(PyByteArrayObject *self, PyObject *index, PyObject *valu slicelen = 1; } else { + assert(0 <= ival && ival < 256); buf[i] = (char)ival; return 0; }