Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-97616: list_resize() checks for integer overflow #97617

Merged
merged 1 commit into from Sep 28, 2022
Merged

gh-97616: list_resize() checks for integer overflow #97617

merged 1 commit into from Sep 28, 2022
+24 −2

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 28, 2022 •
edited by bedevere-bot

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 vstinner added the type-security A security issue label Sep 28, 2022
@vstinner
Copy link
Member Author

vstinner commented Sep 28, 2022

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

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 28, 2022 •
edited

There is no integer overflow here.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 28, 2022

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

@gpshead
Copy link
Member

gpshead commented Sep 28, 2022

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
Copy link
Member

serhiy-storchaka commented Sep 28, 2022

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
Copy link
Member Author

vstinner commented Sep 28, 2022

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

@vstinner
Copy link
Member Author

vstinner commented Sep 28, 2022

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
Copy link
Member Author

vstinner commented Sep 28, 2022

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.

@vstinner
pythongh-97616: list_resize() checks for integer overflow
a5fd8ee 
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
Copy link
Member Author

vstinner commented Sep 28, 2022

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

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

@vstinner vstinner merged commit a5f092f into python:main Sep 28, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Sep 28, 2022

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

@vstinner vstinner deleted the list_resize branch Sep 28, 2022
@bedevere-bot
Copy link

bedevere-bot commented Sep 28, 2022

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 28, 2022

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 28, 2022

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 28, 2022

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 28, 2022

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 28, 2022
@vstinner @miss-islington
pythongh-97616: list_resize() checks for integer overflow (pythonGH-9…
f1d4eb9 
…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 a5f092f3c469b674b8d9ccbd4e4377230c9ac7cf)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 28, 2022
@vstinner @miss-islington
pythongh-97616: list_resize() checks for integer overflow (pythonGH-9…
4abcfcf 
…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 a5f092f3c469b674b8d9ccbd4e4377230c9ac7cf)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 28, 2022
@vstinner @miss-islington
pythongh-97616: list_resize() checks for integer overflow (pythonGH-9…
13a2e5d 
…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 a5f092f3c469b674b8d9ccbd4e4377230c9ac7cf)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 28, 2022
@vstinner @miss-islington
pythongh-97616: list_resize() checks for integer overflow (pythonGH-9…
1d6113d 
…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 a5f092f3c469b674b8d9ccbd4e4377230c9ac7cf)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 28, 2022
@vstinner @miss-islington
pythongh-97616: list_resize() checks for integer overflow (pythonGH-9…
80ebd95 
…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 a5f092f3c469b674b8d9ccbd4e4377230c9ac7cf)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington added a commit that referenced this pull request Sep 28, 2022
@miss-islington @vstinner
gh-97616: list_resize() checks for integer overflow (GH-97617)
7d60d10 
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>
miss-islington added a commit that referenced this pull request Sep 28, 2022
@miss-islington @vstinner
gh-97616: list_resize() checks for integer overflow (GH-97617)
28f1435 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
type-security A security issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vstinner @serhiy-storchaka @gpshead @miss-islington @bedevere-bot