Re: pgsql: Avoid crashing when a JIT-inlined backend function throws an err

On Fri, Jun 28, 2024 at 12:14 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Avoid crashing when a JIT-inlined backend function throws an error.
>
> errfinish() assumes that the __FUNC__ and __FILE__ arguments it's
> passed are compile-time constant strings that can just be pointed
> to rather than physically copied. However, it's possible for LLVM
> to generate code in which those pointers point into a dynamically
> loaded code segment. If that segment gets unloaded before we're
> done with the ErrorData struct, we have dangling pointers that
> will lead to SIGSEGV. In simple cases that won't happen, because we
> won't unload LLVM code before end of transaction. But it's possible
> to happen if the error is thrown within end-of-transaction code run by
> _SPI_commit or _SPI_rollback, because since commit 2e517818f those
> functions clean up by ending the transaction and starting a new one.
>
> Rather than fixing this by adding pstrdup() overhead to every
> elog/ereport sequence, let's fix it by copying the risky pointers
> in CopyErrorData(). That solves it for _SPI_commit/_SPI_rollback
> because they use that function to preserve the error data across
> the transaction end/restart sequence; and it seems likely that
> any other code doing something similar would need to do that too.
>
> I'm suspicious that this behavior amounts to an LLVM bug (or a
> bug in our use of it?), because it implies that string constant
> references that should be pointer-equal according to a naive
> understanding of C semantics will sometimes not be equal.
> However, even if it is a bug and someday gets fixed, we'll have
> to cope with the current behavior for a long time to come.
>
> Report and patch by me. Back-patch to all supported branches.
>
> Discussion: https://postgr.es/m/1565654.1719425368@sss.pgh.pa.us
>
> Branch
> ------
> REL_14_STABLE
>
> Details
> -------
> https://git.postgresql.org/pg/commitdiff/13abc1f660740edab9c32d1ad4ca0fb5d5f387f6
>
> Modified Files
> --------------
> src/backend/utils/error/elog.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>

Hi Tom,

Should we make a similar change to ReThrowError() as well? I'm
particularly concerned about cases where someone might copy error data
using CopyErrorData() and then rethrowing that copied edata.

Regards,
Amul