Be more careful about Python refcounts while creating exception objects.

tglsfdc · tglsfdc · commit cea6de20b356 · 2016-12-09T15:27:23.000-05:00
PLy_generate_spi_exceptions neglected to do Py_INCREF on the new exception objects, evidently supposing that PyModule_AddObject would do that --- but it doesn't. This left us in a situation where a Python garbage collection cycle could result in deletion of exception object(s), causing server crashes or wrong answers if the exception objects are used later in the session. In addition, PLy_generate_spi_exceptions didn't bother to test for a null result from PyErr_NewException, which at best is inconsistent with the code in PLy_add_exceptions. And PLy_add_exceptions, while it did do Py_INCREF on the exceptions it makes, waited to do that till after some PyModule_AddObject calls, creating a similar risk for failure if garbage collection happened within those calls. To fix, refactor to have just one piece of code that creates an exception object and adds it to the spiexceptions module, bumping the refcount first. Also, let's add an additional refcount to represent the pointer we're going to store in a C global variable or hash table. This should only matter if the user does something weird like delete the spiexceptions Python module, but lack of paranoia has caused us enough problems in PL/Python already. The fact that PyModule_AddObject doesn't do a Py_INCREF of its own explains the need for the Py_INCREF added in commit 4c966d9, so we can improve the comment about that; also, this means we really want to do that before not after the PyModule_AddObject call. The missing Py_INCREF in PLy_generate_spi_exceptions was reported and diagnosed by Rafa de la Torre; the other fixes by me. Back-patch to all supported branches. Discussion: https://postgr.es/m/CA+Fz15kR1OXZv43mDrJb3XY+1MuQYWhx5kx3ea6BRKQp6ezGkg@mail.gmail.com
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
@@ -25,6 +25,9 @@ HTAB	   *PLy_spi_exceptions = NULL;
 
 
 static void PLy_add_exceptions(PyObject *plpy);
+static PyObject *PLy_create_exception(char *name,
+					 PyObject *base, PyObject *dict,
+					 const char *modname, PyObject *mod);
 static void PLy_generate_spi_exceptions(PyObject *mod, PyObject *base);
 
 /* module functions */
@@ -192,47 +195,63 @@ PLy_add_exceptions(PyObject *plpy)
 #else
 	excmod = PyModule_Create(&PLy_exc_module);
 #endif
-	if (PyModule_AddObject(plpy, "spiexceptions", excmod) < 0)
-		PLy_elog(ERROR, "could not add the spiexceptions module");
+	if (excmod == NULL)
+		PLy_elog(ERROR, "could not create the spiexceptions module");
 
 	/*
-	 * XXX it appears that in some circumstances the reference count of the
-	 * spiexceptions module drops to zero causing a Python assert failure when
-	 * the garbage collector visits the module. This has been observed on the
-	 * buildfarm. To fix this, add an additional ref for the module here.
-	 *
-	 * This shouldn't cause a memory leak - we don't want this garbage
-	 * collected, and this function shouldn't be called more than once per
-	 * backend.
+	 * PyModule_AddObject does not add a refcount to the object, for some odd
+	 * reason; we must do that.
 	 */
 	Py_INCREF(excmod);
+	if (PyModule_AddObject(plpy, "spiexceptions", excmod) < 0)
+		PLy_elog(ERROR, "could not add the spiexceptions module");
 
-	PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL);
-	PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL);
-	PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, NULL);
-
-	if (PLy_exc_error == NULL ||
-		PLy_exc_fatal == NULL ||
-		PLy_exc_spi_error == NULL)
-		PLy_elog(ERROR, "could not create the base SPI exceptions");
-
-	Py_INCREF(PLy_exc_error);
-	PyModule_AddObject(plpy, "Error", PLy_exc_error);
-	Py_INCREF(PLy_exc_fatal);
-	PyModule_AddObject(plpy, "Fatal", PLy_exc_fatal);
-	Py_INCREF(PLy_exc_spi_error);
-	PyModule_AddObject(plpy, "SPIError", PLy_exc_spi_error);
+	PLy_exc_error = PLy_create_exception("plpy.Error", NULL, NULL,
+										 "Error", plpy);
+	PLy_exc_fatal = PLy_create_exception("plpy.Fatal", NULL, NULL,
+										 "Fatal", plpy);
+	PLy_exc_spi_error = PLy_create_exception("plpy.SPIError", NULL, NULL,
+											 "SPIError", plpy);
 
 	memset(&hash_ctl, 0, sizeof(hash_ctl));
 	hash_ctl.keysize = sizeof(int);
 	hash_ctl.entrysize = sizeof(PLyExceptionEntry);
 	hash_ctl.hash = tag_hash;
-	PLy_spi_exceptions = hash_create("SPI exceptions", 256,
+	PLy_spi_exceptions = hash_create("PL/Python SPI exceptions", 256,
 									 &hash_ctl, HASH_ELEM | HASH_FUNCTION);
 
 	PLy_generate_spi_exceptions(excmod, PLy_exc_spi_error);
 }
 
+/*
+ * Create an exception object and add it to the module
+ */
+static PyObject *
+PLy_create_exception(char *name, PyObject *base, PyObject *dict,
+					 const char *modname, PyObject *mod)
+{
+	PyObject   *exc;
+
+	exc = PyErr_NewException(name, base, dict);
+	if (exc == NULL)
+		PLy_elog(ERROR, "could not create exception \"%s\"", name);
+
+	/*
+	 * PyModule_AddObject does not add a refcount to the object, for some odd
+	 * reason; we must do that.
+	 */
+	Py_INCREF(exc);
+	PyModule_AddObject(mod, modname, exc);
+
+	/*
+	 * The caller will also store a pointer to the exception object in some
+	 * permanent variable, so add another ref to account for that.  This is
+	 * probably excessively paranoid, but let's be sure.
+	 */
+	Py_INCREF(exc);
+	return exc;
+}
+
 /*
  * Add all the autogenerated exceptions as subclasses of SPIError
  */
@@ -258,12 +277,14 @@ PLy_generate_spi_exceptions(PyObject *mod, PyObject *base)
 
 		PyDict_SetItemString(dict, "sqlstate", sqlstate);
 		Py_DECREF(sqlstate);
-		exc = PyErr_NewException(exception_map[i].name, base, dict);
-		PyModule_AddObject(mod, exception_map[i].classname, exc);
+
+		exc = PLy_create_exception(exception_map[i].name, base, dict,
+								   exception_map[i].classname, mod);
+
 		entry = hash_search(PLy_spi_exceptions, &exception_map[i].sqlstate,
 							HASH_ENTER, &found);
-		entry->exc = exc;
 		Assert(!found);
+		entry->exc = exc;
 	}
 }
 
