bpo-34824: Fix a possible NULL pointer dereference in _ssl.c #9606
Conversation
On failure, _PyBytes_Resize() will deallocate the bytes object and set "result" to NULL.
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
Good catch!
Please address the comment.
| nbytes = BIO_read(self->bio, PyBytes_AS_STRING(result), len); | ||
| /* There should never be any short reads but check anyway. */ | ||
| if ((nbytes < len) && (_PyBytes_Resize(&result, len) < 0)) { | ||
| Py_DECREF(result); |
There was a problem hiding this comment.
Let's rewrite the block to conform to the style of other _PyBytes_Resize calls. There is no need to check the return value of _PyBytes_Resize, because the function returns result any way.
if (nbytes < len) {
_PyBytes_Resize(&result, len);
}
|
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. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
The current code doesn't have any effect and errors in BIO_read() are not handled.
Modules/_ssl.c
Outdated
| Py_DECREF(result); | ||
| return NULL; | ||
| if (nbytes < len) { | ||
| _PyBytes_Resize(&result, len); |
There was a problem hiding this comment.
This operation doesn't have any effect. There should be nbytes instead of len. Note that nbytes can be negative, this denotes an error.
|
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. |
|
Thanks for making the requested changes! @serhiy-storchaka, @1st1, @tiran: please review the changes made to this pull request. |
| Py_DECREF(result); | ||
| if (nbytes < 0) { | ||
| _setSSLError(NULL, 0, __FILE__, __LINE__); | ||
| return NULL; |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @tiran, @serhiy-storchaka, @1st1: please review the changes made to this pull request. |
|
Thanks @ZackerySpytz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
…H-9606) On failure, _PyBytes_Resize() will deallocate the bytes object and set "result" to NULL. https://bugs.python.org/issue34824 (cherry picked from commit 365ad2e) Co-authored-by: Zackery Spytz <[email protected]>
|
GH-9743 is a backport of this pull request to the 3.7 branch. |
…H-9606) On failure, _PyBytes_Resize() will deallocate the bytes object and set "result" to NULL. https://bugs.python.org/issue34824 (cherry picked from commit 365ad2e) Co-authored-by: Zackery Spytz <[email protected]>
|
GH-9744 is a backport of this pull request to the 3.6 branch. |
GH-9743) On failure, _PyBytes_Resize() will deallocate the bytes object and set "result" to NULL. https://bugs.python.org/issue34824 (cherry picked from commit 365ad2e) Co-authored-by: Zackery Spytz <[email protected]>
GH-9744) On failure, _PyBytes_Resize() will deallocate the bytes object and set "result" to NULL. https://bugs.python.org/issue34824 (cherry picked from commit 365ad2e) Co-authored-by: Zackery Spytz <[email protected]>
7 participants
On failure, _PyBytes_Resize() will deallocate the bytes object and set
"result" to NULL.
https://bugs.python.org/issue34824