Skip to content

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

Merged
vstinner merged 7 commits intopython:masterfrom
izbyshev:bpo-35090
Oct 28, 2018
Merged

bpo-35090: Fix potential division by zero and integer overflow in allocator wrappers#10174
vstinner merged 7 commits intopython:masterfrom
izbyshev:bpo-35090

Conversation

@izbyshev
Copy link
Copy Markdown
Contributor

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

Reported by Svace static analyzer.

https://bugs.python.org/issue35090

ericvsmith
ericvsmith reviewed Oct 28, 2018
View reviewed changes
@izbyshev
Copy link
Copy Markdown
Contributor Author

izbyshev commented Oct 28, 2018

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

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Oct 28, 2018

Does BZ2_Malloc() even called with size=0?

@izbyshev
Copy link
Copy Markdown
Contributor Author

izbyshev commented Oct 28, 2018

@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
Copy Markdown
Member

serhiy-storchaka commented Oct 28, 2018

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.

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

izbyshev commented Oct 28, 2018

@serhiy-storchaka OK, I removed it.

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Oct 28, 2018

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
Copy Markdown
Contributor Author

izbyshev commented Oct 28, 2018

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

vstinner
vstinner reviewed Oct 28, 2018
View reviewed changes
Modules/_bz2module.c Outdated
/* 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
Copy Markdown
Member

@vstinner vstinner Oct 28, 2018

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
Copy Markdown
Contributor Author

@izbyshev izbyshev Oct 28, 2018

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

vstinner commented Oct 28, 2018

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
Copy Markdown
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
Copy Markdown
Contributor Author

izbyshev commented Oct 28, 2018

@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
Copy Markdown
Contributor

miss-islington commented Oct 28, 2018

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
Copy Markdown
Contributor

miss-islington commented Oct 28, 2018

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
@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
Copy Markdown

bedevere-bot commented Oct 28, 2018

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
@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
Copy Markdown

bedevere-bot commented Oct 28, 2018

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
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
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.

7 participants

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