Date: Sun, 2 Jun 2024 22:44:25 +0200
From: Erik Wienhold <ewie@ewie.name>
To: Pierre Forstmann <pierre.forstmann@gmail.com>
Cc: "a.kozhemyakin" <a.kozhemyakin@postgrespro.ru>, 
	pgsql-hackers@lists.postgresql.org
Subject: Re: pltcl crashes due to a syntax error
Message-ID: <5af13487-7722-4bdd-bad3-939598c8dd4e@ewie.name>
References: <6a2a1c40-2b2c-4a33-8b72-243c0766fcda@postgrespro.ru>
 <CAM-sOH9zC5VeycS0BFCV4CAEfDL4OLEh_41oWtVLfbOZCtxNFw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAM-sOH9zC5VeycS0BFCV4CAEfDL4OLEh_41oWtVLfbOZCtxNFw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/5af13487-7722-4bdd-bad3-939598c8dd4e%40ewie.name>
Precedence: bulk

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