Обсуждение: PL/Python: Fix return in the middle of PG_TRY() block.

Hi hackers,

I was running static analyser against PostgreSQL and found there're 2
return statements in PL/Python module which is not safe. Patch is
attached.

-- 
Best Regards,
Xing

On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
> I was running static analyser against PostgreSQL and found there're 2
> return statements in PL/Python module which is not safe. Patch is
> attached.

Is the problem that PG_exception_stack and error_context_stack aren't
properly reset?

> @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
>      PyObject   *volatile pltdata = NULL;
>      char       *stroid;
>  
> +    pltdata = PyDict_New();
> +    if (!pltdata)
> +        return NULL;
> +
>      PG_TRY();
>      {
> -        pltdata = PyDict_New();
> -        if (!pltdata)
> -            return NULL;
> -
>          pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
>          PyDict_SetItemString(pltdata, "name", pltname);
>          Py_DECREF(pltname);

There's another "return" later on in this PG_TRY block.  I wonder if it's
possible to detect this sort of thing at compile time.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 12, 2023 at 10:44:33AM -0800, Nathan Bossart wrote:
> There's another "return" later on in this PG_TRY block.  I wonder if it's
> possible to detect this sort of thing at compile time.

Note also:
src/pl/tcl/pltcl.c- *   PG_CATCH();
src/pl/tcl/pltcl.c- *   {
src/pl/tcl/pltcl.c- *       pltcl_subtrans_abort(interp, oldcontext, oldowner);
src/pl/tcl/pltcl.c- *       return TCL_ERROR;
src/pl/tcl/pltcl.c- *   }

This is documented once, repeated twice:
src/pl/plpython/plpy_spi.c- *   PG_CATCH();
src/pl/plpython/plpy_spi.c- *   {
src/pl/plpython/plpy_spi.c- *       <do cleanup>
src/pl/plpython/plpy_spi.c- *       PLy_spi_subtransaction_abort(oldcontext, oldowner);
src/pl/plpython/plpy_spi.c- *       return NULL;
src/pl/plpython/plpy_spi.c- *   }

I don't quite get why this would be a sane thing to do here when
aborting a subtransaction in pl/python, but my experience with this
code is limited.
--
Michael

Hi,

On 2023-01-12 10:44:33 -0800, Nathan Bossart wrote:
> On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
> > @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
> >      PyObject   *volatile pltdata = NULL;
> >      char       *stroid;
> >  
> > +    pltdata = PyDict_New();
> > +    if (!pltdata)
> > +        return NULL;
> > +
> >      PG_TRY();
> >      {
> > -        pltdata = PyDict_New();
> > -        if (!pltdata)
> > -            return NULL;
> > -
> >          pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
> >          PyDict_SetItemString(pltdata, "name", pltname);
> >          Py_DECREF(pltname);
> 
> There's another "return" later on in this PG_TRY block.  I wonder if it's
> possible to detect this sort of thing at compile time.

Clang provides some annotations that allow to detect this kind of thing. I
hacked up a test for this, and it finds quite a bit of prolematic
code. plpython is, uh, not being good? But also in plperl, pltcl.

Example complaints:

[776/1239 42  62%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:472:1: warning: no_returns_in_pg_try
'no_returns_handle'is not held on every path through here [-Wthread-safety-analysis]
 
}
^
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:417:2: note: no_returns_in_pg_try acquired here
        PG_TRY();
        ^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:424:7: note: expanded from macro 'PG_TRY'
                    no_returns_start(no_returns_handle##__VA_ARGS__)
                    ^
...
[785/1239 42  63%] Compiling C object src/pl/tcl/pltcl.so.p/pltcl.c.o
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning: no_returns_in_pg_try 'no_returns_handle' is
notheld on every path through here [-Wthread-safety-analysis]
 
}
^
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note: no_returns_in_pg_try acquired here
        PG_CATCH();
        ^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note: expanded from macro 'PG_CATCH'
                    no_returns_start(no_returns_handle##__VA_ARGS__)
                    ^

Not perfect digestible, but also not too bad. I pushed the
no_returns_start()/no_returns_stop() calls into all the PG_TRY related macros,
because that causes the warning to point to block that the problem is
in. E.g. above the first warning points to PG_TRY, the second to
PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.


Clearly this would need a bunch more work, but it seems promising? I think
there'd be other uses than this.


I briefly tried to use it for spinlocks. Mostly works and detects things like
returning with a spinlock held. But it does not like dynahash's habit of
conditionally acquiring and releasing spinlocks.

Greetings,

Andres Freund

Hi,

On 2023-01-12 21:49:00 -0800, Andres Freund wrote:
> Clearly this would need a bunch more work, but it seems promising? I think
> there'd be other uses than this.
> 
> I briefly tried to use it for spinlocks. Mostly works and detects things like
> returning with a spinlock held. But it does not like dynahash's habit of
> conditionally acquiring and releasing spinlocks.

One example is to prevent things like elog()/ereport()/SpinlockAcquire() while
holding a spinlock

The "locks_excluded(thing)" attribute (which is just heuristic, doesn't require
expansive annotation like requires_capability(!thing)), can quite easily be
used to trigger warnings about this kind of thing:

../../../../home/andres/src/postgresql/src/backend/access/transam/xlog.c:6771:2: warning: cannot call function
'errstart'while no_nested_spinlock 'in_spinlock' is held [-Wthread-safety-analysis]
 
        elog(LOG, "logging with spinlock held");


Greetings,

Andres Freund

Hi Nathan.

On 1/13/23, Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
>> I was running static analyser against PostgreSQL and found there're 2
>> return statements in PL/Python module which is not safe. Patch is
>> attached.
>
> Is the problem that PG_exception_stack and error_context_stack aren't
> properly reset?

Yes, it is.

>
>> @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo,
>> PLyProcedure *proc, HeapTuple *r
>>      PyObject   *volatile pltdata = NULL;
>>      char       *stroid;
>>
>> +    pltdata = PyDict_New();
>> +    if (!pltdata)
>> +        return NULL;
>> +
>>      PG_TRY();
>>      {
>> -        pltdata = PyDict_New();
>> -        if (!pltdata)
>> -            return NULL;
>> -
>>          pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
>>          PyDict_SetItemString(pltdata, "name", pltname);
>>          Py_DECREF(pltname);
>
> There's another "return" later on in this PG_TRY block.  I wonder if it's
> possible to detect this sort of thing at compile time.

Thanks for pointing it out! I missed some return statements. Because
my checker is treating 'return statement in PG_TRY() block' as errors.
It stops compiling when it finds the code pattern and I forget to
double check it.

My checker is based on AST matcher and it's possible to turn it into a
clang-tidy plugin or something similar. I want to make it easy to
integrate with scan-build, so I implement it as a static analyzer
plugin :)

If anyone is interested in the checker itself, the source code can be
found here[1]:

> Note also:
> src/pl/tcl/pltcl.c- *   PG_CATCH();
> src/pl/tcl/pltcl.c- *   {
> src/pl/tcl/pltcl.c- *       pltcl_subtrans_abort(interp, oldcontext,
> oldowner);
> src/pl/tcl/pltcl.c- *       return TCL_ERROR;
> src/pl/tcl/pltcl.c- *   }
>
> This is documented once, repeated twice:
> src/pl/plpython/plpy_spi.c- *   PG_CATCH();
> src/pl/plpython/plpy_spi.c- *   {
> src/pl/plpython/plpy_spi.c- *       <do cleanup>
> src/pl/plpython/plpy_spi.c- *       PLy_spi_subtransaction_abort(oldcontext,
> oldowner);
> src/pl/plpython/plpy_spi.c- *       return NULL;
> src/pl/plpython/plpy_spi.c- *   }
>
> I don't quite get why this would be a sane thing to do here when
> aborting a subtransaction in pl/python, but my experience with this
> code is limited.

Hi Michael,

I'll try to understand what's going on in your pasted codes. Thanks
for pointing it out!

> Example complaints:
>
> [776/1239 42  62%] Compiling C object
> src/pl/plpython/plpython3.so.p/plpy_exec.c.o
> ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:472:1:
> warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path
> through here [-Wthread-safety-analysis]
> }
> ^
> ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:417:2:
> note: no_returns_in_pg_try acquired here
>         PG_TRY();
>         ^
> ../../../../home/andres/src/postgresql/src/include/utils/elog.h:424:7: note:
> expanded from macro 'PG_TRY'
>                     no_returns_start(no_returns_handle##__VA_ARGS__)
>                     ^
> ...
> [785/1239 42  63%] Compiling C object src/pl/tcl/pltcl.so.p/pltcl.c.o
> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning:
> no_returns_in_pg_try 'no_returns_handle' is not held on every path through
> here [-Wthread-safety-analysis]
> }
> ^
> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note:
> no_returns_in_pg_try acquired here
>         PG_CATCH();
>         ^
> ../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note:
> expanded from macro 'PG_CATCH'
>                     no_returns_start(no_returns_handle##__VA_ARGS__)
>                     ^

Hi Andres,

Your patch looks interesting and useful. I will play with it in the
next following days. I'm burried with other works recently, so my
reply may delay.

> Not perfect digestible, but also not too bad. I pushed the
> no_returns_start()/no_returns_stop() calls into all the PG_TRY related
> macros,
> because that causes the warning to point to block that the problem is
> in. E.g. above the first warning points to PG_TRY, the second to
> PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.
>
>
> Clearly this would need a bunch more work, but it seems promising? I think
> there'd be other uses than this.
>
>
> I briefly tried to use it for spinlocks. Mostly works and detects things
> like
> returning with a spinlock held. But it does not like dynahash's habit of
> conditionally acquiring and releasing spinlocks.
>
> Greetings,
>
> Andres Freund
>

[1] https://github.com/higuoxing/clang-plugins/blob/main/lib/ReturnInPgTryBlockChecker.cpp

-- 
Best Regards,
Xing









On 1/13/23, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2023-01-12 21:49:00 -0800, Andres Freund wrote:
>> Clearly this would need a bunch more work, but it seems promising? I
>> think
>> there'd be other uses than this.
>>
>> I briefly tried to use it for spinlocks. Mostly works and detects things
>> like
>> returning with a spinlock held. But it does not like dynahash's habit of
>> conditionally acquiring and releasing spinlocks.
>
> One example is to prevent things like elog()/ereport()/SpinlockAcquire()
> while
> holding a spinlock
>
> The "locks_excluded(thing)" attribute (which is just heuristic, doesn't
> require
> expansive annotation like requires_capability(!thing)), can quite easily be
> used to trigger warnings about this kind of thing:
>
> ../../../../home/andres/src/postgresql/src/backend/access/transam/xlog.c:6771:2:
> warning: cannot call function 'errstart' while no_nested_spinlock
> 'in_spinlock' is held [-Wthread-safety-analysis]
>         elog(LOG, "logging with spinlock held");
>
>
> Greetings,
>
> Andres Freund
>


-- 
Best Regards,
Xing

On Thu, Jan 12, 2023 at 09:49:00PM -0800, Andres Freund wrote:
> On 2023-01-12 10:44:33 -0800, Nathan Bossart wrote:
>> There's another "return" later on in this PG_TRY block.  I wonder if it's
>> possible to detect this sort of thing at compile time.
> 
> Clang provides some annotations that allow to detect this kind of thing. I
> hacked up a test for this, and it finds quite a bit of prolematic
> code.

Nice!

> plpython is, uh, not being good? But also in plperl, pltcl.

Yikes.

> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning: no_returns_in_pg_try 'no_returns_handle'
isnot held on every path through here [-Wthread-safety-analysis]
 
> }
> ^
> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note: no_returns_in_pg_try acquired here
>         PG_CATCH();
>         ^
> ../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note: expanded from macro 'PG_CATCH'
>                     no_returns_start(no_returns_handle##__VA_ARGS__)
>                     ^
> 
> Not perfect digestible, but also not too bad. I pushed the
> no_returns_start()/no_returns_stop() calls into all the PG_TRY related macros,
> because that causes the warning to point to block that the problem is
> in. E.g. above the first warning points to PG_TRY, the second to
> PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.

This seems roughly as digestible as the pg_prevent_errno_in_scope stuff.
However, on my macOS machine with clang 14.0.0, the messages say "mutex"
instead of "no_returns_in_pg_try," which is unfortunate since that's the
part that would clue me into what the problem is.  I suppose it'd be easy
enough to figure out after a grep or two, though.

> Clearly this would need a bunch more work, but it seems promising? I think
> there'd be other uses than this.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Xing Guo <higuoxing@gmail.com> writes:
> Are there any unsafe codes in pltcl.c? The return statement is in the
> PG_CATCH() block, I think the exception stack has been recovered in
> PG_CATCH block so the return statement in PG_CATCH block should be ok?

Yes, the stack has already been unwound at the start of a PG_CATCH
(or PG_FINALLY) block, so there is no reason to avoid returning
out of those.

In principle you could also mess things up with a "continue", "break",
or "goto" leading out of PG_TRY.  That's probably far less likely
than "return", but I wonder whether Andres' compiler hack will
catch that.

            regards, tom lane

Hi,

On 2023-01-16 10:29:03 -0500, Tom Lane wrote:
> Xing Guo <higuoxing@gmail.com> writes:
> > Are there any unsafe codes in pltcl.c? The return statement is in the
> > PG_CATCH() block, I think the exception stack has been recovered in
> > PG_CATCH block so the return statement in PG_CATCH block should be ok?
> 
> Yes, the stack has already been unwound at the start of a PG_CATCH
> (or PG_FINALLY) block, so there is no reason to avoid returning
> out of those.

It's probably true for PG_CATCH, but for PG_FINALLY? Won't returning lead us
to miss rethrowing the error? I guess you can argue that's desired, but then
why would one use PG_FINALLY?


I'm somewhat dubious about allowing to return inside PG_CATCH, even if it's
safe today.


> In principle you could also mess things up with a "continue", "break",
> or "goto" leading out of PG_TRY.  That's probably far less likely
> than "return", but I wonder whether Andres' compiler hack will
> catch that.

I haven't tested it, but it should - it basically traces every path and sees
whether there's any way the "capability" isn't released. To the point that
it's very annoying in other contexts, because it doesn't deal well with
conditional lock acquisition/releases.

Greetings,

Andres Freund

On Thu, Jan 19, 2023 at 05:07:11PM -0800, Andres Freund wrote:
> I'm somewhat dubious about allowing to return inside PG_CATCH, even if it's
> safe today.

+1.  Unless there are known use-cases, IMHO it'd be better to restrict it
to prevent future compatibility breaks as PG_TRY evolves.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

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

Nathan Bossart <nathandbossart@gmail.com> 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

On Wed, May 03, 2023 at 04:33:32PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> 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.)

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

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

On Wed, May 03, 2023 at 09:54:13PM -0700, Nathan Bossart wrote:
> 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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> Gah, right after I sent that, I realized we can remove one more volatile
> 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

On Thu, May 04, 2023 at 08:39:03AM -0400, Tom Lane wrote:
> Hmm, I'm not sure why PLy_trigger_build_args's pltargs needs to
> 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!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com