pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedConfused coding in PLy_traceback()
2+ messages / 1 participants
[nested] [flat]
Commits from this thread:
2025-06-01 93aca12461 ecc8fd2b77 4672b62239 b898bb2a79 6f724fcf81 d4556f592e Run pgindent on the previous commit.
2025-06-01 5c7fd59762 1c78d55531 7559a16e23 c6f7f11d8f b56a92651a 31a3a15fa4 Fix edge-case resource leaks in PL/Python error reporting.
* Confused coding in PLy_traceback()
@ 2025-05-31 20:33 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 2+ messages in thread
From: Tom Lane @ 2025-05-31 20:33 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
Attachments:
v1-fix-leaks-in-PLy_traceback.patch (text/x-diff, 4.7K) download
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);
}
/*
* Re: Confused coding in PLy_traceback()
@ 2025-06-01 19:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 2+ messages in thread
From: Tom Lane @ 2025-06-01 19:05 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I wrote:
> 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.
Pushed. After sleeping on it I decided that removing the pfree's
would be a poor tradeoff, as it's not clear how long those allocations
might survive otherwise. The extra "volatile" markers for "xmsg"
and "tbmsg" aren't that big a deal. Trying to mark "emsg" as volatile
would be problematic because none of the StringInfo routines are set
up to allow that, but it shouldn't be a problem because that struct
will surely be on the stack.
regards, tom lane
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2025-06-01 19:05 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-05-31 20:33 Confused coding in PLy_traceback() Tom Lane <tgl@sss.pgh.pa.us>
2025-06-01 19:05 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox