Re: pltcl crashes due to a syntax error

On 2024-06-03 18:57 +0200, Tom Lane wrote:
> Erik Wienhold <ewie(at)ewie(dot)name> writes:
> > On 2024-06-03 00:15 +0200, Tom Lane wrote:
> >> The new bit of information that this bug report provides is that it's
> >> possible to get a TCL_ERROR result without Tcl having set errorInfo.
> >> That seems a tad odd, and it must happen only in weird corner cases,
> >> else we'd have heard of this decades ago. Not sure if it's worth
> >> trying to characterize those cases further, however.
>
> > ISTM that errorInfo is set automatically only during script evaluation.
>
> Yeah, I've just come to the same conclusion. Changing throw_tcl_error
> to ignore errorInfo if it's unset would be wrong, because that implies
> that the function we called doesn't fill errorInfo. I found by
> testing that it's actually possible that errorInfo contains leftover
> text from a previous error (that might not even have been in the same
> function), resulting in completely confusing/misleading output.
>
> So your thought that we should just not use throw_tcl_error here
> was correct, and a minimal fix could look like the attached.

LGTM.

> Also, compile_pltcl_function contains a Tcl_EvalEx() call that
> presumably could use throw_tcl_error, except it wants to add "could
> not create internal procedure" which would require some refactoring.
> As far as I can tell that error case is not reproducibly reachable,
> as it'd require OOM or some other problem inside Tcl, so (a) it's
> probably not worth troubling over and (b) changing it is a bit scary
> for lack of ability to test. I'm inclined to leave that alone too.

Agree.

> The other thing I noticed while looking at this is that the error text
> for the other Tcl_ListObjGetElements() call seems a bit confusingly
> worded: "could not split return value from trigger: %s". You could
> easily read that as suggesting that the return value is somehow
> attached to the trigger and has to be separated from it. I'm
> tempted to suggest rephrasing it to be parallel to the new error
> I added: "could not parse trigger return value: %s". But I didn't
> do that below.

Yeah, I'd fix that trigger error text as well to bring both in line.

--
Erik