Thread: pgsql: Avoid crashing when a JIT-inlined backend function throws an err
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(-)
On Fri, Jun 28, 2024 at 12:14 AM Tom Lane <tgl@sss.pgh.pa.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
Amul Sul <sulamul@gmail.com> writes:
> On Fri, Jun 28, 2024 at 12:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> 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.
I'd prefer not to propagate this kluge further than we provably
have to. We've already created a memory leak with uncertain
consequences in that FreeErrorData no longer reliably frees
everything in the structure. Adding more copying steps makes
that worse. So without demonstrable gain, I'd rather leave it
alone.
regards, tom lane