My attention happened to be drawn to plpython's PLy_traceback()
function, and I noted a couple of things that sure seem like bugs.
First, there's this bit:
e_type_o = PyObject_GetAttrString(e, "__name__");
e_module_o = PyObject_GetAttrString(e, "__module__");
if (e_type_o)
e_type_s = PyString_AsString(e_type_o);
if (e_type_s)
e_module_s = PyString_AsString(e_module_o);
Surely that second "if" is meant to be "if (e_module_o)"? It doesn't
make any sense to be testing whether we could get a string from
e_type_o to decide if it's safe to touch e_module_o. This is probably
only a latent bug because not getting these strings is a can't-happen
case, but still.
Second, the whole function shows truly remarkable faith that none of
what it calls will ever throw an error. If that does happen, the code
will leak PyObject references --- and it *can* happen, if only because
of the possibility of OOM in string allocation. Since the code seems
to be trying not to leak those, the fact that its coverage is so
incomplete seems like a bug.
I then realized that there's another fundamental risk of leaking
PyObject references, which is that this function is charged with
dropping all the references in the passed-in traceback stack;
but if it errors out anywhere, it'll fail to do that. It would
be better for that responsibility to lie with PLy_elog_impl which
obtained the stack in the first place (and which also has unwarranted
faith in its callees not failing).
So I propose the attached. For ease of review, I've not re-indented
the code that needs to move inside PG_TRY blocks. Also, I dropped the
logic about pfree'ing the string buffers in PLy_elog_impl's PG_FINALLY
block: that doesn't seem necessary, and continuing to do it would
require making those things volatile which is notationally messy.
regards, tom lane
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
index ddf3573f0e7..67441d89ee4 100644
--- a/src/pl/plpython/plpy_elog.c
+++ b/src/pl/plpython/plpy_elog.c
@@ -64,6 +64,9 @@ PLy_elog_impl(int elevel, const char *fmt,...)
PyErr_Fetch(&exc, &val, &tb);
+ /* Use a PG_TRY block to ensure we release the PyObjects just acquired */
+ PG_TRY();
+ {
if (exc != NULL)
{
PyErr_NormalizeException(&exc, &val, &tb);
@@ -81,7 +84,6 @@ PLy_elog_impl(int elevel, const char *fmt,...)
elevel = FATAL;
}
- /* this releases our refcount on tb! */
PLy_traceback(exc, val, tb,
&xmsg, &tbmsg, &tb_depth);
@@ -113,8 +115,6 @@ PLy_elog_impl(int elevel, const char *fmt,...)
primary = xmsg;
}
- PG_TRY();
- {
ereport(elevel,
(errcode(sqlerrcode ? sqlerrcode : ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg_internal("%s", primary ? primary : "no exception data"),
@@ -136,14 +136,16 @@ PLy_elog_impl(int elevel, const char *fmt,...)
}
PG_FINALLY();
{
- if (fmt)
- pfree(emsg.data);
- if (xmsg)
- pfree(xmsg);
- if (tbmsg)
- pfree(tbmsg);
Py_XDECREF(exc);
Py_XDECREF(val);
+ /* Must release all the objects in the traceback stack */
+ while (tb != NULL && tb != Py_None)
+ {
+ PyObject *tb_prev = tb;
+
+ tb = PyObject_GetAttrString(tb, "tb_next");
+ Py_DECREF(tb_prev);
+ }
}
PG_END_TRY();
}
@@ -154,21 +156,14 @@ PLy_elog_impl(int elevel, const char *fmt,...)
* The exception error message is returned in xmsg, the traceback in
* tbmsg (both as palloc'd strings) and the traceback depth in
* tb_depth.
- *
- * We release refcounts on all the Python objects in the traceback stack,
- * but not on e or v.
*/
static void
PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
char **xmsg, char **tbmsg, int *tb_depth)
{
- PyObject *e_type_o;
- PyObject *e_module_o;
- char *e_type_s = NULL;
- char *e_module_s = NULL;
- PyObject *vob = NULL;
- char *vstr;
- StringInfoData xstr;
+ PyObject *volatile e_type_o = NULL;
+ PyObject *volatile e_module_o = NULL;
+ PyObject *volatile vob = NULL;
StringInfoData tbstr;
/*
@@ -186,12 +181,18 @@ PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
/*
* Format the exception and its value and put it in xmsg.
*/
+ PG_TRY();
+ {
+ char *e_type_s = NULL;
+ char *e_module_s = NULL;
+ const char *vstr;
+ StringInfoData xstr;
e_type_o = PyObject_GetAttrString(e, "__name__");
e_module_o = PyObject_GetAttrString(e, "__module__");
if (e_type_o)
e_type_s = PLyUnicode_AsString(e_type_o);
- if (e_type_s)
+ if (e_module_o)
e_module_s = PLyUnicode_AsString(e_module_o);
if (v && ((vob = PyObject_Str(v)) != NULL))
@@ -215,18 +216,24 @@ PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
appendStringInfo(&xstr, ": %s", vstr);
*xmsg = xstr.data;
+ }
+ PG_FINALLY();
+ {
+ Py_XDECREF(e_type_o);
+ Py_XDECREF(e_module_o);
+ Py_XDECREF(vob);
+ }
+ PG_END_TRY();
/*
* Now format the traceback and put it in tbmsg.
*/
-
*tb_depth = 0;
initStringInfo(&tbstr);
/* Mimic Python traceback reporting as close as possible. */
appendStringInfoString(&tbstr, "Traceback (most recent call last):");
while (tb != NULL && tb != Py_None)
{
- PyObject *volatile tb_prev = NULL;
PyObject *volatile frame = NULL;
PyObject *volatile code = NULL;
PyObject *volatile name = NULL;
@@ -254,17 +261,6 @@ PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
filename = PyObject_GetAttrString(code, "co_filename");
if (filename == NULL)
elog(ERROR, "could not get file name from Python code object");
- }
- PG_CATCH();
- {
- Py_XDECREF(frame);
- Py_XDECREF(code);
- Py_XDECREF(name);
- Py_XDECREF(lineno);
- Py_XDECREF(filename);
- PG_RE_THROW();
- }
- PG_END_TRY();
/* The first frame always points at <module>, skip it. */
if (*tb_depth > 0)
@@ -320,18 +316,19 @@ PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
}
}
}
+ }
+ PG_FINALLY();
+ {
+ Py_XDECREF(frame);
+ Py_XDECREF(code);
+ Py_XDECREF(name);
+ Py_XDECREF(lineno);
+ Py_XDECREF(filename);
+ }
+ PG_END_TRY();
- Py_DECREF(frame);
- Py_DECREF(code);
- Py_DECREF(name);
- Py_DECREF(lineno);
- Py_DECREF(filename);
-
- /* Release the current frame and go to the next one. */
- tb_prev = tb;
+ /* Advance to the next frame. */
tb = PyObject_GetAttrString(tb, "tb_next");
- Assert(tb_prev != Py_None);
- Py_DECREF(tb_prev);
if (tb == NULL)
elog(ERROR, "could not traverse Python traceback");
(*tb_depth)++;
@@ -339,10 +336,6 @@ PLy_traceback(PyObject *e, PyObject *v, PyObject *tb,
/* Return the traceback. */
*tbmsg = tbstr.data;
-
- Py_XDECREF(e_type_o);
- Py_XDECREF(e_module_o);
- Py_XDECREF(vob);
}
/*
