Repository navigation
gh-127681: Fix overflow vulnerabilities in PyMem_Malloc calls - #153489
pranavchoudhary-tech wants to merge 3 commits into
Conversation
pranavchoudhary-tech
changed the title
|
This PR is stale because it has been open for 90 days with no activity. |
9b9d531 to
c473bb2
Compare
|
Rebased onto the latest main branch and resolved the merge conflict in Objects/capsule.c. All tests are passing. |
|
|
||
| oom: | ||
| if (src_info->format) { | ||
| PyMem_Free(dst_info->format); |
There was a problem hiding this comment.
There is a risk of a double free in the destructor. If you free it here, please set the variable to NULL.
| if (size == PY_SSIZE_T_MAX) { | ||
| PyErr_NoMemory(); | ||
| Py_DECREF(ub); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Since size is obtained from PyUnicode_AsWideCharString(), adding 1 cannot cause an overflow.
| else { | ||
| bytes_per_entry = 5; | ||
| } | ||
| if (code_len > (PY_SSIZE_T_MAX - 1) / bytes_per_entry) { |
There was a problem hiding this comment.
code_len and bytes_per_entry are int, not Py_ssize_t.
gpshead
left a comment
gpshead
left a comment
There was a problem hiding this comment.
overall this PR feels like an attempt to redo #127686 which was also incomplete and rejected (including suspiciously similar placements of TODO comments...). There are enough flaws in here I don't think this is worth working on. Instead of one large PR trying to cover issues of this form, smaller targeted ones aiming to make clear defensible improvements in individual files would be better.
| @@ -0,0 +1 @@ | |||
| Fix overflow vulnerabilities in PyMem_Malloc calls by systematically replacing them with PyMem_New or adding explicit PY_SSIZE_T_MAX checks. | |||
There was a problem hiding this comment.
these are not vulnerabilities. just bugs. "Fix potentially overflowing size computations on internal uses of PyMem_Malloc."
| PyMem_Free(dst_info->format); | ||
| } | ||
| if (src_info->shape) { | ||
| PyMem_Free(dst_info->shape); |
There was a problem hiding this comment.
this is also a double free as this can be reached when the format allocation failed.
| return -1; | ||
|
|
||
| if (!last_dim_is_contiguous(dest, src)) { | ||
| if (dest->shape[0] > PY_SSIZE_T_MAX / dest->itemsize) { |
There was a problem hiding this comment.
be defensive, itemsize could be 0.
| size_t s = strlen(src_info->format); | ||
| if (s >= (size_t)PY_SSIZE_T_MAX) { |
There was a problem hiding this comment.
you're checking strlen > PY_SSIZE_T_MAX in multiple places - this is impossible.
| size = PyBytes_GET_SIZE(ub); | ||
| if (size == PY_SSIZE_T_MAX) { |
|
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 |
|
Thank you both @gpshead and @methane for the thorough review and candid feedback. You are right. This PR tried to take on too much across disparate modules based on #127686, and in doing so introduced new issues (like the double-free risks in stgdict) while adding checks for conditions that cannot happen in practice. I agree with your assessment that an omnibus PR of this nature is the wrong approach. I will close this PR. If there are any genuine, isolated issues worth addressing in specific files down the line, I will handle them as small, targeted PRs with proper tests. Thanks again for your time and guidance. |
3 participants
Fixes #127681 by systematically replacing PyMem_Malloc calls involving arithmetic with PyMem_New or adding explicit PY_SSIZE_T_MAX checks, as well as adding safety notes to appropriately constrained variables.