Types with Py_TPFLAGS_MANAGED_WEAKREF but not Py_TPFLAGS_HAVE_GC crash when creating a weak reference

[chris-se]

Crash report

What happened?

Take the following simple example C extension:

#define PY_SSIZE_T_CLEAN
#include <Python.h>

typedef struct {
    PyObject_HEAD
    /* Type-specific fields go here. */
} CustomObject;

static PyTypeObject CustomType = {
    .ob_base = PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "weak_bug_repro.Custom",
    .tp_doc = PyDoc_STR("Custom objects"),
    .tp_basicsize = sizeof(CustomObject),
    .tp_itemsize = 0,
    .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_MANAGED_WEAKREF,
    .tp_new = PyType_GenericNew,
};

static int
weak_bug_repro_exec(PyObject* m)
{
    if (PyType_Ready(&CustomType) < 0) {
        return -1;
    }

    if (PyModule_AddObjectRef(m, "Custom", (PyObject *) &CustomType) < 0) {
        return -1;
    }

    return 0;
}

static PyModuleDef_Slot weak_bug_repro_module_slots[] = {
    {Py_mod_exec, weak_bug_repro_exec},
    {0, NULL}
};

static PyModuleDef weak_bug_repro_module = {
    .m_base = PyModuleDef_HEAD_INIT,
    .m_name = "weak_bug_repro",
    .m_size = 0,
    .m_slots = weak_bug_repro_module_slots,
};

PyMODINIT_FUNC
PyInit_weak_bug_repro(void)
{
    return PyModuleDef_Init(&weak_bug_repro_module);
}

After compiling, running the following Python code will crash:

import weak_bug_repro
import weakref

obj = weak_bug_repro.Custom()
ref = weakref.ref(obj)

Backtrace for the crash (via gdb on Linux, although it also crashes on macOS, and probably any platform):

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7a0b14b in get_basic_refs (head=0x2000000000000000, refp=refp@entry=0x7fffffffd298, proxyp=proxyp@entry=0x7fffffffd290) at Objects/weakrefobject.c:283
283         if (head != NULL && head->wr_callback == NULL) {
(gdb) bt
#0  0x00007ffff7a0b14b in get_basic_refs (head=0x2000000000000000, refp=refp@entry=0x7fffffffd298, proxyp=proxyp@entry=0x7fffffffd290) at Objects/weakrefobject.c:283
#1  0x00007ffff7a0b6b6 in try_reuse_basic_ref (list=<optimized out>, type=type@entry=0x7ffff7de9ae0 <_PyWeakref_RefType>, callback=callback@entry=0x0) at Objects/weakrefobject.c:338
#2  0x00007ffff7a0b8d2 in get_or_create_weakref (type=type@entry=0x7ffff7de9ae0 <_PyWeakref_RefType>, obj=0x7ffff6e55650, callback=0x0) at Objects/weakrefobject.c:428
#3  0x00007ffff7a0b956 in weakref___new__ (type=0x7ffff7de9ae0 <_PyWeakref_RefType>, args=<optimized out>, kwargs=<optimized out>) at Objects/weakrefobject.c:467
#4  0x00007ffff79b529d in type_call (self=0x7ffff7de9ae0 <_PyWeakref_RefType>, args=0x7ffff6e78780, kwds=0x0) at Objects/typeobject.c:2291
#5  0x00007ffff7926168 in _PyObject_MakeTpCall (tstate=tstate@entry=0x7ffff7e57200 <_PyRuntime+331232>, callable=callable@entry=0x7ffff7de9ae0 <_PyWeakref_RefType>, args=args@entry=0x7fffffffd608, nargs=<optimized out>, 
    keywords=keywords@entry=0x0) at Objects/call.c:242
#6  0x00007ffff792639b in _PyObject_VectorcallTstate (tstate=0x7ffff7e57200 <_PyRuntime+331232>, callable=callable@entry=0x7ffff7de9ae0 <_PyWeakref_RefType>, args=args@entry=0x7fffffffd608, nargsf=<optimized out>, 
    nargsf@entry=9223372036854775809, kwnames=kwnames@entry=0x0) at ./Include/internal/pycore_call.h:167
