Issue #2620: Overflow checking when allocating or reallocating memory

was not always being done properly in some python types and extension
modules.  PyMem_MALLOC, PyMem_REALLOC, PyMem_NEW and PyMem_RESIZE have
all been updated to perform better checks and places in the code that
would previously leak memory on the error path when such an allocation
failed have been fixed.

diff --git a/Doc/c-api/memory.rst b/Doc/c-api/memory.rst
index 1dcb1158383b9e3b3a0ebee819c6548579b08351..81d7cd96cfd087aa623909cfd48b8f53f9f38c88 100644 (file)
--- a/Doc/c-api/memory.rst
+++ b/Doc/c-api/memory.rst
@@ -136,7 +136,9 @@ The following type-oriented macros are provided for convenience.  Note  that
 
    Same as :cfunc:`PyMem_Realloc`, but the memory block is resized to ``(n *
    sizeof(TYPE))`` bytes.  Returns a pointer cast to :ctype:`TYPE\*`. On return,
-   *p* will be a pointer to the new memory area, or *NULL* in the event of failure.
+   *p* will be a pointer to the new memory area, or *NULL* in the event of
+   failure.  This is a C preprocessor macro; p is always reassigned.  Save
+   the original value of p to avoid losing memory when handling errors.
 
 
 .. cfunction:: void PyMem_Del(void *p)

diff --git a/Include/pymem.h b/Include/pymem.h
index f9acb55f745d412772f66a17cccb279a0c372ab4..542aceef25370db0b32f15421d23560f602dcab3 100644 (file)
--- a/Include/pymem.h
+++ b/Include/pymem.h
@@ -69,8 +69,12 @@ PyAPI_FUNC(void) PyMem_Free(void *);
    for malloc(0), which would be treated as an error. Some platforms
    would return a pointer with no memory behind it, which would break
    pymalloc. To solve these problems, allocate an extra byte. */
-#define PyMem_MALLOC(n)         malloc((n) ? (n) : 1)
-#define PyMem_REALLOC(p, n)     realloc((p), (n) ? (n) : 1)
+/* Returns NULL to indicate error if a negative size or size larger than
+   Py_ssize_t can represent is supplied.  Helps prevents security holes. */
+#define PyMem_MALLOC(n)                (((n) < 0 || (n) > PY_SSIZE_T_MAX) ? NULL \
+                               : malloc((n) ? (n) : 1))
+#define PyMem_REALLOC(p, n)    (((n) < 0 || (n) > PY_SSIZE_T_MAX) ? NULL \
+                               : realloc((p), (n) ? (n) : 1))
 #define PyMem_FREE             free
 
 #endif /* PYMALLOC_DEBUG */
@@ -79,24 +83,31 @@ PyAPI_FUNC(void) PyMem_Free(void *);
  * Type-oriented memory interface
  * ==============================
  *
- * These are carried along for historical reasons.  There's rarely a good
- * reason to use them anymore (you can just as easily do the multiply and
- * cast yourself).
+ * Allocate memory for n objects of the given type.  Returns a new pointer
+ * or NULL if the request was too large or memory allocation failed.  Use
+ * these macros rather than doing the multiplication yourself so that proper
+ * overflow checking is always done.
  */
 
 #define PyMem_New(type, n) \
