Skip to content

Commit e677b27

Browse files
colesburycolesbury
committed
pythongh-122527: Fix a crash on deallocation of PyStructSequence
The `PyStructSequence` destructor would crash if it was deallocated after its type's dictionary was cleared by the GC, because it couldn't compute the "real size" of the instance. This could occur with relatively straightforward code in the free-threaded build or with a reference cycle involving the type in the default build, due to differing orders in which `tp_clear()` was called. Account for the non-sequence fields in `tp_basicsize` and use that, along with `Py_SIZE()`, to compute the "real" size of a `PyStructSequence` in the dealloc function. This avoids the accesses to the type's dictionary during dealloc, which were unsafe.
1 parent fda6bd8 commit e677b27

4 files changed

Lines changed: 41 additions & 7 deletions

File tree

‎Lib/test/test_structseq.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
import os
33
import pickle
44
import re
5+
import textwrap
56
import time
67
import unittest
8+
from test.support import script_helper
79

810

911
class StructSeqTest(unittest.TestCase):
@@ -342,6 +344,17 @@ def test_copy_replace_with_unnamed_fields(self):
342344
with self.assertRaisesRegex(TypeError, error_message):
343345
copy.replace(r, st_mode=1, error=2)
344346

347+
def test_reference_cycle(self):
348+
# gh-122527: Check that a structseq that's part of a reference cycle
349+
# with its own type doesn't crash. Previously, if the type's dictionary
350+
# was cleared first, the structseq instance would crash in the
351+
# destructor.
352+
script_helper.assert_python_ok("-c", textwrap.dedent(r"""
353+
import time
354+
t = time.gmtime()
355+
type(t).refcyle = t
356+
"""))
357+
345358

346359
if __name__ == "__main__":
347360
unittest.main()

‎Lib/test/test_sys.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1823,7 +1823,8 @@ def test_pythontypes(self):
18231823
# symtable entry
18241824
# XXX
18251825
# sys.flags
1826-
check(sys.flags, vsize('') + self.P * len(sys.flags))
1826+
# FIXME: The +1 will not be necessary once gh-122575 is fixed
1827+
check(sys.flags, vsize('') + self.P * (1 + len(sys.flags)))
18271828

18281829
def test_asyncgen_hooks(self):
18291830
old = sys.get_asyncgen_hooks()

‎Misc/NEWS.d/next/Core_and_Builtins/2024-08-01-19-13-58.gh-issue-122527.eztso6.rst‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a crash that occurred when a ``PyStructSequence`` was deallocated after
2+
its type's dictionary was cleared by the GC. The type's
3+
:c:member:`~PyTypeObject.tp_basicsize` now accounts for non-sequence fields
4+
that aren't included in the :c:macro:`Py_SIZE` of the sequence.

‎Objects/structseq.c‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,20 @@ get_type_attr_as_size(PyTypeObject *tp, PyObject *name)
4141
get_type_attr_as_size(tp, &_Py_ID(n_sequence_fields))
4242
#define REAL_SIZE_TP(tp) \
4343
get_type_attr_as_size(tp, &_Py_ID(n_fields))
44-
#define REAL_SIZE(op) REAL_SIZE_TP(Py_TYPE(op))
44+
#define REAL_SIZE(op) get_real_size((PyObject *)op)
4545

4646
#define UNNAMED_FIELDS_TP(tp) \
4747
get_type_attr_as_size(tp, &_Py_ID(n_unnamed_fields))
4848
#define UNNAMED_FIELDS(op) UNNAMED_FIELDS_TP(Py_TYPE(op))
4949

50+
static Py_ssize_t
51+
get_real_size(PyObject *op)
52+
{
53+
// Compute the real size from the visible size (i.e., Py_SIZE()) and the
54+
// number of non-sequence fields accounted for in tp_basicsize.
55+
Py_ssize_t hidden = Py_TYPE(op)->tp_basicsize - offsetof(PyStructSequence, ob_item);
56+
return Py_SIZE(op) + hidden / sizeof(PyObject *);
57+
}
5058

5159
PyObject *
5260
PyStructSequence_New(PyTypeObject *type)
@@ -120,6 +128,9 @@ structseq_dealloc(PyStructSequence *obj)
120128
PyObject_GC_UnTrack(obj);
121129

122130
PyTypeObject *tp = Py_TYPE(obj);
131+
// gh-122527: We can't use REAL_SIZE_TP() or any macros that access the
132+
// type's dictionary here, because the dictionary may have already been
133+
// cleared by the garbage collector.
123134
size = REAL_SIZE(obj);
124135
for (i = 0; i < size; ++i) {
125136
Py_XDECREF(obj->ob_item[i]);
@@ -565,10 +576,14 @@ initialize_members(PyStructSequence_Desc *desc,
565576

566577
static void
567578
initialize_static_fields(PyTypeObject *type, PyStructSequence_Desc *desc,
568-
PyMemberDef *tp_members, unsigned long tp_flags)
579+
PyMemberDef *tp_members, Py_ssize_t n_members,
580+
unsigned long tp_flags)
569581
{
570582
type->tp_name = desc->name;
571-
type->tp_basicsize = sizeof(PyStructSequence) - sizeof(PyObject *);
583+
// Account for hidden members in tp_basicsize because they are not
584+
// included in the variable size.
585+
Py_ssize_t n_hidden = n_members - desc->n_in_sequence;
586+
type->tp_basicsize = sizeof(PyStructSequence) + (n_hidden - 1) * sizeof(PyObject *);
572587
type->tp_itemsize = sizeof(PyObject *);
573588
type->tp_dealloc = (destructor)structseq_dealloc;
574589
type->tp_repr = (reprfunc)structseq_repr;
@@ -621,7 +636,7 @@ _PyStructSequence_InitBuiltinWithFlags(PyInterpreterState *interp,
621636
if (members == NULL) {
622637
goto error;
623638
}
624-
initialize_static_fields(type, desc, members, tp_flags);
639+
initialize_static_fields(type, desc, members, n_members, tp_flags);
625640

626641
_Py_SetImmortal((PyObject *)type);
627642
}
@@ -684,7 +699,7 @@ PyStructSequence_InitType2(PyTypeObject *type, PyStructSequence_Desc *desc)
684699
if (members == NULL) {
685700
return -1;
686701
}
687-
initialize_static_fields(type, desc, members, 0);
702+
initialize_static_fields(type, desc, members, n_members, 0);
688703
if (initialize_static_type(type, desc, n_members, n_unnamed_members) < 0) {
689704
PyMem_Free(members);
690705
return -1;
@@ -760,7 +775,8 @@ _PyStructSequence_NewType(PyStructSequence_Desc *desc, unsigned long tp_flags)
760775
/* The name in this PyType_Spec is statically allocated so it is */
761776
/* expected that it'll outlive the PyType_Spec */
762777
spec.name = desc->name;
763-
spec.basicsize = sizeof(PyStructSequence) - sizeof(PyObject *);
778+
Py_ssize_t hidden = n_members - desc->n_in_sequence;
779+
spec.basicsize = sizeof(PyStructSequence) + (hidden - 1) * sizeof(PyObject *);
764780
spec.itemsize = sizeof(PyObject *);
765781
spec.flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | tp_flags;
766782
spec.slots = slots;

0 commit comments

Comments
 (0)