gh-143543: Fix re-entrant use-after-free in itertools.groupby

[VanshAgarwal24036]

Fix a use-after-free in itertools.groupby when a user-defined eq
re-enters next(groupby) during key comparison.

groupby_next() compared borrowed references to currkey and tgtkey using
PyObject_RichCompareBool(). A re-entrant eq could advance the iterator,
replace and decref those keys, leaving the outer comparison accessing
freed memory.

The fix temporarily INCREFs both keys for the duration of the comparison,
preventing re-entrancy from invalidating them. A regression test is added.

• Issue: Use-after-free in itertools.groupby via re-entrant key comparison through __eq__ #143543

[VanshAgarwal24036]

Skip News

[picnixz]

Skip News

No. This is a bug fix that is impacting end users. In the future, please let triagers decide which labels to use when the bot doesn't do it automatically.

[VanshAgarwal24036]

Thanks for the clarification — I’ve added a NEWS entry documenting the user-visible crash fix.

[skirpichev]

@VanshAgarwal24036, please see https://devguide.python.org/core-team/committing/#how-to-add-a-news-entry on how to properly add news entry.

[skirpichev]

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits. When the PR is merged, everything will be squashed into a single commit.

[skirpichev]

@VanshAgarwal24036, please avoid force pushes!

[VanshAgarwal24036]

Thanks for the clarification — understood. I won’t rewrite history or use force-pushes going forward and will only add new commits.

I’m still seeing bedevere/news failing in CI, and I want to make sure I’m following the expected workflow correctly. Would you prefer that I delete the current NEWS entry and re-add it using blurb add, or should I keep the existing file and adjust it further?

Please let me know the preferred approach and I’ll follow that.

[skirpichev]

Suggested change

	PyObject *tgtkey = gbo->tgtkey;
	PyObject *currkey = gbo->currkey;
	/* Hold strong references during comparison to prevent re-entrant __eq__
	from advancing the iterator and invalidating borrowed references. */
	Py_INCREF(tgtkey);
	Py_INCREF(currkey);
	rcmp = PyObject_RichCompareBool(tgtkey, currkey, Py_EQ);
	Py_DECREF(tgtkey);
	Py_DECREF(currkey);
	/* Hold strong references during comparison to prevent re-entrant __eq__
	from advancing the iterator and invalidating borrowed references. */
	Py_INCREF(gbo->tgtkey);
	Py_INCREF(gbo->currkey);
	rcmp = PyObject_RichCompareBool(tgtkey, currkey, Py_EQ);
	Py_DECREF(gbo->tgtkey);
	Py_DECREF(gbo->currkey);

Maybe you want also to remove a separate declaration of rcmp and add a comment, explaining why we do Py_INCREF here.

[VanshAgarwal24036]

Thanks for the suggestion!
I kept local snapshots of tgtkey / currkey instead, because a re-entrant eq can mutate gbo->tgtkey / gbo->currkey, and INCREF/DECREF must apply to the same objects.
This avoids refcount mismatches under ASan.