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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c69a602
gh-128632: fix segfault on nested __classdict__ type param
tom-pytel Jan 11, 2025
c7ec3ed
📜🤖 Added by blurb_it.
blurb-it[bot] Jan 11, 2025
8f813c1
requested changes
tom-pytel Jan 11, 2025
285be46
disallow __classcell__ and __classdictcell__
tom-pytel Jan 11, 2025
4d736e9
requested comment changes
tom-pytel Jan 12, 2025
841f738
Merge branch 'main' into fix-issue-128632
tom-pytel Jan 12, 2025
cee5fe4
faster attr name compare maybe
tom-pytel Jan 24, 2025
e76604e
Merge branch 'main' into fix-issue-128632
tom-pytel Jan 24, 2025
0a9bcf3
Merge branch 'main' into fix-issue-128632
tom-pytel Feb 12, 2025
bc1faff
Merge branch 'main' into fix-issue-128632
tom-pytel Feb 21, 2025
151624a
requested changes
tom-pytel Feb 22, 2025
3a7eb7a
Merge branch 'main' into fix-issue-128632
tom-pytel Feb 28, 2025
e2c44d7
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-11-20-11-28.gh-issu…
JelleZijlstra Mar 1, 2025
3d65ad1
Merge branch 'main' into fix-issue-128632
tom-pytel Mar 19, 2025
26bd8ef
Merge branch 'main' into fix-issue-128632
tom-pytel Apr 4, 2025
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: 12 additions & 0 deletions Lib/test/test_syntax.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2607,6 +2607,18 @@ 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.


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

@support.cpython_only
def test_nested_named_except_blocks(self):
code = ""
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-01-11-20-11-28.gh-issue-128632.ryhnKs.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix segfault on nested __classdict__ type param
15 changes: 8 additions & 7 deletions Python/assemble.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,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);
Expand All @@ -515,14 +515,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;
Expand All @@ -534,6 +534,7 @@ compute_localsplus_info(_PyCompile_CodeUnitMetadata *umd, int nlocalsplus,
assert(offset >= 0);
offset += nlocals - numdropped;
assert(offset < nlocalsplus);
assert(offset > cellvar_offset);
_Py_set_localsplus_info(offset, k, CO_FAST_FREE, names, kinds);
}
return SUCCESS;
Expand Down
3 changes: 3 additions & 0 deletions Python/codegen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3065,6 +3065,9 @@ codegen_addop_yield(compiler *c, location loc) {
return SUCCESS;
}

/* XXX: Currently if this is used to insert a new name into u_freevars when
there are already entries in u_cellvars then the wrong index will be put
into u_freevars causing a hard error downstream. */
static int
codegen_load_classdict_freevar(compiler *c, location loc)
{
Expand Down
3 changes: 3 additions & 0 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,9 @@ _PyCompile_ResolveNameop(compiler *c, PyObject *mangled, int scope,
break;
}
if (*optype != COMPILE_OP_FAST) {
/* XXX: Currently if this is used to insert a new name into u_freevars when
there are already entries in u_cellvars then the wrong index will be put
into u_freevars causing a hard error downstream. */
*arg = _PyCompile_DictAddObj(dict, mangled);
RETURN_IF_ERROR(*arg);
}
Expand Down
27 changes: 20 additions & 7 deletions Python/symtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2536,11 +2536,24 @@ 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_EqualToASCIIString(name, "__class__") ||
_PyUnicode_EqualToASCIIString(name, "__classdict__") ||
_PyUnicode_EqualToASCIIString(name, "__classcell__") ||
_PyUnicode_EqualToASCIIString(name, "__classdictcell__")) {

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;
}

Expand Down Expand Up @@ -2582,12 +2595,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;
Expand All @@ -2597,7 +2610,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;
Expand All @@ -2607,7 +2620,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;
Expand Down
Loading