#7  0x00007ffff7926415 in PyObject_Vectorcall (callable=callable@entry=0x7ffff7de9ae0 <_PyWeakref_RefType>, args=args@entry=0x7fffffffd608, nargsf=9223372036854775809, kwnames=kwnames@entry=0x0) at Objects/call.c:327
#8  0x00007ffff7a5cd9b in _PyEval_EvalFrameDefault (tstate=<optimized out>, frame=0x7ffff7fb7020, throwflag=0) at Python/generated_cases.c.h:1619
#9  0x00007ffff7a7b128 in _PyEval_EvalFrame (tstate=tstate@entry=0x7ffff7e57200 <_PyRuntime+331232>, frame=frame@entry=0x7ffff7fb7020, throwflag=throwflag@entry=0) at ./Include/internal/pycore_ceval.h:119
#10 0x00007ffff7a7b2e8 in _PyEval_Vector (tstate=tstate@entry=0x7ffff7e57200 <_PyRuntime+331232>, func=func@entry=0x7ffff6e66750, locals=locals@entry=0x7ffff6e74050, args=args@entry=0x0, argcount=argcount@entry=0, 
    kwnames=kwnames@entry=0x0) at Python/ceval.c:1961
#11 0x00007ffff7a7b3c5 in PyEval_EvalCode (co=co@entry=0x7ffff6fca260, globals=globals@entry=0x7ffff6e74050, locals=locals@entry=0x7ffff6e74050) at Python/ceval.c:853
#12 0x00007ffff7aeda93 in run_eval_code_obj (tstate=tstate@entry=0x7ffff7e57200 <_PyRuntime+331232>, co=co@entry=0x7ffff6fca260, globals=globals@entry=0x7ffff6e74050, locals=locals@entry=0x7ffff6e74050) at Python/pythonrun.c:1365
#13 0x00007ffff7aedc47 in run_mod (mod=mod@entry=0x555555680260, filename=filename@entry=0x7ffff6eed9a0, globals=globals@entry=0x7ffff6e74050, locals=locals@entry=0x7ffff6e74050, flags=flags@entry=0x7fffffffd9d8, 
    arena=arena@entry=0x7ffff6ee4040, interactive_src=0x0, generate_new_source=0) at Python/pythonrun.c:1436
#14 0x00007ffff7aee4ab in pyrun_file (fp=fp@entry=0x55555555b650, filename=filename@entry=0x7ffff6eed9a0, start=start@entry=257, globals=globals@entry=0x7ffff6e74050, locals=locals@entry=0x7ffff6e74050, closeit=closeit@entry=1, 
    flags=0x7fffffffd9d8) at Python/pythonrun.c:1293
#15 0x00007ffff7aefcd6 in _PyRun_SimpleFileObject (fp=fp@entry=0x55555555b650, filename=filename@entry=0x7ffff6eed9a0, closeit=closeit@entry=1, flags=flags@entry=0x7fffffffd9d8) at Python/pythonrun.c:521
#16 0x00007ffff7aefec7 in _PyRun_AnyFileObject (fp=fp@entry=0x55555555b650, filename=filename@entry=0x7ffff6eed9a0, closeit=closeit@entry=1, flags=flags@entry=0x7fffffffd9d8) at Python/pythonrun.c:81
#17 0x00007ffff7b17aef in pymain_run_file_obj (program_name=program_name@entry=0x7ffff6eeda10, filename=filename@entry=0x7ffff6eed9a0, skip_source_first_line=0) at Modules/main.c:410
#18 0x00007ffff7b17bff in pymain_run_file (config=config@entry=0x7ffff7e222b8 <_PyRuntime+114328>) at Modules/main.c:429
#19 0x00007ffff7b186cf in pymain_run_python (exitcode=exitcode@entry=0x7fffffffdb3c) at Modules/main.c:694
#20 0x00007ffff7b18910 in Py_RunMain () at Modules/main.c:775
#21 0x00007ffff7b1896b in pymain_main (args=args@entry=0x7fffffffdb80) at Modules/main.c:805
#22 0x00007ffff7b189eb in Py_BytesMain (argc=<optimized out>, argv=<optimized out>) at Modules/main.c:829
#23 0x0000555555555142 in main (argc=<optimized out>, argv=<optimized out>) at ./Programs/python.c:15

I've initially seen this in Python 3.12 (where Py_TPFLAGS_MANAGED_WEAKREF was introduced), but I've just tested it against the current main branch, and it still crashes there.

The reason for the crash is the following:

• PyType_GenericAlloc (default tp_alloc of all types) allocates the object by calling _PyType_AllocNoTrack (Objects/typeobject.c)
• _PyType_AllocNoTrack uses _PyType_PreHeaderSize to determine how many bytes to allocate in front of the PyObject structure (Objects/typeobject.c)
• _PyType_PreHeaderSize returns the size of 2 pointers if either Py_TPFLAGS_MANAGED_WEAKREF or Py_TPFLAGS_MANAGED_DICT is a flag plus sizeof(PyGC_Head) if the object supports GC traversal, which is also the size of 2 pointers (technically 2 times uintptr_) (Include/internal/pycore_object.h)
• However, MANAGED_WEAKREF_OFFSET is hard-defined to be (((Py_ssize_t)sizeof(PyObject *))*-4), which is only correct if the PyGC_Head struct is also present, and that is put into tp_weaklistoffset of the object
• And GET_WEAKREFS_LISTPTR used in Objects/weakrefobject.c for creating a weak reference then reads from memory that comes before the area that malloc() allocated from, causing an invalid memory read, causing the crash

