Обсуждение: [MASSMAIL][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

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

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

> 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] https://postgr.es/m/20230504234235.GA2419591%40nathanxps13

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