Repository navigation
gh-145376: Fix various reference leaks objects and modules - #145385
vstinner merged 10 commits into
Conversation
Sorry, something went wrong.
|
Is this going to be a competition between Claude and Codex now 😆? |
Sorry, something went wrong.
As long as the competition is on quality and not quantity we should be fine :-) Update: I checked import.c which was changed in the PR by @JelleZijlstra. Claude found 4 (potential) issues, including the one in the other PR. |
Sorry, something went wrong.
|
It defintely shouldn't be on quantity of PRs. Could you combine these rather than send several PRs at once? |
Sorry, something went wrong.
I know some other core devs strongly prefer to keep PRs small and isolated. For that reason I created more PRs targeting only a single module at a time. To give an idea: analyzing the Modules folder resulted in 60 potential bugs in 30 files. I estimate that about 20 of them are false positives, or tricky cases. With more prompting to the LLM additional potential issues might be found. Let me know how you would like to group the remaining cases. |
Sorry, something went wrong.
|
IMO, you should limit the number of similar PRs you have “in flight”, so if a common issue is found in one of them1 you don't need to update all the others. And to focus the review in one place. Footnotes
|
Sorry, something went wrong.
| static PyObject * | ||
| count_nextlong(countobject *lz) | ||
| { | ||
| PyObject *long_cnt; | ||
| PyObject *stepped_up; | ||
|
|
||
| long_cnt = lz->long_cnt; | ||
| if (long_cnt == NULL) { | ||
| /* Switch to slow_mode */ | ||
| long_cnt = PyLong_FromSsize_t(PY_SSIZE_T_MAX); | ||
| if (long_cnt == NULL) | ||
| if (long_cnt == NULL) { | ||
| return NULL; | ||
| } | ||
| lz->long_cnt = long_cnt; | ||
| } | ||
| assert(lz->cnt == PY_SSIZE_T_MAX && long_cnt != NULL); | ||
|
|
||
| stepped_up = PyNumber_Add(long_cnt, lz->long_step); | ||
| if (stepped_up == NULL) | ||
| return NULL; | ||
| lz->long_cnt = stepped_up; | ||
| return long_cnt; | ||
| } |
There was a problem hiding this comment.
Is this the way to make it less confusing?
static PyObject *
count_nextlong(countobject *lz)
{
if (lz->long_cnt == NULL) {
/* Switch to slow_mode */
lz->long_cnt = PyLong_FromSsize_t(PY_SSIZE_T_MAX);
if (lz->long_cnt == NULL) {
return NULL;
}
}
assert(lz->cnt == PY_SSIZE_T_MAX && lz->long_cnt != NULL);
// We hold one reference to "result" (a.k.a. the old value of
// lz->long_cnt); we'll either return it or keep it in lz->long_cnt.
PyObject *result = lz->long_cnt;
PyObject *stepped_up = PyNumber_Add(result, lz->long_step);
if (stepped_up == NULL) {
return NULL;
}
lz->long_cnt = stepped_up;
return result;
}
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, this looks less confusing.
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Fix reference leaks in various unusual error scenarios. | |||
There was a problem hiding this comment.
I don't think that a NEWS entry is needed since it's unlikely to hit the issue, users are not directly impacted.
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
Merged. Thanks for the fixes! |
Sorry, something went wrong.
…python#145385) Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Found using Claude.