-  ( assert((n) <= PY_SIZE_MAX / sizeof(type)) , \
+  ( ((n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
        ( (type *) PyMem_Malloc((n) * sizeof(type)) ) )
 #define PyMem_NEW(type, n) \
-  ( assert((n) <= PY_SIZE_MAX / sizeof(type)) , \
+  ( ((n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
        ( (type *) PyMem_MALLOC((n) * sizeof(type)) ) )
 
+/*
+ * The value of (p) is always clobbered by this macro regardless of success.
+ * The caller MUST check if (p) is NULL afterwards and deal with the memory
+ * error if so.  This means the original value of (p) MUST be saved for the
+ * caller's memory error handler to not lose track of it.
+ */
 #define PyMem_Resize(p, type, n) \
-  ( assert((n) <= PY_SIZE_MAX / sizeof(type)) , \
-       ( (p) = (type *) PyMem_Realloc((p), (n) * sizeof(type)) ) )
+  ( (p) = ((n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
+       (type *) PyMem_Realloc((p), (n) * sizeof(type)) )
 #define PyMem_RESIZE(p, type, n) \
-  ( assert((n) <= PY_SIZE_MAX / sizeof(type)) , \
-       ( (p) = (type *) PyMem_REALLOC((p), (n) * sizeof(type)) ) )
+  ( (p) = ((n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
+       (type *) PyMem_REALLOC((p), (n) * sizeof(type)) )
 
 /* PyMem{Del,DEL} are left over from ancient days, and shouldn't be used
  * anymore.  They're just confusing aliases for PyMem_{Free,FREE} now.

diff --git a/Misc/NEWS b/Misc/NEWS
index 794663354b1e28f0e96955d7ac3edfc4b47c1c9b..97852ef198c06c7b3b1232323d1f3937781b26ec 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -16,6 +16,13 @@ Core and Builtins
   the python debugger steps into a class statement: the free variables (local
   variables defined in an outer scope) would be deleted from the outer scope.
 
+- Issue #2620: Overflow checking when allocating or reallocating memory
+  was not always being done properly in some python types and extension
+  modules.  PyMem_MALLOC, PyMem_REALLOC, PyMem_NEW and PyMem_RESIZE have
+  all been updated to perform better checks and places in the code that
+  would previously leak memory on the error path when such an allocation
+  failed have been fixed.
+
 Library
 -------
 

diff --git a/Modules/almodule.c b/Modules/almodule.c
index 7f48fffed4b1ab487677d814a39121b6e85340f7..93f26bd48e64fee903eda9a49714eb55d6e32aba 100644 (file)
--- a/Modules/almodule.c
+++ b/Modules/almodule.c
@@ -1633,9 +1633,11 @@ al_QueryValues(PyObject *self, PyObject *args)
        if (nvals < 0)
                goto cleanup;
        if (nvals > setsize) {
+               ALvalue *old_return_set = return_set;
                setsize = nvals;
                PyMem_RESIZE(return_set, ALvalue, setsize);
                if (return_set == NULL) {
+                       return_set = old_return_set;
                        PyErr_NoMemory();
                        goto cleanup;
                }

diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c
index 99d25d38d2f6579a4bbaa85d28efa656ded39df5..bcd82aa977a72fcfff0a8af2d9fc22d621e793fb 100644 (file)
--- a/Modules/arraymodule.c
+++ b/Modules/arraymodule.c
@@ -815,6 +815,7 @@ static int
 array_do_extend(arrayobject *self, PyObject *bb)
 {
        Py_ssize_t size;
+       char *old_item;
 
        if (!array_Check(bb))
                return array_iter_extend(self, bb);
@@ -830,8 +831,10 @@ array_do_extend(arrayobject *self, PyObject *bb)
                return -1;
        }
        size = Py_SIZE(self) + Py_SIZE(b);
+       old_item = self->ob_item;
         PyMem_RESIZE(self->ob_item, char, size*self->ob_descr->itemsize);
         if (self->ob_item == NULL) {
+               self->ob_item = old_item;
                PyErr_NoMemory();
                return -1;
         }
@@ -884,7 +887,7 @@ array_inplace_repeat(arrayobject *self, Py_ssize_t n)
                        if (size > PY_SSIZE_T_MAX / n) {
                                return PyErr_NoMemory();
                        }
-                       PyMem_Resize(items, char, n * size);
+                       PyMem_RESIZE(items, char, n * size);
                        if (items == NULL)
                                return PyErr_NoMemory();
                        p = items;

diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c
index 83a6538a411790fe22e1728a728223401cf405c5..86ee3cc8cbb23c5bdbd6b8b983de1d40b474ed05 100644 (file)
--- a/Modules/selectmodule.c
+++ b/Modules/selectmodule.c
@@ -349,10 +349,12 @@ update_ufd_array(pollObject *self)
 {
        Py_ssize_t i, pos;
        PyObject *key, *value;
+        struct pollfd *old_ufds = self->ufds;
 
        self->ufd_len = PyDict_Size(self->dict);
-       PyMem_Resize(self->ufds, struct pollfd, self->ufd_len);
+       PyMem_RESIZE(self->ufds, struct pollfd, self->ufd_len);
        if (self->ufds == NULL) {
+                self->ufds = old_ufds;
                PyErr_NoMemory();
                return 0;
        }

diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c
index efbd566469eca3e0ca86220e2e8d7d238c220314..da8f9c2d37af1277057300aa948ed27e7a813906 100644 (file)
--- a/Objects/obmalloc.c
+++ b/Objects/obmalloc.c
@@ -726,6 +726,15 @@ PyObject_Malloc(size_t nbytes)
        poolp next;
        uint size;
 
+       /*
+        * Limit ourselves to PY_SSIZE_T_MAX bytes to prevent security holes.
+        * Most python internals blindly use a signed Py_ssize_t to track
+        * things without checking for overflows or negatives.
+        * As size_t is unsigned, checking for nbytes < 0 is not required.
+        */
+       if (nbytes > PY_SSIZE_T_MAX)
+               return NULL;
+
        /*
         * This implicitly redirects malloc(0).
         */
@@ -1130,6 +1139,15 @@ PyObject_Realloc(void *p, size_t nbytes)
        if (p == NULL)
                return PyObject_Malloc(nbytes);
 
+       /*
+        * Limit ourselves to PY_SSIZE_T_MAX bytes to prevent security holes.
+        * Most python internals blindly use a signed Py_ssize_t to track
+        * things without checking for overflows or negatives.
+        * As size_t is unsigned, checking for nbytes < 0 is not required.
+        */
+       if (nbytes > PY_SSIZE_T_MAX)
+               return NULL;
+
        pool = POOL_ADDR(p);
        if (Py_ADDRESS_IN_RANGE(p, pool)) {
                /* We're in charge of this block */