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

Hi Nathan.

On 1/13/23, Nathan Bossart <nathandbossart(at)gmail(dot)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(at)anarazel(dot)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