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.

5 files changed, 24 insertions, 17 deletions

diff --git a/Objects/abstract.c b/Objects/abstract.c
index a48d5dc..4e25061 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; 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 a8bbd15..ccd4980 100644
--- 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 5a03beb..0ada1e7 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 710495f..7afaac0 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 efcb7ae..27fb969 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("<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);
 }