From 83171631c0d13610e4903fcb0b91bf7c2d09b7c1 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 13 Apr 2017 00:10:55 +0300 Subject: [PATCH 1/6] bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() raised an error. Replace them with using concrete types API that never fails if appropriate. --- Lib/test/test_io.py | 8 ++++++ Modules/_ctypes/_ctypes.c | 2 +- Modules/_elementtree.c | 4 +-- Modules/_io/iobase.c | 14 +++++++--- Modules/_winapi.c | 9 +++++-- Modules/cjkcodecs/multibytecodec.c | 3 +++ Modules/itertoolsmodule.c | 3 ++- Modules/posixmodule.c | 42 +++++++++++++++++++++++------- Modules/selectmodule.c | 4 +-- Objects/exceptions.c | 2 +- Objects/namespaceobject.c | 12 +++------ Objects/setobject.c | 6 +++-- Python/bltinmodule.c | 3 ++- 13 files changed, 77 insertions(+), 35 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 929865d9565998..49fade22379c47 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -543,6 +543,14 @@ def test_readline(self): with self.open(support.TESTFN, "r") as f: self.assertRaises(TypeError, f.readline, 5.3) + def test_readline_nonsizeable(self): + # Issue #30061 + # Crash when readline() returns an object without __len__ + class R(self.IOBase): + def readline(self): + return None + self.assertRaises((TypeError, StopIteration), next, R()) + def test_raw_bytes_io(self): f = self.BytesIO() self.write_ops(f) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 830b324a42b711..fc953af51846b4 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5143,7 +5143,7 @@ comerror_init(PyObject *self, PyObject *args, PyObject *kwds) if (!PyArg_ParseTuple(args, "OOO:COMError", &hresult, &text, &details)) return -1; - a = PySequence_GetSlice(args, 1, PySequence_Size(args)); + a = PySequence_GetSlice(args, 1, PyTuple_GET_SIZE(args)); if (!a) return -1; status = PyObject_SetAttrString(self, "args", a); diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 456c4a2a79918f..82df58ff39e34d 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1878,7 +1878,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) ); return -1; } - newlen = PySequence_Size(seq); + newlen = PySequence_Fast_GET_SIZE(seq); if (step != 1 && newlen != slicelen) { @@ -3660,7 +3660,7 @@ _elementtree_XMLParser__setevents_impl(XMLParserObject *self, return NULL; } - for (i = 0; i < PySequence_Size(events_seq); ++i) { + for (i = 0; i < PySequence_Fast_GET_SIZE(events_seq); ++i) { PyObject *event_name_obj = PySequence_Fast_GET_ITEM(events_seq, i); const char *event_name = NULL; if (PyUnicode_Check(event_name_obj)) { diff --git a/Modules/_io/iobase.c b/Modules/_io/iobase.c index 75cfe59624bfb9..3039ac4dcbaa4b 100644 --- a/Modules/_io/iobase.c +++ b/Modules/_io/iobase.c @@ -618,7 +618,8 @@ iobase_iternext(PyObject *self) if (line == NULL) return NULL; - if (PyObject_Size(line) == 0) { + if (PyObject_Size(line) <= 0) { + /* Error or empty */ Py_DECREF(line); return NULL; } @@ -667,6 +668,7 @@ _io__IOBase_readlines_impl(PyObject *self, Py_ssize_t hint) while (1) { PyObject *line = PyIter_Next(self); + Py_ssize_t line_length; if (line == NULL) { if (PyErr_Occurred()) { Py_DECREF(result); @@ -681,11 +683,15 @@ _io__IOBase_readlines_impl(PyObject *self, Py_ssize_t hint) Py_DECREF(result); return NULL; } - length += PyObject_Size(line); + line_length = PyObject_Size(line); Py_DECREF(line); - - if (length > hint) + if (line_length < 0) { + Py_DECREF(result); + return NULL; + } + if (line_length > hint - length) break; + length += line_length; } return result; } diff --git a/Modules/_winapi.c b/Modules/_winapi.c index cb6a85c86f34a5..805199221fc368 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -722,13 +722,18 @@ getenvironment(PyObject* environment) return NULL; } - envsize = PyMapping_Length(environment); - keys = PyMapping_Keys(environment); values = PyMapping_Values(environment); if (!keys || !values) goto error; + envsize = PyList_GET_SIZE(keys); + if (PyList_GET_SIZE(values) != envsize) { + PyErr_SetString(PyExc_RuntimeError, + "environment changed size during iteration"); + goto error; + } + totalsize = 1; /* trailing null character */ for (i = 0; i < envsize; i++) { PyObject* key = PyList_GET_ITEM(keys, i); diff --git a/Modules/cjkcodecs/multibytecodec.c b/Modules/cjkcodecs/multibytecodec.c index 8a67da732489ef..22172b043bcd88 100644 --- a/Modules/cjkcodecs/multibytecodec.c +++ b/Modules/cjkcodecs/multibytecodec.c @@ -1670,6 +1670,9 @@ _multibytecodec_MultibyteStreamWriter_writelines(MultibyteStreamWriterObject *se if (r == -1) return NULL; } + /* PySequence_Length() can fail */ + if (PyErr_Occurred()) + return NULL; Py_RETURN_NONE; } diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 9823c77f776b23..fac5b29a6acc22 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -4325,7 +4325,7 @@ zip_longest_new(PyTypeObject *type, PyObject *args, PyObject *kwds) PyObject *ittuple; /* tuple of iterators */ PyObject *result; PyObject *fillvalue = Py_None; - Py_ssize_t tuplesize = PySequence_Length(args); + Py_ssize_t tuplesize; if (kwds != NULL && PyDict_CheckExact(kwds) && PyDict_GET_SIZE(kwds) > 0) { fillvalue = PyDict_GetItemString(kwds, "fillvalue"); @@ -4338,6 +4338,7 @@ zip_longest_new(PyTypeObject *type, PyObject *args, PyObject *kwds) /* args must be a tuple */ assert(PyTuple_Check(args)); + tuplesize = PyTuple_GET_SIZE(args); /* obtain iterators */ ittuple = PyTuple_New(tuplesize); diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index c03fc15bf808c3..698f2922a0244c 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6642,7 +6642,7 @@ static PyObject * os_setgroups(PyObject *module, PyObject *groups) /*[clinic end generated code: output=3fcb32aad58c5ecd input=fa742ca3daf85a7e]*/ { - int i, len; + Py_ssize_t i, len; gid_t grouplist[MAX_GROUPS]; if (!PySequence_Check(groups)) { @@ -6650,6 +6650,9 @@ os_setgroups(PyObject *module, PyObject *groups) return NULL; } len = PySequence_Size(groups); + if (len < 0) { + return NULL; + } if (len > MAX_GROUPS) { PyErr_SetString(PyExc_ValueError, "too many groups"); return NULL; @@ -7877,9 +7880,9 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) #if (defined(HAVE_SENDFILE) && (defined(__FreeBSD__) || defined(__DragonFly__) \ || defined(__APPLE__))) || defined(HAVE_READV) || defined(HAVE_WRITEV) static Py_ssize_t -iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, int cnt, int type) +iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, int type) { - int i, j; + Py_ssize_t i, j; Py_ssize_t blen, total = 0; *iov = PyMem_New(struct iovec, cnt); @@ -7956,8 +7959,7 @@ static Py_ssize_t os_readv_impl(PyObject *module, int fd, PyObject *buffers) /*[clinic end generated code: output=792da062d3fcebdb input=e679eb5dbfa0357d]*/ { - int cnt; - Py_ssize_t n; + Py_ssize_t cnt, n; int async_err = 0; struct iovec *iov; Py_buffer *buf; @@ -7969,6 +7971,8 @@ os_readv_impl(PyObject *module, int fd, PyObject *buffers) } cnt = PySequence_Size(buffers); + if (cnt < 0) + return -1; if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0) return -1; @@ -8107,8 +8111,16 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) "sendfile() headers must be a sequence"); return NULL; } else { - Py_ssize_t i = 0; /* Avoid uninitialized warning */ - sf.hdr_cnt = PySequence_Size(headers); + Py_ssize_t i = PySequence_Size(headers); + if (i < 0) + return NULL; + if (i > INT_MAX) { + PyErr_SetString(PyExc_OverflowError, + "sendfile() header is too large"); + return NULL; + } + sf.hdr_cnt = (int)i; + i = 0; if (sf.hdr_cnt > 0 && (i = iov_setup(&(sf.headers), &hbuf, headers, sf.hdr_cnt, PyBUF_SIMPLE)) < 0) @@ -8124,8 +8136,16 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) "sendfile() trailers must be a sequence"); return NULL; } else { - Py_ssize_t i = 0; /* Avoid uninitialized warning */ - sf.trl_cnt = PySequence_Size(trailers); + Py_ssize_t i = PySequence_Size(trailers); + if (i < 0) + return NULL; + if (i > INT_MAX) { + PyErr_SetString(PyExc_OverflowError, + "sendfile() trailer is too large"); + return NULL; + } + sf.trl_cnt = (int)i; + i = 0; if (sf.trl_cnt > 0 && (i = iov_setup(&(sf.trailers), &tbuf, trailers, sf.trl_cnt, PyBUF_SIMPLE)) < 0) @@ -8402,7 +8422,7 @@ static Py_ssize_t os_writev_impl(PyObject *module, int fd, PyObject *buffers) /*[clinic end generated code: output=56565cfac3aac15b input=5b8d17fe4189d2fe]*/ { - int cnt; + Py_ssize_t cnt; Py_ssize_t result; int async_err = 0; struct iovec *iov; @@ -8414,6 +8434,8 @@ os_writev_impl(PyObject *module, int fd, PyObject *buffers) return -1; } cnt = PySequence_Size(buffers); + if (cnt < 0) + return -1; if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_SIMPLE) < 0) { return -1; diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c index 6f71d5855685f3..edaeb3f17e761e 100644 --- a/Modules/selectmodule.c +++ b/Modules/selectmodule.c @@ -2021,8 +2021,8 @@ newKqueue_Object(PyTypeObject *type, SOCKET fd) static PyObject * kqueue_queue_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { - if ((args != NULL && PyObject_Size(args)) || - (kwds != NULL && PyObject_Size(kwds))) { + if (PyTuple_GET_SIZE(args) || + (kwds != NULL && PyDict_GET_SIZE(kwds))) { PyErr_SetString(PyExc_ValueError, "select.kqueue doesn't accept arguments"); return NULL; diff --git a/Objects/exceptions.c b/Objects/exceptions.c index 0c7b9b268667dd..858eff5fc26c34 100644 --- a/Objects/exceptions.c +++ b/Objects/exceptions.c @@ -2790,7 +2790,7 @@ _PyErr_TrySetFromCause(const char *format, ...) /* Ensure the instance dict is also empty */ dictptr = _PyObject_GetDictPtr(val); if (dictptr != NULL && *dictptr != NULL && - PyObject_Length(*dictptr) > 0) { + PyDict_GET_SIZE(*dictptr) > 0) { /* While we could potentially copy a non-empty instance dictionary * to the replacement exception, for now we take the more * conservative path of leaving exceptions with attributes set diff --git a/Objects/namespaceobject.c b/Objects/namespaceobject.c index 0bb30638445a93..6deca961a4f117 100644 --- a/Objects/namespaceobject.c +++ b/Objects/namespaceobject.c @@ -40,15 +40,9 @@ namespace_new(PyTypeObject *type, PyObject *args, PyObject *kwds) static int namespace_init(_PyNamespaceObject *ns, PyObject *args, PyObject *kwds) { - // ignore args if it's NULL or empty - if (args != NULL) { - Py_ssize_t argcount = PyObject_Size(args); - if (argcount < 0) - return -1; - else if (argcount > 0) { - PyErr_Format(PyExc_TypeError, "no positional arguments expected"); - return -1; - } + if (PyTuple_GET_SIZE(args) != 0) { + PyErr_Format(PyExc_TypeError, "no positional arguments expected"); + return -1; } if (kwds == NULL) return 0; diff --git a/Objects/setobject.c b/Objects/setobject.c index 23b32d0d83b2e9..62e230eb40303d 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -1546,7 +1546,7 @@ set_difference(PySetObject *so, PyObject *other) PyObject *key; Py_hash_t hash; setentry *entry; - Py_ssize_t pos = 0; + Py_ssize_t pos = 0, other_size; int rv; if (PySet_GET_SIZE(so) == 0) { @@ -1559,7 +1559,9 @@ set_difference(PySetObject *so, PyObject *other) /* If len(so) much more than len(other), it's more efficient to simply copy * so and then iterate other looking for common elements. */ - if ((PySet_GET_SIZE(so) >> 2) > PyObject_Size(other)) { + other_size = PyDict_CheckExact(other) ? PyDict_GET_SIZE(other) + : PySet_GET_SIZE(other); + if ((PySet_GET_SIZE(so) >> 2) > other_size) { return set_copy_and_difference(so, other); } diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 8ae2303e98d333..dafc1365f69cd7 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2440,13 +2440,14 @@ zip_new(PyTypeObject *type, PyObject *args, PyObject *kwds) Py_ssize_t i; PyObject *ittuple; /* tuple of iterators */ PyObject *result; - Py_ssize_t tuplesize = PySequence_Length(args); + Py_ssize_t tuplesize; if (type == &PyZip_Type && !_PyArg_NoKeywords("zip()", kwds)) return NULL; /* args must be a tuple */ assert(PyTuple_Check(args)); + tuplesize = PyTuple_GET_SIZE(args); /* obtain iterators */ ittuple = PyTuple_New(tuplesize); From 68b90d88509c30dc4dacee7f87c9ad78288f43f7 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 14 Apr 2017 10:15:59 +0300 Subject: [PATCH 2/6] Address Xiang's comments. --- Modules/_winapi.c | 12 ++++++------ Modules/posixmodule.c | 30 ++++++++++++++++-------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 805199221fc368..2afc6590123d2b 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -727,8 +727,8 @@ getenvironment(PyObject* environment) if (!keys || !values) goto error; - envsize = PyList_GET_SIZE(keys); - if (PyList_GET_SIZE(values) != envsize) { + envsize = PySequence_Fast_GET_SIZE(keys); + if (PySequence_Fast_GET_SIZE(values) != envsize) { PyErr_SetString(PyExc_RuntimeError, "environment changed size during iteration"); goto error; @@ -736,8 +736,8 @@ getenvironment(PyObject* environment) totalsize = 1; /* trailing null character */ for (i = 0; i < envsize; i++) { - PyObject* key = PyList_GET_ITEM(keys, i); - PyObject* value = PyList_GET_ITEM(values, i); + PyObject* key = PySequence_Fast_GET_ITEM(keys, i); + PyObject* value = PySequence_Fast_GET_ITEM(values, i); if (! PyUnicode_Check(key) || ! PyUnicode_Check(value)) { PyErr_SetString(PyExc_TypeError, @@ -765,8 +765,8 @@ getenvironment(PyObject* environment) end = buffer + totalsize; for (i = 0; i < envsize; i++) { - PyObject* key = PyList_GET_ITEM(keys, i); - PyObject* value = PyList_GET_ITEM(values, i); + PyObject* key = PySequence_Fast_GET_ITEM(keys, i); + PyObject* value = PySequence_Fast_GET_ITEM(values, i); if (!PyUnicode_AsUCS4(key, p, end - p, 0)) goto error; p += PyUnicode_GET_LENGTH(key); diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 698f2922a0244c..e8c15a9473cfb1 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -8119,15 +8119,16 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) "sendfile() header is too large"); return NULL; } - sf.hdr_cnt = (int)i; - i = 0; - if (sf.hdr_cnt > 0 && - (i = iov_setup(&(sf.headers), &hbuf, - headers, sf.hdr_cnt, PyBUF_SIMPLE)) < 0) - return NULL; + if (i > 0) { + sf.hdr_cnt = (int)i; + i = iov_setup(&(sf.headers), &hbuf, + headers, sf.hdr_cnt, PyBUF_SIMPLE); + if (i < 0) + return NULL; #ifdef __APPLE__ - sbytes += i; + sbytes += i; #endif + } } } if (trailers != NULL) { @@ -8144,15 +8145,16 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) "sendfile() trailer is too large"); return NULL; } - sf.trl_cnt = (int)i; - i = 0; - if (sf.trl_cnt > 0 && - (i = iov_setup(&(sf.trailers), &tbuf, - trailers, sf.trl_cnt, PyBUF_SIMPLE)) < 0) - return NULL; + if (i > 0) { + sf.trl_cnt = (int)i; + i = iov_setup(&(sf.trailers), &tbuf, + trailers, sf.trl_cnt, PyBUF_SIMPLE); + if (i < 0) + return NULL; #ifdef __APPLE__ - sbytes += i; + sbytes += i; #endif + } } } From 80e16a25c2c702baf757b694097aa59cb640e3cc Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 14 Apr 2017 10:33:47 +0300 Subject: [PATCH 3/6] Add additional test. --- Lib/test/test_io.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 49fade22379c47..9733ed4345bac5 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -551,6 +551,14 @@ def readline(self): return None self.assertRaises((TypeError, StopIteration), next, R()) + def test_next_nonsizeable(self): + # Issue #30061 + # Crash when __next__() returns an object without __len__ + class R(self.IOBase): + def __next__(self): + return None + self.assertRaises(TypeError, R().readlines, 1) + def test_raw_bytes_io(self): f = self.BytesIO() self.write_ops(f) From e459aa6e4b264d3a1b13af0e05d608e31eaf3d24 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 16 Apr 2017 19:22:48 +0300 Subject: [PATCH 4/6] Fix a leak in iobase.c. --- Modules/_io/iobase.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Modules/_io/iobase.c b/Modules/_io/iobase.c index 31d9bcf0855afd..bcefb3862c3200 100644 --- a/Modules/_io/iobase.c +++ b/Modules/_io/iobase.c @@ -671,8 +671,8 @@ _io__IOBase_readlines_impl(PyObject *self, Py_ssize_t hint) } while (1) { - PyObject *line = PyIter_Next(it); Py_ssize_t line_length; + PyObject *line = PyIter_Next(it); if (line == NULL) { if (PyErr_Occurred()) { goto error; @@ -688,8 +688,7 @@ _io__IOBase_readlines_impl(PyObject *self, Py_ssize_t hint) line_length = PyObject_Size(line); Py_DECREF(line); if (line_length < 0) { - Py_DECREF(result); - return NULL; + goto error; } if (line_length > hint - length) break; From 25cc5b5cc529d48abf8682186d4ffe39d82c1213 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 19 Apr 2017 18:45:43 +0300 Subject: [PATCH 5/6] Add a Misc/NEWS entry and optimize set checks. --- Misc/NEWS | 5 +++++ Objects/setobject.c | 10 +++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS b/Misc/NEWS index 512592e81bec41..4dc7b088b7d480 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -313,6 +313,11 @@ Extension Modules Library ------- +- bpo-30061: Fixed crashes in IOBase methods __next__() and readlines() when + readline() or __next__() respectively return non-sizeable object. + Fixed possible other errors caused by not checking results of PyObject_Size(), + PySequence_Size(), or PyMapping_Size(). + - bpo-10076: Compiled regular expression and match objects in the re module now support copy.copy() and copy.deepcopy() (they are considered atomic). diff --git a/Objects/setobject.c b/Objects/setobject.c index 62e230eb40303d..472f32a558c8cf 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -1553,14 +1553,18 @@ set_difference(PySetObject *so, PyObject *other) return set_copy(so); } - if (!PyAnySet_Check(other) && !PyDict_CheckExact(other)) { + if (PyAnySet_Check(other) { + other_size = PySet_GET_SIZE(other); + } + else if (PyDict_CheckExact(other)) { + other_size = PyDict_GET_SIZE(other); + } + else { return set_copy_and_difference(so, other); } /* If len(so) much more than len(other), it's more efficient to simply copy * so and then iterate other looking for common elements. */ - other_size = PyDict_CheckExact(other) ? PyDict_GET_SIZE(other) - : PySet_GET_SIZE(other); if ((PySet_GET_SIZE(so) >> 2) > other_size) { return set_copy_and_difference(so, other); } From 6908bd9a4caef2bdd0ab0077715cdd68c6b12723 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 19 Apr 2017 18:57:56 +0300 Subject: [PATCH 6/6] Fixed a syntax error. --- Objects/setobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/setobject.c b/Objects/setobject.c index 472f32a558c8cf..a9dba31c423859 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -1553,7 +1553,7 @@ set_difference(PySetObject *so, PyObject *other) return set_copy(so); } - if (PyAnySet_Check(other) { + if (PyAnySet_Check(other)) { other_size = PySet_GET_SIZE(other); } else if (PyDict_CheckExact(other)) {