Re: pltcl crashes due to a syntax error

On 2024-06-02 14:32 +0200, Pierre Forstmann wrote:
> I understand that Tcl_GetVar should not be used any more and should be
> replaced by Tcl_GetStringResult
> (but I know nothing about Tcl internals)
>
> Following patch :
> diff postgres/src/pl/tcl/pltcl.c.orig postgres/src/pl/tcl/pltcl.c
> 1373c1373,1376
> < econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
> TCL_GLOBAL_ONLY));
> ---
> > /*
> > * econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
> TCL_GLOBAL_ONLY));
> > */
> > econtext = utf_u2e(Tcl_GetStringResult(interp));
>
> gives:
>
> pierre=# CREATE OR REPLACE PROCEDURE test_proc(INOUT a text)
> AS $$
> set aa [concat $1 "+" $1]
> return [list $aa $aa])
> $$
> LANGUAGE pltcl;
> CREATE PROCEDURE
> pierre=# CALL test_proc('abc');
> 2024-06-02 14:22:45.223 CEST [61649] ERROR: list element in braces
> followed by ")" instead of space
> 2024-06-02 14:22:45.223 CEST [61649] CONTEXT: list element in braces
> followed by ")" instead of space
> in PL/Tcl function "test_proc"
> 2024-06-02 14:22:45.223 CEST [61649] STATEMENT: CALL test_proc('abc');
> ERROR: list element in braces followed by ")" instead of space
> CONTEXT: list element in braces followed by ")" instead of space
> in PL/Tcl function "test_proc"

Tcl_GetStringResult is already used for emsg. Setting econtext to same
string is rather pointless. The problem is that Tcl_ListObjGetElements
does not set errorInfo if conversion fails. From the manpage:

"If listPtr is not already a list value, Tcl_ListObjGetElements will
attempt to convert it to one; if the conversion fails, it returns
TCL_ERROR and leaves an error message in the interpreter's result value
if interp is not NULL."

Tcl_GetVar returns null if errorInfo does not exist. Omitting econtext
from errcontext in that case looks like the proper fix to me.

Or just do away with throw_tcl_error and call ereport directly. Compare
that to pltcl_trigger_handler where the same case is handled like this:

/************************************************************
* Otherwise, the return value should be a column name/value list
* specifying the modified tuple to return.
************************************************************/
if (Tcl_ListObjGetElements(interp, Tcl_GetObjResult(interp),
&result_Objc, &result_Objv) != TCL_OK)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("could not split return value from trigger: %s",
utf_u2e(Tcl_GetStringResult(interp)))));

--
Erik