Remove cyclic reference in CFuncPtr instances; see issue #2682.

Backport candidate for the release25-maint branch.

4 files changed, 136 insertions, 50 deletions

diff --git a/Misc/NEWS b/Misc/NEWS
index 52aee80..7862ffe 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -43,6 +43,9 @@ Extensions Modules
 Library
 -------
 
+- Issue #2682: ctypes callback functions now longer contain a cyclic
+  reference to themselves.
+
 - The getpass module has been improved on Unix.  It now uses /dev/tty
   by default and uses stderr instead of stdout.  A GetPassWarning is
   issued when input echo cannot be controlled.
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index 355fda2..c9da7d3 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -3096,7 +3096,7 @@ CFuncPtr_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 	CFuncPtrObject *self;
 	PyObject *callable;
 	StgDictObject *dict;
-	ffi_info *thunk;
+	CThunkObject *thunk;
 
 	if (PyTuple_GET_SIZE(args) == 0)
 		return GenericCData_new(type, args, kwds);
@@ -3153,11 +3153,6 @@ CFuncPtr_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 		return NULL;
 	}
 
-	/*****************************************************************/
-	/* The thunk keeps unowned references to callable and dict->argtypes
-	   so we have to keep them alive somewhere else: callable is kept in self,
-	   dict->argtypes is in the type's stgdict.
-	*/
 	thunk = AllocFunctionCallback(callable,
 				      dict->argtypes,
 				      dict->restype,
@@ -3166,27 +3161,22 @@ CFuncPtr_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 		return NULL;
 
 	self = (CFuncPtrObject *)GenericCData_new(type, args, kwds);
-	if (self == NULL)
+	if (self == NULL) {
+		Py_DECREF(thunk);
 		return NULL;
+	}
 
 	Py_INCREF(callable);
 	self->callable = callable;
 
 	self->thunk = thunk;
-	*(void **)self->b_ptr = *(void **)thunk;
-
-	/* We store ourself in self->b_objects[0], because the whole instance
-	   must be kept alive if stored in a structure field, for example.
-	   Cycle GC to the rescue! And we have a unittest proving that this works
-	   correctly...
-	*/
-
-	Py_INCREF((PyObject *)self); /* for KeepRef */
-	if (-1 == KeepRef((CDataObject *)self, 0, (PyObject *)self)) {
+	*(void **)self->b_ptr = (void *)thunk->pcl;
+	
+	Py_INCREF((PyObject *)thunk); /* for KeepRef */
+	if (-1 == KeepRef((CDataObject *)self, 0, (PyObject *)thunk)) {
 		Py_DECREF((PyObject *)self);
 		return NULL;
 	}
-
 	return (PyObject *)self;
 }
 
@@ -3639,6 +3629,7 @@ CFuncPtr_traverse(CFuncPtrObject *self, visitproc visit, void *arg)
 	Py_VISIT(self->argtypes);
 	Py_VISIT(self->converters);
 	Py_VISIT(self->paramflags);
+	Py_VISIT(self->thunk);
 	return CData_traverse((CDataObject *)self, visit, arg);
 }
 
@@ -3652,13 +3643,7 @@ CFuncPtr_clear(CFuncPtrObject *self)
 	Py_CLEAR(self->argtypes);
 	Py_CLEAR(self->converters);
 	Py_CLEAR(self->paramflags);
-
-	if (self->thunk) {
-		FreeClosure(self->thunk->pcl);
-		PyMem_Free(self->thunk);
-		self->thunk = NULL;
-	}
-
+	Py_CLEAR(self->thunk);
 	return CData_clear((CDataObject *)self);
 }
 
@@ -5186,6 +5171,9 @@ init_ctypes(void)
 	if (PyType_Ready(&PyCArg_Type) < 0)
 		return;
 
+	if (PyType_Ready(&CThunk_Type) < 0)
+		return;
+
 	/* StgDict is derived from PyDict_Type */
 	StgDict_Type.tp_base = &PyDict_Type;
 	if (PyType_Ready(&StgDict_Type) < 0)
