`PyCode_AddWatcher` callback must be non-null if the watcher bit is set

[nybblista]

PyCode_AddWatcher does not validate that the callback is non-NULL. As a result, the function will still assign a watcher slot, store the NULL pointer in interp->code_watchers[i], and set the corresponding bit in active_code_watchers.

cpython/Objects/codeobject.c

Lines 64 to 82 in b8d3fdd

	int
	PyCode_AddWatcher(PyCode_WatchCallback callback)
	{
	PyInterpreterState *interp = _PyInterpreterState_GET();
	assert(interp->_initialized);
	for (int i = 0; i < CODE_MAX_WATCHERS; i++) {
	if (!interp->code_watchers[i]) {
	interp->code_watchers[i] = callback;
	interp->active_code_watchers |= (1 << i);
	return i;
	}
	}
	PyErr_SetString(PyExc_RuntimeError, "no more code watcher IDs available");
	return -1;
	}

This creates an inconsistent internal state, because notify_code_watchers assumes, in release build, that any watcher bit set implies a non-NULL callback.

cpython/Objects/codeobject.c

Lines 39 to 64 in b8d3fdd

	static void
	notify_code_watchers(PyCodeEvent event, PyCodeObject *co)
	{
	assert(Py_REFCNT(co) > 0);
	PyInterpreterState *interp = _PyInterpreterState_GET();
	assert(interp->_initialized);
	uint8_t bits = interp->active_code_watchers;
	int i = 0;
	while (bits) {
	assert(i < CODE_MAX_WATCHERS);
	if (bits & 1) {
	PyCode_WatchCallback cb = interp->code_watchers[i];
	// callback must be non-null if the watcher bit is set
	assert(cb != NULL);
	if (cb(event, co) < 0) {
	PyErr_FormatUnraisable(
	"Exception ignored in %s watcher callback for %R",
	code_event_name(event), co);
	}
	}
	i++;
	bits >>= 1;
	}
	}

[ZeroIntensity]

I'm not sure I understand the problem here. Is there any legitimate use for calling PyCode_AddWatcher with a NULL pointer? If not, it's already well known that the C API can be broken by passing NULL around. In fact, the docs explicitly warn you about this:

While the functions described in this chapter carefully check the type of the objects which are passed in, many of them do not check for NULL being passed instead of a valid object. Allowing NULL to be passed in can cause memory access violations and immediate termination of the interpreter.

Did this cause an actual bug in practice? It could be worth adding an assertion here or clarifying the docs.

[Fidget-Spinner]

I don't think this is an issue. PyType_AddWatcher is only for C API use (mainly Cinder IIRC), and they know what they're doing.

[nybblista]

@ZeroIntensity
Thank you for the link, and yes I agree with you. The warning in the documentation is clear. So, I think, in the debug mode at least, assert that will be enough, and I think it does not hurt.

assert(callback != NULL);

[ZeroIntensity]

Generally speaking, I'd accept a PR that adds assertions, but I'm not really familiar with these APIs and don't want to encourage copycats.