Skip to content

gh-101765: Fix SystemError / segmentation fault in iter __reduce__ when internal access of builtins.__dict__ exhausts the iterator #101769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Feb 24, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f256afe
Fix undefined behavior in listiter_reduce from _PyEval_GetBuiltin sid…
ionite34 Feb 10, 2023
6b4faad
Update comment to not mention `__eq__`
ionite34 Feb 10, 2023
a6d6211
Fix undefined behavior in iter_reduce and calliter_reduce
ionite34 Feb 10, 2023
4ccf427
Update listiter_reduce_general comment
ionite34 Feb 10, 2023
c2c9cfb
Fix undefined behavior in bytearrayiter_reduce from _PyEval_GetBuilti…
ionite34 Feb 10, 2023
e2989d9
Fix undefined behavior in striter_reduce from _PyEval_GetBuiltin side…
ionite34 Feb 10, 2023
71960a8
Fix undefined behavior in tupleiter_reduce from _PyEval_GetBuiltin si…
ionite34 Feb 10, 2023
efa0540
Move iter call in unicodeiter_reduce before `it` pointer access due t…
ionite34 Feb 10, 2023
c5abb14
Add iter reduce tests for issue #101765
ionite34 Feb 10, 2023
45522c6
Remove C++ reference in comments
ionite34 Feb 10, 2023
4f5fc19
Remove C++ reference in comments
ionite34 Feb 10, 2023
049a8dd
Move builtin declarations inside if blocks
ionite34 Feb 10, 2023
ef4f955
Move _PyEval_GetBuiltin before gi checks, add gi NULL check in ga_ite…
ionite34 Feb 10, 2023
7d4afb0
Update iter reduce mutating tests for generic alias
ionite34 Feb 10, 2023
8e4418d
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 10, 2023
d8ced8e
Fix backticks format for news
ionite34 Feb 10, 2023
178b8ea
Refactor iter reduce builtins mutation tests
ionite34 Feb 10, 2023
49ba8c3
Update iter mutating builtins test to include reversed iterator for l…
ionite34 Feb 10, 2023
93854e1
Add comment in unicodeiter_reduce for moving iter call before it poin…
ionite34 Feb 10, 2023
98ec3c6
Change test `__builtins__` to builtins import
ionite34 Feb 10, 2023
e661495
Change NEWS blurb phrasing
ionite34 Feb 10, 2023
19ab9c6
Update iter reduce mutating builtins test comments and simplify logic
ionite34 Feb 10, 2023
9b664c2
Update comments to better reflect issue
ionite34 Feb 24, 2023
c67b11a
Clarify test comments
ionite34 Feb 24, 2023
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
12 changes: 9 additions & 3 deletions Objects/bytearrayobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2391,11 +2391,17 @@ PyDoc_STRVAR(length_hint_doc,
static PyObject *
bytearrayiter_reduce(bytesiterobject *it, PyObject *Py_UNUSED(ignored))
{
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

if (it->it_seq != NULL) {
return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
} else {
return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
return Py_BuildValue("N(())", iter);
}
}

Expand Down
12 changes: 9 additions & 3 deletions Objects/bytesobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3169,11 +3169,17 @@ PyDoc_STRVAR(length_hint_doc,
static PyObject *
striter_reduce(striterobject *it, PyObject *Py_UNUSED(ignored))
{
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

if (it->it_seq != NULL) {
return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
} else {
return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
return Py_BuildValue("N(())", iter);
}
}

Expand Down
24 changes: 18 additions & 6 deletions Objects/iterobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,17 @@ PyDoc_STRVAR(length_hint_doc, "Private method returning an estimate of len(list(
static PyObject *
iter_reduce(seqiterobject *it, PyObject *Py_UNUSED(ignored))
{
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

if (it->it_seq != NULL)
return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
else
return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
return Py_BuildValue("N(())", iter);
}

PyDoc_STRVAR(reduce_doc, "Return state information for pickling.");
Expand Down Expand Up @@ -239,11 +245,17 @@ calliter_iternext(calliterobject *it)
static PyObject *
calliter_reduce(calliterobject *it, PyObject *Py_UNUSED(ignored))
{
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

if (it->it_callable != NULL && it->it_sentinel != NULL)
return Py_BuildValue("N(OO)", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_callable, it->it_sentinel);
return Py_BuildValue("N(OO)", iter, it->it_callable, it->it_sentinel);
else
return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
return Py_BuildValue("N(())", iter);
}

static PyMethodDef calliter_methods[] = {
Expand Down
15 changes: 11 additions & 4 deletions Objects/listobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3443,19 +3443,26 @@ static PyObject *
listiter_reduce_general(void *_it, int forward)
{
PyObject *list;
PyObject *iter;
PyObject *reversed;

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `_it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

/* the objects are not the same, index is of different types! */
if (forward) {
iter = _PyEval_GetBuiltin(&_Py_ID(iter));
_PyListIterObject *it = (_PyListIterObject *)_it;
if (it->it_seq) {
return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
}
} else {
reversed = _PyEval_GetBuiltin(&_Py_ID(reversed));
listreviterobject *it = (listreviterobject *)_it;
if (it->it_seq) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it_seq is NULL then reversed is leaked here, _PyEval_GetBuiltin returns a strong reference.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, but doesn't that mean the old code also leaked references? I'll prepare a PR to fix this now.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, the N code to Py_BuildValue steals a reference, so the previous code was right in terms of refcounting.

return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(reversed)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", reversed, it->it_seq, it->it_index);
}
}
/* empty iterator, create an empty list */
Expand Down
12 changes: 9 additions & 3 deletions Objects/tupleobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1048,11 +1048,17 @@ PyDoc_STRVAR(length_hint_doc, "Private method returning an estimate of len(list(
static PyObject *
tupleiter_reduce(_PyTupleIterObject *it, PyObject *Py_UNUSED(ignored))
{
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));

/* _PyEval_GetBuiltin can invoke arbitrary code.
* calls must be *before* access of `it` pointers,
* since C/C++ parameter eval order is undefined.
* see issue #101765 */

if (it->it_seq)
return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
it->it_seq, it->it_index);
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
else
return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
return Py_BuildValue("N(())", iter);
}

static PyObject *
Expand Down