gh-143543: Fix re-entrant use-after-free in itertools.groupby #143738
Conversation
|
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. |
|
Thanks for the clarification — I’ve added a NEWS entry documenting the user-visible crash fix. |
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
@VanshAgarwal24036, please see https://devguide.python.org/core-team/committing/#how-to-add-a-news-entry on how to properly add news entry.
| PyObject *tgtkey = gbo->tgtkey; | ||
| PyObject *currkey = gbo->currkey; | ||
|
|
||
| Py_INCREF(tgtkey); | ||
| Py_INCREF(currkey); | ||
|
|
||
| rcmp = PyObject_RichCompareBool(tgtkey, currkey, Py_EQ); | ||
|
|
||
| Py_DECREF(tgtkey); | ||
| Py_DECREF(currkey); |
There was a problem hiding this comment.
| PyObject *tgtkey = gbo->tgtkey; | |
| PyObject *currkey = gbo->currkey; | |
| Py_INCREF(tgtkey); | |
| Py_INCREF(currkey); | |
| rcmp = PyObject_RichCompareBool(tgtkey, currkey, Py_EQ); | |
| Py_DECREF(tgtkey); | |
| Py_DECREF(currkey); | |
| 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
force-pushed
the
gh-143543-groupby-uaf
branch
from
3f86c12 to
0e86303
Compare
|
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. |
VanshAgarwal24036
force-pushed
the
gh-143543-groupby-uaf
branch
from
d509c2c to
740218c
Compare
|
@VanshAgarwal24036, please avoid force pushes! |
|
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. |
3 participants
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.
itertools.groupbyvia re-entrant key comparison through__eq__#143543