Call PyType_Ready() on ThreadCanary_Type

[emontnemery]

Proposed change

Call PyType_Ready() on ThreadCanary_Type before the first thread_canary is created. Placed in thread_canary_register().

Background

ThreadCanary_Type (src/c/misc_thread_common.h) is declared with PyVarObject_HEAD_INIT(NULL, 0) and is never passed to PyType_Ready(), nor is its ob_type set explicitly - so its metatype stays NULL for the life of the process. Yet instances are created: thread_canary_register() does PyObject_New(ThreadCanaryObj, &ThreadCanary_Type) and stores the object in the foreign thread's PyThreadState dict.

Code that inspects such an instance's type dereferences the NULL metatype and crashes; in my case we get crashes when using zhuyifei1999/guppy3 to walk the heap.

CPython reference

The NULL in PyVarObject_HEAD_INIT(NULL, 0) is only a placeholder for compilers that reject &PyType_Type as a static initializer; it isn't meant to survive to runtime. Per the CPython C-API docs for the type ob_type field (https://docs.python.org/3/c-api/typeobj.html#pyobject-slots):

…the convention is to pass NULL to the PyObject_HEAD_INIT macro and to initialize this field explicitly at the start of the module's initialization function, before doing anything else. […] This should be done before any instances of the type are created. PyType_Ready() checks if ob_type is NULL, and if so, initializes it to the ob_type field of the base class.

[mattip]

Why are you inspecting that type? Is this some kind of test harness?

[emontnemery]

We see crashes when calling the Home Assistant profiler.memory action, which uses guppy3 to do a heap walk and dump the output to a file. The purpose of that action is to aid debugging of memory leaks etc.

guppy3 doesn't guard against NULL metatype and segfaults (I've opened a PR on guppy3 to guard against that zhuyifei1999/guppy3#56)

My understanding is that live CPython objects should not have a NULL metatype, or is that intentionally the case here for some reason?

[mattip]

It is, as the name says, a canary probe. Maybe we should make it only live in tests, nothing at runtime uses it.

[emontnemery]

It is, as the name says, a canary probe. Maybe we should make it only live in tests, nothing at runtime uses it.

OK, that makes sense 👍
Should I close this PR and replace it with one which hides it in runtime?

Edit: I seems to me the canary was added to fix https://foss.heptapod.net/pypy/cffi/-/work_items/362, and is not there to aid testing.

cffi/src/c/misc_thread_common.h

Lines 32 to 50 in 1f2d6ec

	/* We try to keep the PyThreadState around in a thread not started by
	* Python but where cffi callbacks occur. If we didn't do that, then
	* the standard logic in PyGILState_Ensure() and PyGILState_Release()
	* would create a new PyThreadState and completely free it for every
	* single call. For some applications, this is a huge slow-down.
	*
	* As shown by issue #362, it is quite messy to do. The current
	* solution is to keep the PyThreadState alive by incrementing its
	* 'gilstate_counter'. We detect thread shut-down, and we put the
	* PyThreadState inside a list of zombies (we can't free it
	* immediately because we don't have the GIL at that point in time).
	* We also detect other pieces of code (notably Py_Finalize()) which
	* clear and free PyThreadStates under our feet, using ThreadCanaryObj.
	*/
	#define TLS_ZOM_LOCK() PyThread_acquire_lock(cffi_zombie_lock, WAIT_LOCK)
	#define TLS_ZOM_UNLOCK() PyThread_release_lock(cffi_zombie_lock)
	static PyThread_type_lock cffi_zombie_lock = NULL;

cffi/src/c/misc_thread_common.h

Lines 52 to 82 in 1f2d6ec

	/* A 'canary' object is created in a thread when there is a callback
	invoked, and that thread has no PyThreadState so far. It is an
	object of reference count equal to 1, which is stored in the
	PyThreadState->dict. Two things can occur then:
	1. The PyThreadState can be forcefully cleared by Py_Finalize().
	Then thread_canary_dealloc() is called, and we have to cancel
	the hacks we did to keep the PyThreadState alive.
	2. The thread finishes. In that case, we put the canary in a list
	of zombies, and at some convenient time later when we have the
	GIL, we free all PyThreadStates in the zombie list.
	Some more fun comes from the fact that thread_canary_dealloc() can
	be called at a point where the canary is in the zombie list already.
	Also, the various pieces are freed at specific points in time, and
	we must make sure not to access already-freed structures:
	- the struct cffi_tls_s is valid until the thread shuts down, and
	then it is freed by cffi_thread_shutdown().
	- the canary is a normal Python object, but we have a borrowed
	reference to it from cffi_tls_s.local_thread_canary.
	*/
	typedef struct thread_canary_s {
	PyObject_HEAD
	struct thread_canary_s *zombie_prev, *zombie_next;
	PyThreadState *tstate;
	struct cffi_tls_s *tls;
	} ThreadCanaryObj;

[mattip]

OK, then the PyType_Ready should probably be called next to the type object creation, around line 214. Does that work?

cffi/src/c/misc_thread_common.h

Lines 210 to 220 in 61fe449

	goto ignore_error;
	canary->zombie_prev = NULL;
	canary->zombie_next = NULL;
	canary->tstate = tstate;
	canary->tls = tls;
	err = PyDict_SetItemString(tdict, "cffi.thread.canary", (PyObject *)canary);
	Py_DECREF(canary);
	if (err < 0)
	goto ignore_error;

[emontnemery]

OK, then the PyType_Ready should probably be called next to the type object creation, around line 214. Does that work?

Done.

[mattip]

Thanks @emontnemery

[emontnemery]

@mattip thanks a lot for the quick merge! Do you have plans to make a release anytime soon?