diff --git a/Modules/_ctypes/callbacks.c b/Modules/_ctypes/callbacks.c
index 21637ad..0d3655e 100644
--- a/Modules/_ctypes/callbacks.c
+++ b/Modules/_ctypes/callbacks.c
@@ -12,6 +12,73 @@
 #endif
 #include "ctypes.h"
 
+/**************************************************************/
+
+static CThunkObject_dealloc(PyObject *_self)
+{
+	CThunkObject *self = (CThunkObject *)_self;
+	Py_XDECREF(self->converters);
+	Py_XDECREF(self->callable);
+	Py_XDECREF(self->restype);
+	if (self->pcl)
+		FreeClosure(self->pcl);
+	PyObject_Del(self);
+}
+
+static int
+CThunkObject_traverse(PyObject *_self, visitproc visit, void *arg)
+{
+	CThunkObject *self = (CThunkObject *)_self;
+	Py_VISIT(self->converters);
+	Py_VISIT(self->callable);
+	Py_VISIT(self->restype);
+	return 0;
+}
+
+static int
+CThunkObject_clear(PyObject *_self)
+{
+	CThunkObject *self = (CThunkObject *)_self;
+	Py_CLEAR(self->converters);
+	Py_CLEAR(self->callable);
+	Py_CLEAR(self->restype);
+	return 0;
+}
+
+PyTypeObject CThunk_Type = {
+	PyVarObject_HEAD_INIT(NULL, 0)
+	"_ctypes.CThunkObject",
+	sizeof(CThunkObject),			/* tp_basicsize */
+	sizeof(ffi_type),			/* tp_itemsize */
+	CThunkObject_dealloc,			/* tp_dealloc */
+	0,					/* tp_print */
+	0,					/* tp_getattr */
+	0,					/* tp_setattr */
+	0,					/* tp_compare */
+	0,					/* tp_repr */
+	0,					/* tp_as_number */
+	0,					/* tp_as_sequence */
+	0,					/* tp_as_mapping */
+	0,					/* tp_hash */
+	0,					/* tp_call */
+	0,					/* tp_str */
+	0,					/* tp_getattro */
+	0,					/* tp_setattro */
+	0,					/* tp_as_buffer */
+	Py_TPFLAGS_DEFAULT,			/* tp_flags */
+	"CThunkObject",				/* tp_doc */
+	CThunkObject_traverse,			/* tp_traverse */
+	CThunkObject_clear,	       		/* tp_clear */
+	0,					/* tp_richcompare */
+	0,					/* tp_weaklistoffset */
+	0,					/* tp_iter */
+	0,					/* tp_iternext */
+	0,					/* tp_methods */
+	0,					/* tp_members */
+};
+
+/**************************************************************/
+
 static void
 PrintError(char *msg, ...)
 {
@@ -247,32 +314,56 @@ static void closure_fcn(ffi_cif *cif,
 			void **args,
 			void *userdata)
 {
-	ffi_info *p = userdata;
+	CThunkObject *p = (CThunkObject *)userdata;
 
 	_CallPythonObject(resp,
-			  p->restype,
+			  p->ffi_restype,
 			  p->setfunc,
 			  p->callable,
 			  p->converters,
 			  args);
 }
 
-ffi_info *AllocFunctionCallback(PyObject *callable,
-				PyObject *converters,
-				PyObject *restype,
-				int is_cdecl)
+static CThunkObject* CThunkObject_new(Py_ssize_t nArgs)
+{
+	CThunkObject *p;
+	int i;
+
+	p = PyObject_NewVar(CThunkObject, &CThunk_Type, nArgs);
+	if (p == NULL) {
+		PyErr_NoMemory();
+		return NULL;
+	}
+
+	p->pcl = NULL;
+	memset(&p->cif, 0, sizeof(p->cif));
+	p->converters = NULL;
+	p->callable = NULL;
+	p->setfunc = NULL;
+	p->ffi_restype = NULL;
+	
+	for (i = 0; i < nArgs + 1; ++i)
+		p->atypes[i] = NULL;
+	return p;
+}
+
+CThunkObject *AllocFunctionCallback(PyObject *callable,
+				    PyObject *converters,
+				    PyObject *restype,
+				    int is_cdecl)
 {
 	int result;
-	ffi_info *p;
+	CThunkObject *p;
 	Py_ssize_t nArgs, i;
 	ffi_abi cc;
 
 	nArgs = PySequence_Size(converters);
-	p = (ffi_info *)PyMem_Malloc(sizeof(ffi_info) + sizeof(ffi_type) * (nArgs));
-	if (p == NULL) {
-		PyErr_NoMemory();
+	p = CThunkObject_new(nArgs);
+	if (p == NULL)
 		return NULL;
-	}
+
+	assert(CThunk_CheckExact(p));
+
 	p->pcl = MallocClosure();
 	if (p->pcl == NULL) {
 		PyErr_NoMemory();
@@ -288,9 +379,11 @@ ffi_info *AllocFunctionCallback(PyObject *callable,
 	}
 	p->atypes[i] = NULL;
 
+	Py_INCREF(restype);
+	p->restype = restype;
 	if (restype == Py_None) {
 		p->setfunc = NULL;
-		p->restype = &ffi_type_void;
+		p->ffi_restype = &ffi_type_void;
 	} else {
 		StgDictObject *dict = PyType_stgdict(restype);
 		if (dict == NULL || dict->setfunc == NULL) {
@@ -299,7 +392,7 @@ ffi_info *AllocFunctionCallback(PyObject *callable,
 		  goto error;
 		}
 		p->setfunc = dict->setfunc;
-		p->restype = &dict->ffi_type_pointer;
+		p->ffi_restype = &dict->ffi_type_pointer;
 	}
 
 	cc = FFI_DEFAULT_ABI;
@@ -323,16 +416,14 @@ ffi_info *AllocFunctionCallback(PyObject *callable,
 		goto error;
 	}
 
+	Py_INCREF(converters);
 	p->converters = converters;
+	Py_INCREF(callable);
 	p->callable = callable;
 	return p;
 
   error:
-	if (p) {
-		if (p->pcl)
-			FreeClosure(p->pcl);
-		PyMem_Free(p);
-	}
+	Py_XDECREF(p);
 	return NULL;
 }
 
diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
index 8f6846a..1a104cf 100644
--- a/Modules/_ctypes/ctypes.h
+++ b/Modules/_ctypes/ctypes.h
@@ -84,14 +84,18 @@ struct tagCDataObject {
 };
 
 typedef struct {
+	PyObject_VAR_HEAD
 	ffi_closure *pcl; /* the C callable */
 	ffi_cif cif;
 	PyObject *converters;
 	PyObject *callable;
+	PyObject *restype;
 	SETFUNC setfunc;
-	ffi_type *restype;
+	ffi_type *ffi_restype;
 	ffi_type *atypes[1];
-} ffi_info;
+} CThunkObject;
+extern PyTypeObject CThunk_Type;
+#define CThunk_CheckExact(v)	    ((v)->ob_type == &CThunk_Type)
 
 typedef struct {
 	/* First part identical to tagCDataObject */
@@ -107,7 +111,7 @@ typedef struct {
 	union value b_value;
 	/* end of tagCDataObject, additional fields follow */
 
-	ffi_info *thunk;
+	CThunkObject *thunk;
 	PyObject *callable;
 
 	/* These two fields will override the ones in the type's stgdict if
@@ -178,10 +182,10 @@ extern void init_callbacks_in_module(PyObject *m);
 
 extern PyMethodDef module_methods[];
 
-extern ffi_info *AllocFunctionCallback(PyObject *callable,
-				       PyObject *converters,
-				       PyObject *restype,
-				       int stdcall);
+extern CThunkObject *AllocFunctionCallback(PyObject *callable,
+					   PyObject *converters,
+					   PyObject *restype,
+					   int stdcall);
 /* a table entry describing a predefined ctypes type */
 struct fielddesc {
 	char code;