Date: Wed, 3 May 2023 13:21:16 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Xing Guo <higuoxing@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: PL/Python: Fix return in the middle of PG_TRY() block.
Message-ID: <20230503202116.GA2110623@nathanxps13>
References: 
 <CACpMh+CMsGMRKFzFMm3bYTzQmMU5nfEEoEDU2apJcc4hid36AQ@mail.gmail.com>
 <20230112184433.GA2104952@nathanxps13>
 <20230113054900.b7onkvwtkrykeu3z@awork3.anarazel.de>
 <20230113180335.GA2160040@nathanxps13>
 <CACpMh+D9jz4Lq3qieoU2NkhGASqekhDXnXX2USW4mHa+SWNQag@mail.gmail.com>
 <1864041.1673882943@sss.pgh.pa.us>
 <20230120010711.jpqbaelpgmuvt2vq@awork3.anarazel.de>
 <20230120190201.GC4106863@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="k+w/mQv8wyuph6w0"
Content-Disposition: inline
In-Reply-To: <20230120190201.GC4106863@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20230503202116.GA2110623%40nathanxps13>
Precedence: bulk


--k+w/mQv8wyuph6w0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--k+w/mQv8wyuph6w0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Fix-improper-returns-in-PG_TRY-blocks.patch"

From 12daf35ca398a34046d911270283f2cb7ebcbf3f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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


--k+w/mQv8wyuph6w0--