Here's a new version of the patch. Besides adding comments and a commit message, I made sure to decrement the reference count for pltargs in the PG_CATCH block (which means that pltargs likely needs to be volatile). I'm not too wild about moving the chunk of code for pltargs like this, but I haven't thought of a better option. We could error instead of returning NULL, but IIUC that would go against d0aa965's stated purpose.
-- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 12daf35ca398a34046d911270283f2cb7ebcbf3f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Wed, 3 May 2023 11:32:43 -0700 Subject: [PATCH v3 1/1] Fix improper returns in PG_TRY blocks. If we exit a PG_TRY block early via "continue", "break", "goto", or "return", we'll skip unwinding its exception stack. This change moves a couple of such "return" statements in PL/Python out of PG_TRY blocks. This was introduced in d0aa965c0a and affects all supported versions. We might also be able to add compile-time checks to avoid recurrence, but that is left as a future exercise. Reported-by: Mikhail Gribkov, Xing Guo Author: Xing Guo Reviewed-by: Michael Paquier, Andres Freund, Tom Lane Discussion: https://postgr.es/m/CAMEv5_v5Y%2B-D%3DCO1%2Bqoe16sAmgC4sbbQjz%2BUtcHmB6zcgS%2B5Ew%40mail.gmail.com Discussion: https://postgr.es/m/CACpMh%2BCMsGMRKFzFMm3bYTzQmMU5nfEEoEDU2apJcc4hid36AQ%40mail.gmail.com Backpatch-through: 11 (all supported versions) --- src/pl/plpython/plpy_exec.c | 48 +++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c index 923703535a..9a483daa8e 100644 --- a/src/pl/plpython/plpy_exec.c +++ b/src/pl/plpython/plpy_exec.c @@ -414,12 +414,17 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc) PyObject *volatile args = NULL; int i; + /* + * Make any Py*_New() calls before the PG_TRY block so that we can quickly + * return NULL on failure. We can't return within the PG_TRY block, else + * we'd miss unwinding the exception stack. + */ + args = PyList_New(proc->nargs); + if (!args) + return NULL; + PG_TRY(); { - args = PyList_New(proc->nargs); - if (!args) - return NULL; - for (i = 0; i < proc->nargs; i++) { PLyDatumToOb *arginfo = &proc->args[i]; @@ -683,19 +688,34 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r *pltlevel, *pltrelid, *plttablename, - *plttableschema; - PyObject *pltargs, + *plttableschema, *pytnew, *pytold; + PyObject *volatile pltargs = NULL; PyObject *volatile pltdata = NULL; char *stroid; - PG_TRY(); + /* + * Make any Py*_New() calls before the PG_TRY block so that we can quickly + * return NULL on failure. We can't return within the PG_TRY block, else + * we'd miss unwinding the exception stack. + */ + pltdata = PyDict_New(); + if (!pltdata) + return NULL; + + if (tdata->tg_trigger->tgnargs) { - pltdata = PyDict_New(); - if (!pltdata) + pltargs = PyList_New(tdata->tg_trigger->tgnargs); + if (!pltargs) + { + Py_DECREF(pltdata); return NULL; + } + } + PG_TRY(); + { pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname); PyDict_SetItemString(pltdata, "name", pltname); Py_DECREF(pltname); @@ -835,12 +855,9 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r int i; PyObject *pltarg; - pltargs = PyList_New(tdata->tg_trigger->tgnargs); - if (!pltargs) - { - Py_DECREF(pltdata); - return NULL; - } + /* pltargs should have been allocated before the PG_TRY block. */ + Assert(pltargs); + for (i = 0; i < tdata->tg_trigger->tgnargs; i++) { pltarg = PLyUnicode_FromString(tdata->tg_trigger->tgargs[i]); @@ -861,6 +878,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r } PG_CATCH(); { + Py_XDECREF(pltargs); Py_XDECREF(pltdata); PG_RE_THROW(); } -- 2.25.1
