Hi,
I revised my patch, added the missing one that Nathan mentioned.
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?
```
PG_TRY();
{
UTF_BEGIN;
ereport(level,
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg("%s", UTF_U2E(Tcl_GetString(objv[2])))));
UTF_END;
}
PG_CATCH();
{
ErrorData *edata;
/* Must reset elog.c's state */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
/* Pass the error data to Tcl */
pltcl_construct_errorCode(interp, edata);
UTF_BEGIN;
Tcl_SetObjResult(interp, Tcl_NewStringObj(UTF_E2U(edata->message), -1));
UTF_END;
FreeErrorData(edata);
return TCL_ERROR;
}
PG_END_TRY();
```
Best Regards,
Xing
On Sat, Jan 14, 2023 at 2:03 AM Nathan Bossart <[email protected]>
wrote:
> 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' 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__)
> > ^
> >
> > 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
>
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..7181b5e898 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -414,12 +414,12 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
PyObject *volatile args = NULL;
int i;
+ 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];
@@ -684,18 +684,28 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
*pltrelid,
*plttablename,
*plttableschema;
- PyObject *pltargs,
+ PyObject *pltargs = NULL,
*pytnew,
*pytold;
PyObject *volatile pltdata = NULL;
char *stroid;
- PG_TRY();
+ 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 +845,6 @@ 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;
- }
for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
{
pltarg = PLyUnicode_FromString(tdata->tg_trigger->tgargs[i]);
