pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Avoid crashing when a JIT-inlined backend function throws an err
8+ messages / 2 participants
[nested] [flat]
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/5d6c64d290978dab76c00460ba809156874be035
Modified Files
--------------
src/backend/utils/error/elog.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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_13_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/86fac88eed4b9a0874b1183573aa62d042262eba
Modified Files
--------------
src/backend/utils/error/elog.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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_16_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/07d66d3cc2a92f58a8c30a99ec7c9c68684900e9
Modified Files
--------------
src/backend/utils/error/elog.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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_12_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/dccda847b709e38ce6f09ac5c47f2bdf30e93a2c
Modified Files
--------------
src/backend/utils/error/elog.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-06-27 18:44 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-06-27 18:44 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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_15_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/5401e70e47aab30a24f35a6fc909c52a0ccbbe3f
Modified Files
--------------
src/backend/utils/error/elog.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-07-11 11:11 Amul Sul <sulamul@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Amul Sul @ 2024-07-11 11:11 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Avoid crashing when a JIT-inlined backend function throws an err
@ 2024-07-11 14:42 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amul Sul <sulamul@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2024-07-11 14:42 UTC (permalink / raw)
To: Amul Sul <sulamul@gmail.com>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2024-07-11 14:42 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
2024-07-11 11:11 ` Amul Sul <sulamul@gmail.com>
2024-07-11 14:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
2024-06-27 18:44 pgsql: Avoid crashing when a JIT-inlined backend function throws an err Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox