Skip to content

gh-128632: fix segfault on nested __classdict__ type param #128744

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 15 commits into from
Apr 4, 2025
Merged
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
19 changes: 19 additions & 0 deletions Lib/test/test_syntax.py
Original file line number Diff line number Diff line change
@@ -2690,6 +2690,25 @@ def test_continuation_bad_indentation(self):

self.assertRaises(IndentationError, exec, code)

@support.cpython_only
def test_disallowed_type_param_names(self):
# See gh-128632
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 also test the following related cases:

  • Using a generic function or type alias instead of a class with these names for the type parameters
  • Using the names __classcell__ and __classdictcell__, which appear internally in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added func and alias tests. Didn't add __classcell__ or __classdictcell__ tests as those are not excluded and don't cause problems currently. Do you want to add them to the forbidden list?

>>> class A:
...     class B[__classcell__]: print(type(__classcell__))
...     
<class 'typing.TypeVar'>
>>> class A:
...     class B[__classdictcell__]: print(type(__classdictcell__))
...     
<class 'typing.TypeVar'>

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the tests! I want the *cell* names to also be tested, because they're also somewhat likely to cause problems, even if they don't currently crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I interpret 'somewhat likely to cause problems' to mean yes disallow these names (and test them).

Copy link
Member

Choose a reason for hiding this comment

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

Since they work fine now we should not disallow them, especially because we'd want to backport this change. However, we should have a test to make sure future changes don't introduce crashes with these names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names re-allowed and tests added. Also re-allowed __class__ as that doesn't crash either even though it is special.


self._check_error(f"class A[__classdict__]: pass",
f"reserved name '__classdict__' cannot be used for type parameter")
self._check_error(f"def f[__classdict__](): pass",
f"reserved name '__classdict__' cannot be used for type parameter")
self._check_error(f"type T[__classdict__] = tuple[__classdict__]",
f"reserved name '__classdict__' cannot be used for type parameter")

# These compilations are here to make sure __class__, __classcell__ and __classdictcell__
# don't break in the future like __classdict__ did in this case.
for name in ('__class__', '__classcell__', '__classdictcell__'):
compile(f"""
class A:
class B[{name}]: pass
""", "<testcase>", mode="exec")

@support.cpython_only
def test_nested_named_except_blocks(self):
code = ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Disallow ``__classdict__`` as the name of a type parameter. Using this
name would previously crash the interpreter in some circumstances.
18 changes: 11 additions & 7 deletions Python/assemble.c
Original file line number Diff line number Diff line change
@@ -516,7 +516,7 @@ compute_localsplus_info(_PyCompile_CodeUnitMetadata *umd, int nlocalsplus,
int nlocals = (int)PyDict_GET_SIZE(umd->u_varnames);

// This counter mirrors the fix done in fix_cell_offsets().
int numdropped = 0;
int numdropped = 0, cellvar_offset = -1;
pos = 0;
while (PyDict_Next(umd->u_cellvars, &pos, &k, &v)) {
int has_name = PyDict_Contains(umd->u_varnames, k);
@@ -527,14 +527,14 @@ compute_localsplus_info(_PyCompile_CodeUnitMetadata *umd, int nlocalsplus,
continue;
}

int offset = PyLong_AsInt(v);
if (offset == -1 && PyErr_Occurred()) {
cellvar_offset = PyLong_AsInt(v);
if (cellvar_offset == -1 && PyErr_Occurred()) {
return ERROR;
}
assert(offset >= 0);
offset += nlocals - numdropped;
assert(offset < nlocalsplus);
_Py_set_localsplus_info(offset, k, CO_FAST_CELL, names, kinds);
assert(cellvar_offset >= 0);
cellvar_offset += nlocals - numdropped;
assert(cellvar_offset < nlocalsplus);
_Py_set_localsplus_info(cellvar_offset, k, CO_FAST_CELL, names, kinds);
}

pos = 0;
@@ -546,6 +546,10 @@ compute_localsplus_info(_PyCompile_CodeUnitMetadata *umd, int nlocalsplus,
assert(offset >= 0);
offset += nlocals - numdropped;
assert(offset < nlocalsplus);
/* XXX If the assertion below fails it is most likely because a freevar
was added to u_freevars with the wrong index due to not taking into
account cellvars already present, see gh-128632. */
assert(offset > cellvar_offset);
_Py_set_localsplus_info(offset, k, CO_FAST_FREE, names, kinds);
}
return SUCCESS;
22 changes: 16 additions & 6 deletions Python/symtable.c
Original file line number Diff line number Diff line change
@@ -2569,11 +2569,21 @@ symtable_visit_expr(struct symtable *st, expr_ty e)
static int
symtable_visit_type_param_bound_or_default(
struct symtable *st, expr_ty e, identifier name,
void *key, const char *ste_scope_info)
type_param_ty tp, const char *ste_scope_info)
{
if (_PyUnicode_Equal(name, &_Py_ID(__classdict__))) {

PyObject *error_msg = PyUnicode_FromFormat("reserved name '%U' cannot be "
"used for type parameter", name);
PyErr_SetObject(PyExc_SyntaxError, error_msg);
Py_DECREF(error_msg);
SET_ERROR_LOCATION(st->st_filename, LOCATION(tp));
return 0;
}

if (e) {
int is_in_class = st->st_cur->ste_can_see_class_scope;
if (!symtable_enter_block(st, name, TypeVariableBlock, key, LOCATION(e))) {
if (!symtable_enter_block(st, name, TypeVariableBlock, (void *)tp, LOCATION(e))) {
return 0;
}

@@ -2615,12 +2625,12 @@ symtable_visit_type_param(struct symtable *st, type_param_ty tp)
// The only requirement for the key is that it is unique and it matches the logic in
// compile.c where the scope is retrieved.
if (!symtable_visit_type_param_bound_or_default(st, tp->v.TypeVar.bound, tp->v.TypeVar.name,
(void *)tp, ste_scope_info)) {
tp, ste_scope_info)) {
return 0;
}

if (!symtable_visit_type_param_bound_or_default(st, tp->v.TypeVar.default_value, tp->v.TypeVar.name,
(void *)((uintptr_t)tp + 1), "a TypeVar default")) {
(type_param_ty)((uintptr_t)tp + 1), "a TypeVar default")) {
return 0;
}
break;
@@ -2630,7 +2640,7 @@ symtable_visit_type_param(struct symtable *st, type_param_ty tp)
}

if (!symtable_visit_type_param_bound_or_default(st, tp->v.TypeVarTuple.default_value, tp->v.TypeVarTuple.name,
(void *)tp, "a TypeVarTuple default")) {
tp, "a TypeVarTuple default")) {
return 0;
}
break;
@@ -2640,7 +2650,7 @@ symtable_visit_type_param(struct symtable *st, type_param_ty tp)
}

if (!symtable_visit_type_param_bound_or_default(st, tp->v.ParamSpec.default_value, tp->v.ParamSpec.name,
(void *)tp, "a ParamSpec default")) {
tp, "a ParamSpec default")) {
return 0;
}
break;
Loading