gh-127681: add overflow checks when using `PyMem_Malloc` #127686

picnixz · 2024-12-06T12:31:56Z

For now, this only adds checks in Modules and Objects.

Issue: Add overflow checks before calling memory allocators #127681

picnixz · 2024-12-06T13:37:32Z

I've added comments for future readers so that they know that something does not need to be checked or not, but I can also remove them if you think it's overly verbose.

serhiy-storchaka

I reviewed, and found only one site where an integer overflow may be possible. In all other cases it is not possible.

serhiy-storchaka · 2024-12-06T15:47:03Z

Modules/_ctypes/_ctypes.c

+    if (prefix) {
+        size_t prefix_len = strlen(prefix);
+        if (len > PY_SIZE_MAX - prefix_len) {
+            goto oom;
+        }
+        len += prefix_len;
+    }
+    if (len == PY_SIZE_MAX) {
+        goto oom;
+    }


Do you have an example of overflow? AFAIK, prefix is smaller than 1030 bytes, and suffix also is a short string.

serhiy-storchaka · 2024-12-06T15:48:22Z

Modules/_ctypes/_ctypes.c

+    if (prefix) {
+        size_t l = strlen(prefix);
+        if ((size_t)prefix > (size_t)PY_SSIZE_T_MAX - l) {
+            goto oom;
+        }
+        prefix_len += l;
+    }


prefix is always "&" here.

serhiy-storchaka · 2024-12-06T15:49:04Z

Modules/_ctypes/_ctypes.c

@@ -1666,7 +1685,7 @@ PyCArrayType_init(PyObject *self, PyObject *args, PyObject *kwds)
    if (stginfo->format == NULL)
        goto error;
    stginfo->ndim = iteminfo->ndim + 1;
-    stginfo->shape = PyMem_Malloc(sizeof(Py_ssize_t) * stginfo->ndim);
+    stginfo->shape = PyMem_New(Py_ssize_t, stginfo->ndim);


This never overflows. ndim < 32.

serhiy-storchaka · 2024-12-06T15:49:54Z

Modules/_ctypes/_ctypes.c

@@ -3094,7 +3113,7 @@ PyCData_MallocBuffer(CDataObject *obj, StgInfo *info)
        /* In python 2.4, and ctypes 0.9.6, the malloc call took about
           33% of the creation time for c_int().
        */
-        obj->b_ptr = (char *)PyMem_Malloc(info->size);
+        obj->b_ptr = PyMem_New(char, info->size);


This is the same.

serhiy-storchaka · 2024-12-06T15:51:23Z

Modules/_ctypes/_ctypes.c

@@ -4775,7 +4794,7 @@ Array_subscript(PyObject *myself, PyObject *item)
                return PyBytes_FromStringAndSize(ptr + start,
                                                 slicelen);
            }
-            dest = (char *)PyMem_Malloc(slicelen);
+            dest = PyMem_New(char, slicelen);


This is the same.

serhiy-storchaka · 2024-12-06T16:15:10Z

Modules/_posixsubprocess.c

@@ -1192,7 +1195,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
 #endif /* HAVE_SETREUID */
    }

-    c_fds_to_keep = PyMem_Malloc(fds_to_keep_len * sizeof(int));
+    c_fds_to_keep = PyMem_New(int, fds_to_keep_len);


It never overflows. fds_to_keep_len is the size of a tuple. sizeof(int) <= sizeof(PyObject *).

serhiy-storchaka · 2024-12-06T16:16:56Z

Modules/_struct.c

@@ -1725,7 +1725,7 @@ prepare_s(PyStructObject *self)
    }

    /* check for overflow */
