Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
15 changes: 15 additions & 0 deletions Lib/test/test_typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -5844,6 +5844,21 @@ class A:
with self.assertRaises(TypeError):
a[int]

def test_return_non_tuple_while_unpacking(self):
# GH-138497: GenericAlias objects didn't ensure that __typing_subst__ actually
# returned a tuple
class EvilTypeVar:
__typing_is_unpacked_typevartuple__ = True
def __typing_prepare_subst__(*_):
return None # any value
def __typing_subst__(*_):
return 42 # not tuple

evil = EvilTypeVar()
type type_alias[*_] = 0
with self.assertRaises(TypeError):
Copy link
Member

Choose a reason for hiding this comment

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

Could you please test an error message? No need to test the full match, just some unique pattern to ensure that this is an expected TypeError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

type_alias[evil][0]


class ClassVarTests(BaseTestCase):

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a crash when a generic object's ``__typing_subst__`` returns an object
that isn't a :class:`tuple`.
12 changes: 12 additions & 0 deletions Objects/genericaliasobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -525,12 +525,24 @@ _Py_subs_parameters(PyObject *self, PyObject *args, PyObject *parameters, PyObje
return NULL;
}
if (unpack) {
if (!PyTuple_Check(arg)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about tuple subclasses here? If so, do we want to test this? Or use PyTuple_CheckExact?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be fine to do either. It would technically be a breaking change to disallow tuple subclasses here, but I doubt anyone is actually doing that in practice.

Copy link
Member

Choose a reason for hiding this comment

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

It's an internal undocumented API so I wouldn't feel terrible about disallowing tuple subclasses, but we could go either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's just use PyTuple_Check for the time being. There's no drawback to keeping subclass support here, so we might as well keep it.

Py_DECREF(newargs);
Py_DECREF(item);
Py_XDECREF(tuple_args);
PyObject *original = PyTuple_GET_ITEM(args, iarg);
PyErr_Format(PyExc_TypeError,
"expected __typing_subst__ of %T objects to return a tuple, not %T",
original, arg);
Py_DECREF(arg);
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't comment there but are we leaking newargs on line 543 below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it. Do you want me to fix that in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, might as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

jarg = tuple_extend(&newargs, jarg,
&PyTuple_GET_ITEM(arg, 0), PyTuple_GET_SIZE(arg));
Py_DECREF(arg);
if (jarg < 0) {
Py_DECREF(item);
Py_XDECREF(tuple_args);
/* newargs was stolen */
return NULL;
}
}
Expand Down
Loading