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.

diff --git a/Include/bytesobject.h b/Include/bytesobject.h
index 12ecf64d0c3131bbd6d4228bbc922ec9c64404db..729af398f038e8dd9ff081ec5b4427370ef4013a 100644 (file)
--- 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 d936bcaa808f2ee643c7eadc196f7c5d5d791f1d..dfabefdd83920d0efede299a7e1ffe3dc14eb81d 100644 (file)
--- 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 079ce2963c308d0ceb127e6fbe1f58ab32125708..3714a4115aa1c39f04f791cd23a05c5a0b98edd8 100644 (file)
--- 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 656fc091734294166f3fefeaf4db2eaba7de1330..0e8673f1649a68f971bacdb8a233ce0529d00a03 100644 (file)
--- 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 a48d5dc992460528d3181979f18d206f018e824b..4e250614e42b1d9e8ee63994c8caf9fcd5ffb709 100644 (file)
--- 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; k<view_src.ndim; k++) {
+               /* XXX(nnorwitz): can this overflow? */
                 elements *= view_src.shape[k];
         }
         while (elements--) {

diff --git a/Objects/bufferobject.c b/Objects/bufferobject.c
index a8bbd151c8f9aadad1d9a2743b00b12016bf4e7d..ccd49805ceab5411d3a8bd02f1643c719146a2b8 100644 (file)
--- a/Objects/bufferobject.c
+++ b/Objects/bufferobject.c
@@ -64,7 +64,7 @@ buffer_releasebuf(PyBufferObject *self, PyBuffer *view)
                         (*bp->bf_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 5a03beb234a8342f74c1196c5a257de347890b05..0ada1e720914d68a0d529af7f3cf6607b3b92784 100644 (file)
--- 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 710495f6105fa6d8f181f242ad29b8de37f5e8ed..7afaac04f3f28eda3cd1ee55166ac4f54e75c9c5 100644 (file)
--- 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 efcb7aeb190e909934ef9b69a8ec421cefbbb2c6..27fb9695e9fa3e18a0155872c8f03b459f8b0a82 100644 (file)
--- 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("<memory at %p>",
-                                          self);
+       /* XXX(nnorwitz): the code should be different or remove condition. */
+       if (self->base == NULL)
+               return PyUnicode_FromFormat("<memory at %p>", self);
        else
-               return PyUnicode_FromFormat(
-                       "<memory at %p>",
-                       self);
+               return PyUnicode_FromFormat("<memory at %p>", self);
 }