-    if ((ncodes + 1) > ((size_t)PY_SSIZE_T_MAX / sizeof(formatcode))) {
+    if (ncodes > ((size_t)PY_SSIZE_T_MAX / sizeof(formatcode)) - 1) {


ncodes + 1 does not overflow. ncodes is not longer then the length of str/bytes.

serhiy-storchaka · 2024-12-06T16:18:44Z

Objects/abstract.c

@@ -705,8 +705,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);
+    indices = PyMem_New(Py_ssize_t, view_src.ndim);


It does not overflow. ndim < 32.

serhiy-storchaka · 2024-12-06T16:19:42Z

Objects/call.c

@@ -484,10 +484,13 @@ _PyObject_Call_Prepend(PyThreadState *tstate, PyObject *callable,
    PyObject **stack;

    Py_ssize_t argcount = PyTuple_GET_SIZE(args);
-    if (argcount + 1 <= (Py_ssize_t)Py_ARRAY_LENGTH(small_stack)) {
+    if (argcount <= (Py_ssize_t)Py_ARRAY_LENGTH(small_stack) - 1) {


argcount + 1 does not overflow. argcount is the length of a tuple.

serhiy-storchaka · 2024-12-06T16:21:33Z

Objects/listobject.c

@@ -3637,8 +3637,7 @@ list_ass_subscript_lock_held(PyObject *_self, PyObject *item, PyObject *value)
                step = -step;
            }

-            garbage = (PyObject**)
-                PyMem_Malloc(slicelength*sizeof(PyObject*));
+            garbage = PyMem_New(PyObject *, slicelength);


It does not overflow. slicelength is the length of a slice in existing list. The list already uses at least slicelength*sizeof(PyObject*) bytes.

bedevere-app · 2024-12-06T16:24:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

picnixz · 2024-12-06T16:28:31Z

So actually, there are parts that could be removed instead of fortified. I won't work on this one today but my question is: can I actually add the comments so that we know that there won't be someone else wondering "oh maybe we should check for this"?

serhiy-storchaka · 2024-12-06T16:38:06Z

I don't like pollution of the code with trivial comments. They actually make reading the code more difficult -- you need to read and comprehend the comment instead of just skipping the presumably correct code. It is responsibility of the contributor to prove that they changes are necessary. The CPython code is mature enough, and most trivial bugs are fixed.

So I suggest you to close this PR and open a new PR with only cases for which you can show at least theoretical possibility of integer overflow.

picnixz added 2 commits December 6, 2024 13:30

audit PyMem_Malloc allocators in Modules

adf0276

audit PyMem_Malloc allocators in Objects

3dce338

bedevere-app bot mentioned this pull request Dec 6, 2024

Add overflow checks before calling memory allocators #127681

Open

picnixz added the skip news label Dec 6, 2024

picnixz changed the title ~~gh-127681: add overflow checks when using memory allocators~~ gh-127681: add overflow checks when using PyMem_Malloc Dec 6, 2024

picnixz added 6 commits December 6, 2024 13:44

fix warning

73796f7

fix compilation

8f40dc0

fix checks

b79c86e

fix checks

010a3ab

fix test

33dc820

simplify arithmetic

999d5e6

picnixz marked this pull request as ready for review December 6, 2024 13:37

picnixz requested review from rhettinger, gpshead, FFY00, markshannon and methane as code owners December 6, 2024 13:37

bedevere-app bot added the awaiting review label Dec 6, 2024

picnixz requested review from vstinner and serhiy-storchaka December 6, 2024 14:01

serhiy-storchaka requested changes Dec 6, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Dec 6, 2024

picnixz marked this pull request as draft December 6, 2024 16:28

bedevere-app bot removed the awaiting changes label Dec 6, 2024

picnixz closed this Dec 6, 2024

picnixz deleted the fix/mem/overflow-checks-127681 branch December 6, 2024 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-127681: add overflow checks when using `PyMem_Malloc` #127686

gh-127681: add overflow checks when using `PyMem_Malloc` #127686

picnixz commented Dec 6, 2024 •

edited

Loading

picnixz commented Dec 6, 2024

serhiy-storchaka left a comment

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

serhiy-storchaka Dec 6, 2024

bedevere-app bot commented Dec 6, 2024

picnixz commented Dec 6, 2024

serhiy-storchaka commented Dec 6, 2024

gh-127681: add overflow checks when using PyMem_Malloc #127686

gh-127681: add overflow checks when using PyMem_Malloc #127686

Conversation

picnixz commented Dec 6, 2024 • edited Loading

picnixz commented Dec 6, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Dec 6, 2024

picnixz commented Dec 6, 2024

serhiy-storchaka commented Dec 6, 2024

gh-127681: add overflow checks when using `PyMem_Malloc` #127686

gh-127681: add overflow checks when using `PyMem_Malloc` #127686

picnixz commented Dec 6, 2024 •

edited

Loading