[plpython] Add missing volatile qualifier.
Hi hackers,
I'm playing a toy static analysis checker with PostgreSQL and found a
variable is missing volatile qualifier.
Best Regards,
Xing
Attachments:
v1-0001-Add-missing-volatile-qualifier.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Add-missing-volatile-qualifier.patchDownload
From 7084055da64d0433b09e50faff630e551c363f27 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
Date: Mon, 1 Apr 2024 21:39:04 +0800
Subject: [PATCH v1] Add missing volatile qualifier.
The object pltargs is modified in the PG_TRY block (line 874) and used
in the PG_CATCH block (line 881) which should be qualified with
volatile.
---
src/pl/plpython/plpy_exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index e06fde1dd9..cd71082a5f 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -689,7 +689,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
*pltrelid,
*plttablename,
*plttableschema,
- *pltargs = NULL,
+ *volatile pltargs = NULL,
*pytnew,
*pytold,
*pltdata;
--
2.44.0
Xing Guo <higuoxing@gmail.com> writes:
I'm playing a toy static analysis checker with PostgreSQL and found a
variable is missing volatile qualifier.
Good catch! It looks like the consequences of a failure would be
pretty minimal --- AFAICS, no worse than a possible failure to remove
a refcount on Py_None --- but that's still a bug.
I don't care for your proposed fix though. I think the real problem
here is schizophrenia about when to set up pltargs, and we could
fix it more nicely as attached. (Perhaps the Asserts are overkill
though.)
regards, tom lane
Attachments:
fix-unsafe-initialization-of-pltargs.patchtext/x-diff; charset=us-ascii; name=fix-unsafe-initialization-of-pltargs.patchDownload
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index e06fde1dd9..3145c69699 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -689,7 +689,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
*pltrelid,
*plttablename,
*plttableschema,
- *pltargs = NULL,
+ *pltargs,
*pytnew,
*pytold,
*pltdata;
@@ -713,6 +713,11 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
return NULL;
}
}
+ else
+ {
+ Py_INCREF(Py_None);
+ pltargs = Py_None;
+ }
PG_TRY();
{
@@ -856,7 +861,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
PyObject *pltarg;
/* pltargs should have been allocated before the PG_TRY block. */
- Assert(pltargs);
+ Assert(pltargs && pltargs != Py_None);
for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
{
@@ -870,8 +875,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
}
else
{
- Py_INCREF(Py_None);
- pltargs = Py_None;
+ Assert(pltargs == Py_None);
}
PyDict_SetItemString(pltdata, "args", pltargs);
Py_DECREF(pltargs);
On Mon, Apr 01, 2024 at 11:57:07AM -0400, Tom Lane wrote:
Xing Guo <higuoxing@gmail.com> writes:
I'm playing a toy static analysis checker with PostgreSQL and found a
variable is missing volatile qualifier.Good catch! It looks like the consequences of a failure would be
pretty minimal --- AFAICS, no worse than a possible failure to remove
a refcount on Py_None --- but that's still a bug.
Huh. I seem to have dropped that "volatile" shortly before committing for
some reason [0]/messages/by-id/20230504234235.GA2419591@nathanxps13.
I don't care for your proposed fix though. I think the real problem
here is schizophrenia about when to set up pltargs, and we could
fix it more nicely as attached. (Perhaps the Asserts are overkill
though.)
Your fix seems fine to me.
[0]: /messages/by-id/20230504234235.GA2419591@nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
On Mon, Apr 01, 2024 at 11:57:07AM -0400, Tom Lane wrote:
Good catch! It looks like the consequences of a failure would be
pretty minimal --- AFAICS, no worse than a possible failure to remove
a refcount on Py_None --- but that's still a bug.
Huh. I seem to have dropped that "volatile" shortly before committing for
some reason [0].
Oh, I'd forgotten that discussion. Given that we were both confused
about the need for it, all the more reason to try to avoid using a
within-PG_TRY assignment.
Your fix seems fine to me.
Thanks for looking, I'll push it shortly.
regards, tom lane