Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Lib/test/test_itertools.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,22 @@ def keyfunc(obj):
keyfunc.skip = 1
self.assertRaises(ExpectedError, gulp, [None, None], keyfunc)

def test_groupby_reentrant_eq_does_not_crash(self):

class Key(bytearray):
seen = False
def __eq__(self, other):
if not Key.seen:
Key.seen = True
next(g)
return False

data = [Key(b"a"), Key(b"b")]

g = itertools.groupby(data)
next(g)
next(g) # must not segfault

def test_filter(self):
self.assertEqual(list(filter(isEven, range(6))), [0,2,4])
self.assertEqual(list(filter(None, [0,1,0,2,0])), [1,2])
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a crash in itertools.groupby that could occur when a user-defined
:meth:`~object.__eq__` method re-enters the iterator during key comparison.
13 changes: 12 additions & 1 deletion Modules/itertoolsmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -545,8 +545,19 @@ groupby_next(PyObject *op)
break;
else {
int rcmp;
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);
Comment on lines +548 to +559
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


rcmp = PyObject_RichCompareBool(gbo->tgtkey, gbo->currkey, Py_EQ);
if (rcmp == -1)
return NULL;
else if (rcmp == 0)
Expand Down
Loading