In Python 3.12 this would always crash, in 3.13+ this would only crash if GIL is not disabled at compile time, because Include/internal/pycore_typeobject.h defines MANAGED_WEAKREF_OFFSET to be (((Py_ssize_t)sizeof(PyObject *))*-2) if Py_GIL_DISABLED is defined.

The correct behavior should be be to set the tp_weaklistoffset correctly according to whether _PyType_IS_GC is true or not - but since there are several explicit checks for tp_weaklistoffset against the constant MANAGED_WEAKREF_OFFSET, this will probably require some level of refactoring.

CPython versions tested on:

3.12

Operating systems tested on:

No response

Output from running 'python -VV' on the command line:

No response

Linked PRs

• gh-134786: Py_TPFLAGS_MANAGED_WEAKREF and Py_TPFLAGS_MANAGED_DICT must be used only if Py_TPFLAGS_HAVE_GC set #135863

[sergey-miryanov]

I propose to change requirements for Py_TPFLAGS_MANAGED_WEAKREF and also force checking of having Py_TPFLAGS_HAVE_GC set for Py_TPFLAGS_MANAGED_WEAKREF and Py_TPFLAGS_MANAGED_DICT.

Please take a look at linked PR.

[neonene]

Although Py_TPFLAGS_MANAGED_* are not limited C API, some docs have encouraged the flags since 3.12, and the tutorial has required Py_TPFLAGS_MANAGED_WEAKREF on a static type instead of setting tp_weaklistoffset. Then, MANAGED_WEAKREF_OFFSET should be more flexible to be consistent.

Py_TPFLAGS_MANAGED_DICT has little to do with this issue, as it requires a heaptype and the type is now supposed to be used with the gc protocol.

[sergey-miryanov]

@neonene Yeah, Py_TPFLAGS_MANAGED_DICT doesn't relate to this issue. But if we omit Py_TPFLAGS_HAVE_GC when use Py_TPFLAGS_MANAGED_DICT then we also can get segfault. I propose to add checking of presence of the gc flag for both cases for dict and weakref.

[sergey-miryanov]

In some sense it is breaking change. I understand.

[neonene]

I propose to add checking of presence of the gc flag for both cases for dict and weakref.

Just to avoid a crash, I would use NotImplementedError keeping the design unchanged.

[sergey-miryanov]

@neonene I'm not sure I get you. Can you elaborate?

[neonene]

Static types are not discouraged from using tp_dictoffset since the alternative is not available:

cpython/Doc/c-api/typeobj.rst

Lines 1845 to 1848 in 033aa5c

	.. c:member:: Py_ssize_t PyTypeObject.tp_dictoffset
	While this field is still supported, :c:macro:`Py_TPFLAGS_MANAGED_DICT` should be
	used instead, if at all possible.

Even on heap types without the GC flag, authors should go back to the tp slot because the Py_TPFLAGS_MANAGED_DICT handling can be considered to be not implemented yet with very low priority. Raising an exception to work around a possible crash seems fine when missing Py_TPFLAGS_HAVE_GC, but requiring/recommending the flag in the message/doc will make the things unnecessarily unrecoverable. Also, there must be a strong reason the author avoids the GC. So, I would use NotImplementedError as "under implementation" without touching the coredevs' original design as much as possible, including the documentation.

The same applies more seriously to the Py_TPFLAGS_MANAGED_WEAKREF case. As for the priority of the non-GC version of MANAGED_WEAKREF_OFFSET, I just rely on coredevs.

[sergey-miryanov]

I mostly agree with you. Some clarification to be sure we speak the same.
Py_TPFLAGS_MANAGED_DICT implies Py_TPFLAGS_HAVE_GC and this is documented. If we don't want to raise a TypeError exception, then we can add an assert instead.

Py_TPFLAGS_MANAGED_WEAKREF not officially implies Py_TPFLAGS_HAVE_GC at this time, but code written as it should imply gc-flag.

[neonene]

Py_TPFLAGS_MANAGED_DICT implies Py_TPFLAGS_HAVE_GC and this is documented.

Yes. Keep the original remarks as-is. Py_TPFLAGS_HAVE_GC is recommended by using "should," since Py_TPFLAGS_MANAGED_DICT accepts only heaptypes as I said above.
(EDIT: since the dict can make reference cycles, strictly?)

