Be more careful about Python refcounts while creating exception objects.

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 4c966d920, 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
index d80dc51b27462b603b12ef9142427c355429f14c..0cf2ad29cbdd209d8a60021813478489b7673258 100644 (file)
--- 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,46 +195,62 @@ 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);
-       PLy_spi_exceptions = hash_create("SPI exceptions", 256,
+       PLy_spi_exceptions = hash_create("PL/Python SPI exceptions", 256,
                                                                         &hash_ctl, HASH_ELEM | HASH_BLOBS);
 
        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
  */
@@ -257,12 +276,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;
        }
 }