public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Tom Lane <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Xing Guo <[email protected]>
Cc: [email protected]
Subject: Re: PL/Python: Fix return in the middle of PG_TRY() block.
Date: Wed, 3 May 2023 21:54:13 -0700
Message-ID: <20230504045413.GA2202617@nathanxps13> (raw)
In-Reply-To: <20230503205838.GA2112379@nathanxps13>
References: <20230112184433.GA2104952@nathanxps13>
<[email protected]>
<20230113180335.GA2160040@nathanxps13>
<CACpMh+D9jz4Lq3qieoU2NkhGASqekhDXnXX2USW4mHa+SWNQag@mail.gmail.com>
<[email protected]>
<[email protected]>
<20230120190201.GC4106863@nathanxps13>
<20230503202116.GA2110623@nathanxps13>
<[email protected]>
<20230503205838.GA2112379@nathanxps13>
On Wed, May 03, 2023 at 01:58:38PM -0700, Nathan Bossart wrote:
> With this change, pltdata isn't modified in the PG_TRY section, and the
> only modification of pltargs happens after all elogs. It might be worth
> keeping pltargs volatile in case someone decides to add an elog() in the
> future, but I see no need to keep it for pltdata.
Here's a new patch that removes the volatile marker from pltdata.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Fix-improper-returns-in-PG_TRY-blocks.patch (3.5K, ../20230504045413.GA2202617@nathanxps13/2-v4-0001-Fix-improper-returns-in-PG_TRY-blocks.patch)
download | inline diff:
From 53d2d942b8ee553439da6f324186a62ebb7fce1f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 3 May 2023 11:32:43 -0700
Subject: [PATCH v4 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 | 52 +++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..1e3efd4d86 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 pltdata = NULL;
+ *pytold,
+ *pltdata;
+ PyObject *volatile pltargs = 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
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: PL/Python: Fix return in the middle of PG_TRY() block.
In-Reply-To: <20230504045413.GA2202617@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox