Repository navigation
Types with Py_TPFLAGS_MANAGED_WEAKREF but not Py_TPFLAGS_HAVE_GC crash when creating a weak reference #134786
Description
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(defaulttp_allocof all types) allocates the object by calling_PyType_AllocNoTrack(Objects/typeobject.c)_PyType_AllocNoTrackuses_PyType_PreHeaderSizeto determine how many bytes to allocate in front of thePyObjectstructure (Objects/typeobject.c)_PyType_PreHeaderSizereturns the size of 2 pointers if eitherPy_TPFLAGS_MANAGED_WEAKREForPy_TPFLAGS_MANAGED_DICTis a flag plussizeof(PyGC_Head)if the object supports GC traversal, which is also the size of 2 pointers (technically 2 timesuintptr_) (Include/internal/pycore_object.h)- However,
MANAGED_WEAKREF_OFFSETis hard-defined to be(((Py_ssize_t)sizeof(PyObject *))*-4), which is only correct if thePyGC_Headstruct is also present, and that is put intotp_weaklistoffsetof the object - And
GET_WEAKREFS_LISTPTRused inObjects/weakrefobject.cfor creating a weak reference then reads from memory that comes before the area thatmalloc()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
Activity
sergey-miryanov commentedon Jun 23, 2025
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 commentedon Jun 25, 2025
Although
Py_TPFLAGS_MANAGED_*are not limited C API, some docs have encouraged the flags since 3.12, and the tutorial has requiredPy_TPFLAGS_MANAGED_WEAKREFon a static type instead of settingtp_weaklistoffset. Then,MANAGED_WEAKREF_OFFSETshould be more flexible to be consistent.Py_TPFLAGS_MANAGED_DICThas 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 commentedon Jun 25, 2025
@neonene Yeah,
Py_TPFLAGS_MANAGED_DICTdoesn't relate to this issue. But if we omitPy_TPFLAGS_HAVE_GCwhen usePy_TPFLAGS_MANAGED_DICTthen 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 commentedon Jun 25, 2025
In some sense it is breaking change. I understand.
neonene commentedon Jun 26, 2025
Just to avoid a crash, I would use
NotImplementedErrorkeeping the design unchanged.sergey-miryanov commentedon Jun 26, 2025
@neonene I'm not sure I get you. Can you elaborate?
neonene commentedon Jun 26, 2025
Static types are not discouraged from using
tp_dictoffsetsince the alternative is not available:cpython/Doc/c-api/typeobj.rst
Lines 1845 to 1848 in 033aa5c
Even on heap types without the GC flag, authors should go back to the tp slot because the
Py_TPFLAGS_MANAGED_DICThandling can be considered to be not implemented yet with very low priority. Raising an exception to work around a possible crash seems fine when missingPy_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 useNotImplementedErroras "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_WEAKREFcase. As for the priority of the non-GC version ofMANAGED_WEAKREF_OFFSET, I just rely on coredevs.sergey-miryanov commentedon Jun 26, 2025
I mostly agree with you. Some clarification to be sure we speak the same.
Py_TPFLAGS_MANAGED_DICTimpliesPy_TPFLAGS_HAVE_GCand this is documented. If we don't want to raise a TypeError exception, then we can add an assert instead.Py_TPFLAGS_MANAGED_WEAKREFnot officially impliesPy_TPFLAGS_HAVE_GCat this time, but code written as it should imply gc-flag.neonene commentedon Jun 26, 2025
Yes. Keep the original remarks as-is.
Py_TPFLAGS_HAVE_GCis recommended by using "should," sincePy_TPFLAGS_MANAGED_DICTaccepts 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?
The PR raises
SystemError, and my suggestion is replacing it withNotImplementedError. Assertion cannot avoid a crash in non-debug builds, which should be against your concern.No. This issue is mainly about the static types in 3rd party extension modules, where
Py_TPFLAGS_MANAGED_WEAKREFis highly encouraged.sergey-miryanov commentedon Jun 27, 2025
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
I read this as if
Py_TPFLAGS_HAVE_GCflag is not set thenPy_TPFLAGS_MANAGED_DICTwill not work properly and bad things will happen. As I seePy_TPFLAGS_MANAGED_DICTmaybe used with static types too, not only with heaptypes.I don't believe
NotImplementedErrorsuitable 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_WEAKREFor get direction from coredevs that we should fix implementation.chris-se commentedon Jun 27, 2025
From my perspective, as the one who ran into this issue and reported it:
No, in my eyes should implies that in the vast majority of cases the
Py_TPFLAGS_HAVE_GCflag will also be set if thePy_TPFLAGS_MANAGED_DICTflag is set, but there might be specific exceptions where this doesn't apply. In contrast must implies that there is no situation in whichPy_TPFLAGS_MANAGED_DICTcan be set without thePy_TPFLAGS_HAVE_GCflag being also set.If you throw an error when
Py_TPFLAGS_MANAGED_DICTis set but notPy_TPFLAGS_HAVE_GC, then you've implicitly created a must relation between the flags. If it's aNotImplementedErrorthere 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 think there are three sane options here:
NotImplementedErrorand 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 thePy_TPFLAGS_HAVE_GCflag, but the current implementation is not therePy_TPFLAGS_HAVE_GCflag has to also be setPy_TPFLAGS_HAVE_GCflagFrom my personal perspective:
I don't think
Py_TPFLAGS_MANAGED_DICTmakes much sense withoutPy_TPFLAGS_HAVE_GC, so I think here the documentation should be upgraded to use must and aSystemError(or similar, but notNotImplementedError) 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_WEAKREFwithoutPy_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.sergey-miryanov commentedon Jun 27, 2025
Thanks for clarification! I didn't think about this in RFC terms.
I agree that we should fix docs about
Py_TPFLAGS_MANAGED_DICT.Does it make sense for you if
Py_TPFLAGS_MANAGED_WEAKREFandPy_TPFLAGS_HAVE_GCmust be used in conjunction? On my side I will try to fix current implementation.chris-se commentedon Jun 27, 2025
As I said previously: in an ideal world I think it would be preferable if you could use
Py_TPFLAGS_MANAGED_WEAKREFindependently ofPy_TPFLAGS_HAVE_GC, but I totally get that it's a lot of added complexity to make this work without (that also makes it harder to test all corner cases), so I'd also be fine withPy_TPFLAGS_HAVE_GCbeing enforced ifPy_TPFLAGS_MANAGED_WEAKREF. In my current extension code I've addedPy_TPFLAGS_HAVE_GC(+ the necessary other bits) anyway because of the crash, so I don't even need to change anything if you do that. (AddingPy_TPFLAGS_HAVE_GCfor a type that doesn't reference any other Python objects itself is simple, I just needed to add aPyObject_GC_Untrack()to thedeallocmethod and atraverseimplementation that does nothing but return0.)A compromise could be that you raise a
NotImplementedErrorinstead of aSystemErrorat the moment to leave the option open to supporting this in the future. But the docs should still be updated in that case to indicate thatPy_TPFLAGS_HAVE_GCis not optional whenPy_TPFLAGS_MANAGED_WEAKREFis used, at least in current Python versions, if you decide to go that route.neonene commentedon Jun 28, 2025
cpython/Include/internal/pycore_object.h
Lines 939 to 942 in c419af9
It looks to me like the offset issues can be resolved all at once. If so, raising
NotImplementedErrorshould be enough for now in both cases. Ultimately, we could make the object layout redundant on higher demand, in order to keep the offsets unchanged regardless of the gc support.I think that if
Py_TPFLAGS_HAVE_GCwere necessary, coredevs would have already forbidden the omission.E: Tweaked the permalink as
Py_GIL_DISABLEDhas no issue. It is a breaking change to requirePy_TPFLAGS_HAVE_GCthere.neonene commentedon Jun 29, 2025
These may be good starting points to follow the history/context:
tp_dictoffset semantics changed without a deprecation period steering-council#133
https://docs.python.org/3.11/c-api/typeobj.html#c.PyTypeObject.tp_dictoffset
sergey-miryanov commentedon Jun 29, 2025
@neonene Thanks! I already read those links, also I read through the #95245 and related PR.
Python: add weakref support to builtin types
sergey-miryanov commentedon Sep 15, 2025
We face this is in real work task, so I believe we should fix this to properly work with
Py_TPFLAGS_MANAGED_WEAKREFbut withoutPy_TPFLAGS_HAVE_GC.Starbuck5 commentedon Sep 20, 2025
I just ran into this myself doing something similar to this. A Python wrapper over a C struct. Thank you to the people in this thread for tracking down all the causes and discussing solutions.
Regardless of whether this is intended to work or not, or intended to work or not in the future, the documentation should say that it doesn't work now.
Merge branch 'main' into pythongh-134786-managed-weakref
markshannon commentedon Sep 26, 2025
If
Py_TPFLAGS_MANAGED_WEAKREFis set, thenPy_TPFLAGS_HAVE_GCmust also be set.If a class needs its instances to support weak references, then it can use
tp_weaklistoffset.I really don't want to add the extra code on hot paths to handle this relatively obscure case.
We should raise an exception in
PyType_Readyfor this, as well as fixing the docs.sergey-miryanov commentedon Sep 26, 2025
@markshannon Ok, thanks! At first attempt I made literally as you said.
I tried to support
Py_TPFLAGS_MANAGED_WEAKREFwithoutPy_TPFLAGS_HAVE_GCwhen removing GC support from_thread.lockad @kumaraditya303 suggested but faced this bug - #116946 (comment).So, IIUC, we should use
tp_weaklistoffsetfor_thread.lock- I will try.Merge branch 'main' into pythongh-134786-managed-weakref
Merge branch 'main' into pythongh-134786-managed-weakref
gh-134786: raise error if `Py_TPFLAGS_MANAGED_WEAKREF` or `Py_TPFLAGS…
pythongh-134786: raise error if `Py_TPFLAGS_MANAGED_WEAKREF` or `Py_T…