However, your PR enforces the gc flag in some parts (title, description, NEWS, code). The OP has pointed out the inconsistency (should vs. must) there. Are you aware of the problem?

If we don't want to raise a TypeError exception, then we can add an assert instead.

The PR raises SystemError, and my suggestion is replacing it with NotImplementedError. Assertion cannot avoid a crash in non-debug builds, which should be against your concern.

Py_TPFLAGS_MANAGED_WEAKREF not officially implies Py_TPFLAGS_HAVE_GC at this time, but code written as it should imply gc-flag.

No. This issue is mainly about the static types in 3rd party extension modules, where Py_TPFLAGS_MANAGED_WEAKREF is highly encouraged.

[sergey-miryanov]

Thanks for clarification!

I'm not sure I get thin difference between should and must here.

cpython/Doc/c-api/typeobj.rst

Lines 1205 to 1212 in 033aa5c

	.. c:macro:: Py_TPFLAGS_MANAGED_DICT
	This bit indicates that instances of the class have a `~object.__dict__`
	attribute, and that the space for the dictionary is managed by the VM.
	If this flag is set, :c:macro:`Py_TPFLAGS_HAVE_GC` should also be set.
	.. versionadded:: 3.12

I read this as if Py_TPFLAGS_HAVE_GC flag is not set then Py_TPFLAGS_MANAGED_DICT will not work properly and bad things will happen. As I see Py_TPFLAGS_MANAGED_DICT maybe used with static types too, not only with heaptypes.

I don't believe NotImplementedError suitable here (anyway I open for any changes). I read it as it is not implemented now but may be implemented in the future. I'm not sure it will be implemented in such way. But who knows.

The same I want to add to Py_TPFLAGS_MANAGED_WEAKREF or get direction from coredevs that we should fix implementation.

[chris-se]

From my perspective, as the one who ran into this issue and reported it:

I'm not sure I get thin difference between should and must here.

cpython/Doc/c-api/typeobj.rst

Lines 1205 to 1212 in 033aa5c
.. c:macro:: Py_TPFLAGS_MANAGED_DICT

   This bit indicates that instances of the class have a `~object.__dict__` 
   attribute, and that the space for the dictionary is managed by the VM. 

   If this flag is set, :c:macro:`Py_TPFLAGS_HAVE_GC` should also be set. 

   .. versionadded:: 3.12 

I read this as if Py_TPFLAGS_HAVE_GC flag is not set then Py_TPFLAGS_MANAGED_DICT will not work properly and bad things will happen.

No, in my eyes should implies that in the vast majority of cases the Py_TPFLAGS_HAVE_GC flag will also be set if the Py_TPFLAGS_MANAGED_DICT flag is set, but there might be specific exceptions where this doesn't apply. In contrast must implies that there is no situation in which Py_TPFLAGS_MANAGED_DICT can be set without the Py_TPFLAGS_HAVE_GC flag being also set.

If you throw an error when Py_TPFLAGS_MANAGED_DICT is set but not Py_TPFLAGS_HAVE_GC, then you've implicitly created a must relation between the flags. If it's a NotImplementedError there is an indication that the must relation is due to a current (transient) technical limitation and not a design choice, so the documentation may still say should in that case.

While the Python documentation (to my knowledge) does not officially reference this, I think RFC 2119 provides a good framework for how words such as must or should are to be used in technical documentations and/or specifications.

I don't believe NotImplementedError suitable here (anyway I open for any changes). I read it as it is not implemented now but may be implemented in the future. I'm not sure it will be implemented in such way. But who knows.

I think there are three sane options here:

• Either you throw a NotImplementedError and leave the documentation mostly as-is, so that the documentation is treated as lore, i.e. it should be possible to use the 2 flags without the Py_TPFLAGS_HAVE_GC flag, but the current implementation is not there
• Or you throw a different error and update the documentation that it is now enforced that the Py_TPFLAGS_HAVE_GC flag has to also be set
• Or you actually change the code to allow the flags to be set without the Py_TPFLAGS_HAVE_GC flag

From my personal perspective:

I don't think Py_TPFLAGS_MANAGED_DICT makes much sense without Py_TPFLAGS_HAVE_GC, so I think here the documentation should be upgraded to use must and a SystemError (or similar, but not NotImplementedError) should be raised when they are not used in conjunction.

On the other hand I think it can make sense to have Py_TPFLAGS_MANAGED_WEAKREF without Py_TPFLAGS_HAVE_GC (think a third-party library representing some resource handle that doesn't transitively reference other Python objects, so GC support isn't required, but taking a weak reference to a resource can make sense - this is actually how I discovered this issue), so my personal preference would be for this to simply work. But I will understand if you decide that it's just too much complexity for too little gain.