Subject: Re: PL/Python: Fix return in the middle of PG_TRY() block.

From: Tom Lane <[email-masked]>
Date: Wed, 03 May 2023 16:33:32 -0400

Nathan Bossart <[email-masked]> writes:
> 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).

Hmm, actually I think these changes should allow you to *remove* some
volatile markers.  IIUC, you need volatile for variables that are declared
outside PG_TRY but modified within it.  That is the case for these
pointers as the code stands, but your patch is changing them to the
non-risky case where they are assigned once before entering PG_TRY.

(My mental model of this is that without "volatile", the compiler
may keep the variable in a register, creating the hazard that longjmp
will revert the variable's value to what it was at setjmp time thanks
to the register save/restore that those functions do.  But if it hasn't
changed value since entering PG_TRY, then that doesn't matter.)

> 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.

Agreed, throwing an error in these situations doesn't improve matters.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 3 May 2023 13:58:38 -0700

> changed value since entering PG_TRY, then that doesn't matter.)

Ah, I think you are right.  elog.h states as follows:

 * Note: if a local variable of the function containing PG_TRY is modified
 * in the PG_TRY section and used in the PG_CATCH section, that variable
 * must be declared "volatile" for POSIX compliance.  This is not mere
 * pedantry; we have seen bugs from compilers improperly optimizing code
 * away when such a variable was not marked.  Beware that gcc's -Wclobbered
 * warnings are just about entirely useless for catching such oversights.

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.
 
-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 3 May 2023 21:54:13 -0700

> future, but I see no need to keep it for pltdata.

Here's a new patch that removes the volatile marker from pltdata.

>From 53d2d942b8ee553439da6f324186a62ebb7fce1f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 3 May 2023 22:01:08 -0700

> Here's a new patch that removes the volatile marker from pltdata.

Gah, right after I sent that, I realized we can remove one more volatile
marker.  Sorry for the noise.

>From 24a4e91b3cdd66b985fab70f3aed8f6a202b31fe Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Wed, 3 May 2023 11:32:43 -0700
Subject: [PATCH v5 1/1] Fix improper returns in PG_TRY blocks.

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 | 54 ++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..fcb363891a 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -411,15 +411,20 @@ static PyObject *
 PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 {
 	PyObject   *volatile arg = NULL;
-	PyObject   *volatile args = NULL;
+	PyObject   *args = NULL;
 	int			i;
 
+	/*
 
-	PG_TRY();
+	/*
 
+	PG_TRY();
+	{

---

From: Tom Lane <[email-masked]>
Date: Thu, 04 May 2023 08:39:03 -0400

> marker.  Sorry for the noise.

Hmm, I'm not sure why PLy_trigger_build_args's pltargs needs to
gain a "volatile" here?  LGTM otherwise.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 4 May 2023 16:42:35 -0700

> gain a "volatile" here?  LGTM otherwise.

I removed that new "volatile" marker before committing.  I was trying to
future-proof a bit and follow elog.h's recommendation to the letter, but
following your mental model upthread, it doesn't seem to be strictly
necessary, and we'd need to set pltargs to NULL after decrementing its
reference count in the PG_TRY section for such future-proofing to be
effective, anyway.

Thank you for reviewing!

---

From: Xing Guo <[email-masked]>
Date: Fri, 5 May 2023 09:40:32 +0800

Sorry for not responding to this thread for a long time and a huge thank
you for pushing it forward!

Best Regards,
Xing