gh-145376: Fix various reference leaks objects and modules

[eendebakpt]

Found using Claude.

• Issue: Various reference leaks in the core #145376

[StanFromIreland]

Is this going to be a competition between Claude and Codex now 😆?

[eendebakpt]

Is this going to be a competition between Claude and Codex now 😆?

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.

[encukou]

It defintely shouldn't be on quantity of PRs. Could you combine these rather than send several PRs at once?

[eendebakpt]

It defintely shouldn't be on quantity of PRs. Could you combine these rather than send several PRs at once?

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.

[encukou]

IMO, you should limit the number of similar PRs you have “in flight”, so if a common issue is found in one of them¹ you don't need to update all the others. And to focus the review in one place.
Queue up PRs on your side, or combine similar changes to multiple files, or even send drafts :)

---

Footnotes

1. for example: What needs a news entry? Or even: how much should you combine the PRs? ↩

[encukou]

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;
}

[sergey-miryanov]

Yeah, this looks less confusing.

[vstinner]

I don't think that a NEWS entry is needed since it's unlikely to hit the issue, users are not directly impacted.

[vstinner]

The current count_nextlong() code doesn't leak at if (long_cnt == NULL) return NULL; but it does leak long_cnt at if (stepped_up == NULL) return NULL;.

I like @encukou's code. It's more readable and doesn't leak on error.

[vstinner]

Can you refactor this function as you did for count_nextlong() to make it less confusing?