gh-97616: list_resize() checks for integer overflow by vstinner · Pull Request #97617 · python/cpython

vstinner · 2022-09-28T15:14:27Z

Fix multiplying a list by an integer (list * int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor.

list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)).

Issue: [security] list_resize(): integer overflow if newsize is too large #97616

vstinner · 2022-09-28T15:15:04Z

@gpshead @serhiy-storchaka @methane: Would you mind to review my list_resize() fix?

serhiy-storchaka · 2022-09-28T17:23:18Z

~~There is no integer overflow here.~~

serhiy-storchaka · 2022-09-28T17:27:05Z

Or rather there should not be integer overflow. Unsigned sizes are intentionally used to avoid integer overflow.

gpshead · 2022-09-28T17:31:23Z

There is no integer overflow here.

Clarification: are you suggesting there was never a problem in the first place or that this change looks good?

A sufficiently large newsize previously would have caused num_allocated_bytes passed to realloc to be too small.

serhiy-storchaka · 2022-09-28T17:38:35Z

At first glance, I saw that the existing code was already written to protect against integer overflow, so an explicit check seemed redundant. Then I read the issue and saw that it only protected against some cases of integer overflows. So the code in this PR is correct and necessary.

vstinner · 2022-09-28T18:18:28Z

See #97616 (comment) for an explanation of the integer overflow.

vstinner · 2022-09-28T19:42:37Z

Explicitly cast PY_SSIZE_T_MAX to size_t

I hate implicit conversion between signed and unsigned, it commonly emits compiler warnings. I prefer to only work on unsigned numbers there.

vstinner · 2022-09-28T19:52:03Z

I ran the added test on the main branch: it does crash as expected.

vstinner@mona$ ./python -m test -v test_list -m test_list_resize_overflow
...
test_list_resize_overflow (test.test_list.ListTest.test_list_resize_overflow) ... Fatal Python error: Segmentation fault

Current thread 0x00007faeb98da740 (most recent call first):
  File "/home/vstinner/python/main/Lib/test/test_list.py", line 110 in test_list_resize_overflow
  ...

It only crashs on the second test (lst *= size): tuple.__imul__() works in-place and so calls list_resize().

        size = ((2 ** (tuple.__itemsize__ * 8) - 1) // 2)
        with self.assertRaises((MemoryError, OverflowError)):
            lst * size
        with self.assertRaises((MemoryError, OverflowError)):
            lst *= size   # <=== HERE

The first one (lst * size) doesn't crash: tuple.__mul__() creates a new list, it doesn't call list_resize().

I prefer to test both cases.

Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)).

vstinner · 2022-09-28T20:32:43Z

Fix multiplying a list by an integer (list * int)

Well, the bug is on list *= int, I fixed the doc.

miss-islington · 2022-09-28T22:07:10Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8, 3.9, 3.10, 3.11.
🐍🍒⛏🤖

bedevere-bot · 2022-09-28T22:07:20Z

GH-97625 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2022-09-28T22:07:24Z

GH-97626 is a backport of this pull request to the 3.10 branch.

bedevere-bot · 2022-09-28T22:07:29Z

GH-97627 is a backport of this pull request to the 3.9 branch.

bedevere-bot · 2022-09-28T22:07:33Z

GH-97628 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2022-09-28T22:07:37Z

GH-97629 is a backport of this pull request to the 3.7 branch.

…7617) Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

…H-97627) gh-97616: list_resize() checks for integer overflow (GH-97617) Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

…H-97628) gh-97616: list_resize() checks for integer overflow (GH-97617) Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

…97629) Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

lazka · 2022-10-07T21:57:29Z

If such overflow checks are common in CPython you could consider using macros like in glib for example: https://developer-old.gnome.org/glib/stable/glib-Bounds-checked-integer-arithmetic.html

Fix multiplying a list by an integer (list *= int): detect the integer overflow when the new allocated length is close to the maximum size. Issue reported by Jordan Limor. list_resize() now checks for integer overflow before multiplying the new allocated length by the list item size (sizeof(PyObject*)). (cherry picked from commit a5f092f) Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner added needs backport to 3.7 needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 28, 2022

bedevere-bot added the awaiting core review label Sep 28, 2022

vstinner added the type-security A security issue label Sep 28, 2022

gpshead approved these changes Sep 28, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 28, 2022

serhiy-storchaka approved these changes Sep 28, 2022

View reviewed changes

vstinner merged commit a5f092f into python:main Sep 28, 2022

bedevere-bot removed the awaiting merge label Sep 28, 2022

vstinner deleted the list_resize branch September 28, 2022 22:07

bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 28, 2022

bedevere-bot removed needs backport to 3.10 only security fixes needs backport to 3.9 labels Sep 28, 2022

bedevere-bot removed the needs backport to 3.8 label Sep 28, 2022

bedevere-bot removed the needs backport to 3.7 label Sep 28, 2022

Uh oh!

Uh oh!

Conversation

vstinner commented Sep 28, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 28, 2022

Uh oh!

serhiy-storchaka commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Sep 28, 2022

Uh oh!

gpshead commented Sep 28, 2022

Uh oh!

serhiy-storchaka commented Sep 28, 2022

Uh oh!

vstinner commented Sep 28, 2022

Uh oh!

vstinner commented Sep 28, 2022

Uh oh!

vstinner commented Sep 28, 2022

Uh oh!

vstinner commented Sep 28, 2022

Uh oh!

miss-islington commented Sep 28, 2022

Uh oh!

bedevere-bot commented Sep 28, 2022

Uh oh!

bedevere-bot commented Sep 28, 2022

Uh oh!

bedevere-bot commented Sep 28, 2022

Uh oh!

bedevere-bot commented Sep 28, 2022

Uh oh!

bedevere-bot commented Sep 28, 2022

Uh oh!

lazka commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vstinner commented Sep 28, 2022 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Sep 28, 2022 •

edited

Loading