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.

Sign up for GitHub

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

bpo-35090: Fix potential division by zero and integer overflow in allocator wrappers #10174

Merged
merged 7 commits into from Oct 28, 2018
Merged

bpo-35090: Fix potential division by zero and integer overflow in allocator wrappers #10174

merged 7 commits into from Oct 28, 2018

Conversation

izbyshev
Copy link
Contributor

@izbyshev izbyshev commented Oct 28, 2018 •
edited by bedevere-bot

Reported by Svace static analyzer.

https://bugs.python.org/issue35090

@izbyshev
Copy link
Contributor Author

Thanks for the review, @ericvsmith. I've added a NEWS entry.

@serhiy-storchaka
Copy link
Member

Does BZ2_Malloc() even called with size=0?

@izbyshev
Copy link
Contributor Author

@serhiy-storchaka I don't know -- it depends on the implementation of the bz2 library CPython is linked to. But since BZ2_Malloc already checks for size < 0, ISTM it should check for size == 0 too.

@serhiy-storchaka
Copy link
Member

Since we have no evidence that BZ2_Malloc is called with size=0 and this change fixes an actual bug, I think it is better to not add a news entry.

I suppose that size is always a constant: 1, 2, 8, 512, etc. Perhaps even returning NULL for size=0 will not break any code.

@izbyshev
Revert "Add NEWS entry"
51ee58e 
This reverts commit e324296.
@izbyshev
Copy link
Contributor Author

@serhiy-storchaka OK, I removed it.

@serhiy-storchaka
Copy link
Member

There is yet one issue in BZ2_Malloc. Since arguments are of type int, their production is of type int. items * size should be replaced with (Py_ssize_t)items * (Py_ssize_t)size.

@izbyshev izbyshev changed the title bpo-35090: bz2: Fix potential division by zero in BZ2_Malloc() bpo-35090: bz2: Fix potential division by zero and integer overflow in BZ2_Malloc() Oct 28, 2018
@izbyshev
Copy link
Contributor Author

@serhiy-storchaka Ah, I missed it, shame on me. Thanks!

vstinner
vstinner reviewed Oct 28, 2018
View reviewed changes
Modules/_bz2module.c Outdated
return NULL;
/* PyMem_Malloc() cannot be used: compress() and decompress()
release the GIL */
return PyMem_RawMalloc(items * size);
return PyMem_RawMalloc((Py_ssize_t)items * (Py_ssize_t)size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum no, RawMalloc expects size_t. You have to cast to size_t instead to avoid undefined behavior on integer overflow (which cannot occur, but well, i'm pedantic, sorry!).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the types, but this change has nothing to do with undefined behavior -- as you say, it can't occur because we've explicitly ensured that it doesn't. This change is just for consistency with the code above which casts to size_t and with the type expected by PyRaw_Malloc.

@vstinner
Copy link
Member

I would prefer to have a single PR to fix the 3 memory allocators: https://bugs.python.org/issue35090#msg328693

@izbyshev izbyshev changed the title bpo-35090: bz2: Fix potential division by zero and integer overflow in BZ2_Malloc() bpo-35090: Fix potential division by zero and integer overflow in allocator wrappers Oct 28, 2018
vstinner
vstinner approved these changes Oct 28, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @serhiy-storchaka: would you mind to double check the PR?

@izbyshev
Copy link
Contributor Author

@vstinner, I've added fixes for other wrappers as you suggested.

@vstinner vstinner merged commit 3d4fabb into python:master Oct 28, 2018
@izbyshev izbyshev deleted the bpo-35090 branch October 28, 2018 17:07
@miss-islington
Copy link
Contributor

Thanks @izbyshev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @izbyshev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 28, 2018
@izbyshev @miss-islington
bpo-35090: Fix potential division by zero in allocator wrappers (pyth…
f3daed3 
…onGH-10174)

* Fix potential division by zero in BZ2_Malloc()
* Avoid division by zero in PyLzma_Malloc()
* Avoid division by zero and integer overflow in PyZlib_Malloc()

Reported by Svace static analyzer.
(cherry picked from commit 3d4fabb)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
@bedevere-bot
Copy link

GH-10198 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 Oct 28, 2018
@izbyshev @miss-islington
bpo-35090: Fix potential division by zero in allocator wrappers (pyth…
ea11829 
…onGH-10174)

* Fix potential division by zero in BZ2_Malloc()
* Avoid division by zero in PyLzma_Malloc()
* Avoid division by zero and integer overflow in PyZlib_Malloc()

Reported by Svace static analyzer.
(cherry picked from commit 3d4fabb)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
@bedevere-bot
Copy link

GH-10199 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit that referenced this pull request Oct 28, 2018
@miss-islington @izbyshev
bpo-35090: Fix potential division by zero in allocator wrappers (GH-1…
1d7d165 
…0174)

* Fix potential division by zero in BZ2_Malloc()
* Avoid division by zero in PyLzma_Malloc()
* Avoid division by zero and integer overflow in PyZlib_Malloc()

Reported by Svace static analyzer.
(cherry picked from commit 3d4fabb)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
miss-islington added a commit that referenced this pull request Oct 28, 2018
@miss-islington @izbyshev
bpo-35090: Fix potential division by zero in allocator wrappers (GH-1…
fd0a3bc 
…0174)

* Fix potential division by zero in BZ2_Malloc()
* Avoid division by zero in PyLzma_Malloc()
* Avoid division by zero and integer overflow in PyZlib_Malloc()

Reported by Svace static analyzer.
(cherry picked from commit 3d4fabb)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith ericvsmith left review comments

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@izbyshev @serhiy-storchaka @vstinner @miss-islington @bedevere-bot @ericvsmith @the-knights-who-say-ni