From faa54a392951468090f0095ffa927b16fcb20ca4 Mon Sep 17 00:00:00 2001 From: Neal Norwitz Date: Sun, 19 Aug 2007 04:23:20 +0000 Subject: [PATCH] Code review of the new buffer protocol. Mostly add questions that should be answered with the comments removed. There are many places that require checks when doing arithmetic for memory sizes when allocating memory. Otherwise, overflow is possible with a subsequent crash. Fix SF #1777057 which was a result of not initializing the new BufferError properly. Had to update the test for exceptions for BufferError too. --- Include/bytesobject.h | 1 + Include/object.h | 2 +- Lib/test/exception_hierarchy.txt | 1 + Modules/arraymodule.c | 4 ++-- Objects/abstract.c | 4 ++++ Objects/bufferobject.c | 5 ++++- Objects/bytesobject.c | 1 + Objects/exceptions.c | 2 ++ Objects/memoryobject.c | 29 +++++++++++++---------------- 9 files changed, 29 insertions(+), 20 deletions(-) diff --git a/Include/bytesobject.h b/Include/bytesobject.h index 12ecf64d0c3..729af398f03 100644 --- a/Include/bytesobject.h +++ b/Include/bytesobject.h @@ -21,6 +21,7 @@ extern "C" { /* Object layout */ typedef struct { PyObject_VAR_HEAD + /* XXX(nnorwitz): should ob_exports be Py_ssize_t? */ int ob_exports; /* how many buffer exports */ Py_ssize_t ob_alloc; /* How many bytes allocated */ char *ob_bytes; diff --git a/Include/object.h b/Include/object.h index d936bcaa808..dfabefdd839 100644 --- a/Include/object.h +++ b/Include/object.h @@ -147,7 +147,7 @@ typedef struct bufferinfo { Py_ssize_t len; Py_ssize_t itemsize; int readonly; - int ndim; + int ndim; /* XXX(nnorwitz): should be Py_ssize_t??? */ char *format; Py_ssize_t *shape; Py_ssize_t *strides; diff --git a/Lib/test/exception_hierarchy.txt b/Lib/test/exception_hierarchy.txt index 079ce2963c3..3714a4115aa 100644 --- a/Lib/test/exception_hierarchy.txt +++ b/Lib/test/exception_hierarchy.txt @@ -10,6 +10,7 @@ BaseException | +-- ZeroDivisionError +-- AssertionError +-- AttributeError + +-- BufferError +-- EnvironmentError | +-- IOError | +-- OSError diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c index 656fc091734..0e8673f1649 100644 --- a/Modules/arraymodule.c +++ b/Modules/arraymodule.c @@ -42,10 +42,8 @@ static PyTypeObject Arraytype; #ifdef Py_UNICODE_WIDE #define PyArr_UNI 'w' -/*static const char *PyArr_UNISTR = "w"; */ #else #define PyArr_UNI 'u' -/*static const char *PyArr_UNISTR = "u"; */ #endif #define array_Check(op) PyObject_TypeCheck(op, &Arraytype) @@ -1773,6 +1771,8 @@ array_buffer_getbuf(arrayobject *self, PyBuffer *view, int flags) view->internal = NULL; if ((flags & PyBUF_FORMAT) == PyBUF_FORMAT) { view->internal = malloc(3); + /* XXX(nnorwitz): need to check for malloc failure. + Should probably use PyObject_Malloc. */ view->format = view->internal; view->format[0] = (char)(self->ob_descr->typecode); view->format[1] = '\0'; diff --git a/Objects/abstract.c b/Objects/abstract.c index a48d5dc9924..4e250614e42 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -471,6 +471,7 @@ PyBuffer_ToContiguous(void *buf, PyBuffer *view, Py_ssize_t len, char fort) /* Otherwise a more elaborate scheme is needed */ + /* XXX(nnorwitz): need to check for overflow! */ indices = (Py_ssize_t *)PyMem_Malloc(sizeof(Py_ssize_t)*(view->ndim)); if (indices == NULL) { PyErr_NoMemory(); @@ -521,6 +522,7 @@ PyBuffer_FromContiguous(PyBuffer *view, void *buf, Py_ssize_t len, char fort) /* Otherwise a more elaborate scheme is needed */ + /* XXX(nnorwitz): need to check for overflow! */ indices = (Py_ssize_t *)PyMem_Malloc(sizeof(Py_ssize_t)*(view->ndim)); if (indices == NULL) { PyErr_NoMemory(); @@ -594,6 +596,7 @@ int PyObject_CopyData(PyObject *dest, PyObject *src) /* Otherwise a more elaborate copy scheme is needed */ + /* XXX(nnorwitz): need to check for overflow! */ indices = (Py_ssize_t *)PyMem_Malloc(sizeof(Py_ssize_t)*view_src.ndim); if (indices == NULL) { PyErr_NoMemory(); @@ -606,6 +609,7 @@ int PyObject_CopyData(PyObject *dest, PyObject *src) } elements = 1; for (k=0; kbf_releasebuffer)(self->b_base, view); } } - return; + /* XXX(nnorwitz): do we need to release view here? it leaks. */ } static PyObject * @@ -401,6 +401,7 @@ buffer_concat(PyBufferObject *self, PyObject *other) return NULL; } + /* XXX(nnorwitz): need to check for overflow! */ ob = PyBytes_FromStringAndSize(NULL, view.len+view2.len); if ( ob == NULL ) { PyObject_ReleaseBuffer((PyObject *)self, &view); @@ -427,6 +428,7 @@ buffer_repeat(PyBufferObject *self, Py_ssize_t count) count = 0; if (!get_buf(self, &view, PyBUF_SIMPLE)) return NULL; + /* XXX(nnorwitz): need to check for overflow! */ ob = PyBytes_FromStringAndSize(NULL, view.len * count); if ( ob == NULL ) return NULL; @@ -474,6 +476,7 @@ buffer_slice(PyBufferObject *self, Py_ssize_t left, Py_ssize_t right) right = view.len; if ( right < left ) right = left; + /* XXX(nnorwitz): is it possible to access unitialized memory? */ ob = PyBytes_FromStringAndSize((char *)view.buf + left, right - left); PyObject_ReleaseBuffer((PyObject *)self, &view); diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c index 5a03beb234a..0ada1e72091 100644 --- a/Objects/bytesobject.c +++ b/Objects/bytesobject.c @@ -507,6 +507,7 @@ bytes_setslice(PyBytesObject *self, Py_ssize_t lo, Py_ssize_t hi, memmove(self->ob_bytes + lo + needed, self->ob_bytes + hi, Py_Size(self) - hi); } + /* XXX(nnorwitz): need to verify this can't overflow! */ if (PyBytes_Resize((PyObject *)self, Py_Size(self) + needed - avail) < 0) { res = -1; diff --git a/Objects/exceptions.c b/Objects/exceptions.c index 710495f6105..7afaac04f3f 100644 --- a/Objects/exceptions.c +++ b/Objects/exceptions.c @@ -1694,6 +1694,7 @@ _PyExc_Init(void) PRE_INIT(ZeroDivisionError) PRE_INIT(SystemError) PRE_INIT(ReferenceError) + PRE_INIT(BufferError) PRE_INIT(MemoryError) PRE_INIT(Warning) PRE_INIT(UserWarning) @@ -1753,6 +1754,7 @@ _PyExc_Init(void) POST_INIT(ZeroDivisionError) POST_INIT(SystemError) POST_INIT(ReferenceError) + POST_INIT(BufferError) POST_INIT(MemoryError) POST_INIT(Warning) POST_INIT(UserWarning) diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index efcb7aeb190..27fb9695e9f 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -26,6 +26,7 @@ Create a new memoryview object which references the given object."); PyObject * PyMemoryView_FromMemory(PyBuffer *info) { + /* XXX(nnorwitz): need to implement something here? */ return NULL; } @@ -59,7 +60,7 @@ static PyObject * memory_new(PyTypeObject *subtype, PyObject *args, PyObject *kwds) { PyObject *obj; - if (!PyArg_ParseTuple(args, "O", &obj)) return NULL; + if (!PyArg_UnpackTuple(args, "memoryview", 1, 1, &obj)) return NULL; return PyMemoryView_FromObject(obj); } @@ -136,6 +137,7 @@ _indirect_copy_nd(char *dest, PyBuffer *view, char fort) void (*func)(int, Py_ssize_t *, Py_ssize_t *); + /* XXX(nnorwitz): need to check for overflow! */ indices = (Py_ssize_t *)PyMem_Malloc(sizeof(Py_ssize_t)*view->ndim); if (indices == NULL) { PyErr_NoMemory(); @@ -260,6 +262,7 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) /* return a shadowed memory-view object */ view->buf = dest; mem->base = PyTuple_Pack(2, obj, bytes); + /* XXX(nnorwitz): need to verify alloc was successful. */ Py_DECREF(bytes); } else { @@ -373,17 +376,15 @@ static PyGetSetDef memory_getsetlist[] ={ static PyObject * -memory_tobytes(PyMemoryViewObject *mem, PyObject *args) +memory_tobytes(PyMemoryViewObject *mem, PyObject *noargs) { - if (!PyArg_ParseTuple(args, "")) return NULL; /* Create new Bytes object for data */ return PyBytes_FromObject((PyObject *)mem); } static PyObject * -memory_tolist(PyMemoryViewObject *mem, PyObject *args) +memory_tolist(PyMemoryViewObject *mem, PyObject *noargs) { - if (!PyArg_ParseTuple(args, "")) return NULL; Py_INCREF(Py_NotImplemented); return Py_NotImplemented; } @@ -391,8 +392,8 @@ memory_tolist(PyMemoryViewObject *mem, PyObject *args) static PyMethodDef memory_methods[] = { - {"tobytes", (PyCFunction)memory_tobytes, 1, NULL}, - {"tolist", (PyCFunction)memory_tolist, 1, NULL}, + {"tobytes", (PyCFunction)memory_tobytes, METH_NOARGS, NULL}, + {"tolist", (PyCFunction)memory_tolist, METH_NOARGS, NULL}, {NULL, NULL} /* sentinel */ }; @@ -400,7 +401,6 @@ static PyMethodDef memory_methods[] = { static void memory_dealloc(PyMemoryViewObject *self) { - if (PyTuple_Check(self->base)) { /* Special case when first element is generic object with buffer interface and the second element is a @@ -423,21 +423,18 @@ memory_dealloc(PyMemoryViewObject *self) else { PyObject_ReleaseBuffer(self->base, &(self->view)); } - Py_DECREF(self->base); + Py_CLEAR(self->base); PyObject_DEL(self); } static PyObject * memory_repr(PyMemoryViewObject *self) { - - if ( self->base == NULL ) - return PyUnicode_FromFormat("", - self); + /* XXX(nnorwitz): the code should be different or remove condition. */ + if (self->base == NULL) + return PyUnicode_FromFormat("", self); else - return PyUnicode_FromFormat( - "", - self); + return PyUnicode_FromFormat("", self); }