pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedUse of signal-unsafe functions from signal handlers
8+ messages / 5 participants
[nested] [flat]
* Use of signal-unsafe functions from signal handlers
@ 2022-05-24 09:42 Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 8+ messages in thread
From: Mats Kindahl @ 2022-05-24 09:42 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi all,
Typically, signal-unsafe functions should not be called from signal
handlers. In particular, calling malloc() directly or indirectly can cause
deadlocks, making PostgreSQL unresponsive to signals.
Unless I am missing something, bgworker_die uses ereport, which indirectly
calls printf-like functions, which are not signal-safe since they use
malloc(). In rare cases, this can lead to deadlocks with stacks that look
like this (from https://github.com/timescale/timescaledb/issues/4200):
#0 0x00007f0e4d1040eb in __lll_lock_wait_private () from
target:/lib/x86_64-linux-gnu/libc.so.6
[...]
#3 malloc (size=53)
[...]
#7 0x000055b9212235b1 in errmsg ()
#8 0x00007f0e27bf27a8 in handle_sigterm (postgres_signal_arg=15) at
/build/timescaledb/src/bgw/scheduler.c:841
#9 <signal handler called>
[...]
#13 free (ptr=<optimized out>)
#14 0x00007f0e4db12cb4 in OPENSSL_LH_free () from
target:/lib/x86_64-linux-gnu/libcrypto.so.1.1
[...]
A simple fix for this is to introduce a signal-safe version of write_stderr
and use that from bgworker_die, which the attached patch does. Am I missing
something or is this a bug?
Best wishes,
Mats Kindahl (Timescale)
Attachments:
0001-Add-signal-safe-function-to-write-to-log.patch (text/x-patch, 3.7K) download
From 032ecd53ebc31d567ff28643a3083fe5405e38fd Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 24 May 2022 11:02:38 +0200
Subject: Add signal-safe function to write to log
Any functions that use `malloc` directly or indirectly are not
signal-safe, and `vfprintf` is not signal-safe but used from inside
`bgworker_die`. This can cause the system to end up in a deadlock and
not respond to signals.
This commit adds a signal-safe version of write_stderr() and uses it
from signal handlers rather than write_stderr().
---
src/backend/postmaster/bgworker.c | 16 ++++++++++++----
src/backend/utils/error/elog.c | 21 ++++++++++++++++-----
src/include/utils/elog.h | 6 ++++++
3 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 30682b63b3..3ad44d303b 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -719,16 +719,24 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
/*
* Standard SIGTERM handler for background workers
+ *
+ * Only use signal-safe function in signal handler, in particular, ereport()
+ * and and any printf()-like functions since they directly or indirectly use
+ * malloc().
*/
static void
bgworker_die(SIGNAL_ARGS)
{
PG_SETMASK(&BlockSig);
+ char message[256] = {0};
+ char* ptr = message;
+
+ ptr = stpncpy(ptr, "terminating background worker \"", sizeof(message) - (message - ptr));
+ ptr = stpncpy(ptr, MyBgworkerEntry->bgw_type, sizeof(message) - (message - ptr));
+ ptr = stpncpy(ptr, "\" due to administrator command", sizeof(message) - (message - ptr));
- ereport(FATAL,
- (errcode(ERRCODE_ADMIN_SHUTDOWN),
- errmsg("terminating background worker \"%s\" due to administrator command",
- MyBgworkerEntry->bgw_type)));
+ signal_safe_write_stderr(message, strlen(message));
+ die(postgres_signal_arg);
}
/*
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 55ee5423af..d8c94ba87f 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -54,11 +54,11 @@
*/
#include "postgres.h"
+#include <ctype.h>
#include <fcntl.h>
+#include <signal.h>
#include <time.h>
#include <unistd.h>
-#include <signal.h>
-#include <ctype.h>
#ifdef HAVE_SYSLOG
#include <syslog.h>
#endif
@@ -3384,23 +3384,34 @@ write_stderr(const char *fmt,...)
fflush(stderr);
#else
vsnprintf(errbuf, sizeof(errbuf), fmt, ap);
+ signal_safe_write_stderr(errbuf, strlen(errbuf));
+#endif
+ va_end(ap);
+}
+
+void
+signal_safe_write_stderr(const char *errbuf, size_t buflen)
+{
+#ifndef WIN32
+ /* If this fails, there is not much we can do, so ignore the error */
+ (void) write(fileno(stderr), errbuf, buflen);
+#else
/*
* On Win32, we print to stderr if running on a console, or write to
* eventlog if running as a service
*/
if (pgwin32_is_service()) /* Running as a service */
{
- write_eventlog(ERROR, errbuf, strlen(errbuf));
+ write_eventlog(ERROR, errbuf, buflen);
}
else
{
/* Not running as service, write to stderr */
- write_console(errbuf, strlen(errbuf));
+ write_console(errbuf, buflen);
fflush(stderr);
}
#endif
- va_end(ap);
}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f5c6cd904d..d6ed4e9c03 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -467,4 +467,10 @@ extern void set_syslog_parameters(const char *ident, int facility);
*/
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+/*
+ * Signal-safe function to write error messages. Can be used from inside
+ * signal-handlers.
+ */
+extern void signal_safe_write_stderr(const char* errbuf, size_t buflen);
+
#endif /* ELOG_H */
--
2.25.1
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-24 10:14 Julien Rouhaud <rjuju123@gmail.com>
parent: Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 8+ messages in thread
From: Julien Rouhaud @ 2022-05-24 10:14 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: pgsql-bugs@lists.postgresql.org
Hi,
On Tue, May 24, 2022 at 11:42:35AM +0200, Mats Kindahl wrote:
>
> Typically, signal-unsafe functions should not be called from signal
> handlers. In particular, calling malloc() directly or indirectly can cause
> deadlocks, making PostgreSQL unresponsive to signals.
>
> Unless I am missing something, bgworker_die uses ereport, which indirectly
> calls printf-like functions, which are not signal-safe since they use
> malloc(). In rare cases, this can lead to deadlocks with stacks that look
> like this (from https://github.com/timescale/timescaledb/issues/4200):
>
> #0 0x00007f0e4d1040eb in __lll_lock_wait_private () from
> target:/lib/x86_64-linux-gnu/libc.so.6
> [...]
> #3 malloc (size=53)
> [...]
> #7 0x000055b9212235b1 in errmsg ()
> #8 0x00007f0e27bf27a8 in handle_sigterm (postgres_signal_arg=15) at
> /build/timescaledb/src/bgw/scheduler.c:841
> #9 <signal handler called>
> [...]
> #13 free (ptr=<optimized out>)
> #14 0x00007f0e4db12cb4 in OPENSSL_LH_free () from
> target:/lib/x86_64-linux-gnu/libcrypto.so.1.1
> [...]
As far as I can see the problem comes from your handle_sigterm:
static void handle_sigterm(SIGNAL_ARGS)
{
/*
* do not use a level >= ERROR because we don't want to exit here but
* rather only during CHECK_FOR_INTERRUPTS
*/
ereport(LOG,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating TimescaleDB job scheduler due to administrator command")));
die(postgres_signal_arg);
}
As mentioned in the topmost comment of elog.c, there's an escape hatch for
out of memory situations, to make sure that a small enough message can be
displayed without trying to allocate memory. But this is only for ERROR or
higher levels. Using an ereport(LOG, ...) level in a sigterm handler indeed
isn't safe.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-24 12:10 Mats Kindahl <mats@timescale.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Mats Kindahl @ 2022-05-24 12:10 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Tue, May 24, 2022 at 12:14 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> Hi,
>
> On Tue, May 24, 2022 at 11:42:35AM +0200, Mats Kindahl wrote:
> >
> > Typically, signal-unsafe functions should not be called from signal
> > handlers. In particular, calling malloc() directly or indirectly can
> cause
> > deadlocks, making PostgreSQL unresponsive to signals.
> >
> > Unless I am missing something, bgworker_die uses ereport, which
> indirectly
> > calls printf-like functions, which are not signal-safe since they use
> > malloc(). In rare cases, this can lead to deadlocks with stacks that look
> > like this (from https://github.com/timescale/timescaledb/issues/4200):
> >
> > #0 0x00007f0e4d1040eb in __lll_lock_wait_private () from
> > target:/lib/x86_64-linux-gnu/libc.so.6
> > [...]
> > #3 malloc (size=53)
> > [...]
> > #7 0x000055b9212235b1 in errmsg ()
> > #8 0x00007f0e27bf27a8 in handle_sigterm (postgres_signal_arg=15) at
> > /build/timescaledb/src/bgw/scheduler.c:841
> > #9 <signal handler called>
> > [...]
> > #13 free (ptr=<optimized out>)
> > #14 0x00007f0e4db12cb4 in OPENSSL_LH_free () from
> > target:/lib/x86_64-linux-gnu/libcrypto.so.1.1
> > [...]
>
> As far as I can see the problem comes from your handle_sigterm:
>
> static void handle_sigterm(SIGNAL_ARGS)
> {
> /*
> * do not use a level >= ERROR because we don't want to exit here
> but
> * rather only during CHECK_FOR_INTERRUPTS
> */
> ereport(LOG,
> (errcode(ERRCODE_ADMIN_SHUTDOWN),
> errmsg("terminating TimescaleDB job scheduler due
> to administrator command")));
> die(postgres_signal_arg);
> }
>
> As mentioned in the topmost comment of elog.c, there's an escape hatch for
> out of memory situations, to make sure that a small enough message can be
> displayed without trying to allocate memory. But this is only for ERROR or
> higher levels. Using an ereport(LOG, ...) level in a sigterm handler
> indeed
> isn't safe.
>
Yes, and we have already fixed this in the TimescaleDB code, so this is not
an issue for us (https://github.com/timescale/timescaledb/pull/4323). (We
actually replace the bgworker_die with just die as signal handler.)
However, bgworker_die()---which is part of PostgreSQL and is the default
signal handler for background workers---is using ereport() and I think this
should be a problem for all error levels since the problem is in the usage
of the formatting function to format the error message, not where you write
it. This would mean that any existing background workers would have a
window for triggering this issue on shutdown.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-24 12:37 Julien Rouhaud <rjuju123@gmail.com>
parent: Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 8+ messages in thread
From: Julien Rouhaud @ 2022-05-24 12:37 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Tue, May 24, 2022 at 02:10:36PM +0200, Mats Kindahl wrote:
> On Tue, May 24, 2022 at 12:14 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > As mentioned in the topmost comment of elog.c, there's an escape hatch for
> > out of memory situations, to make sure that a small enough message can be
> > displayed without trying to allocate memory. But this is only for ERROR or
> > higher levels. Using an ereport(LOG, ...) level in a sigterm handler
> > indeed
> > isn't safe.
> >
>
> Yes, and we have already fixed this in the TimescaleDB code, so this is not
> an issue for us (https://github.com/timescale/timescaledb/pull/4323). (We
> actually replace the bgworker_die with just die as signal handler.)
>
> However, bgworker_die()---which is part of PostgreSQL and is the default
> signal handler for background workers---is using ereport() and I think this
> should be a problem for all error levels since the problem is in the usage
> of the formatting function to format the error message, not where you write
> it. This would mean that any existing background workers would have a
> window for triggering this issue on shutdown.
Yes, but it's using ereport with FATAL level, so if it's the top level message
the ErrorContext should be in initial state or have been reset previously, and
if it's not then the escape hatch will reset the context. So in any case there
will be a guarantee to have at least 8kB available in that context, that any
palloc will be able to use to format the message.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-24 14:15 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2022-05-24 14:15 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Mats Kindahl <mats@timescale.com>; pgsql-bugs@lists.postgresql.org
Julien Rouhaud <rjuju123@gmail.com> writes:
> Yes, but it's using ereport with FATAL level, so if it's the top level message
> the ErrorContext should be in initial state or have been reset previously, and
> if it's not then the escape hatch will reset the context. So in any case there
> will be a guarantee to have at least 8kB available in that context, that any
> palloc will be able to use to format the message.
ereport() itself is just the tip of the iceberg; even if it's safe
(which I concur it isn't), there's also the atexit/on_proc_exit
functions that are likely to be called during shutdown. So yeah,
this coding is not too safe. I'm not sure that getting rid of it
would be a net win though, as we'd replace it-might-crash hazards
with it-might-never-exit hazards, from bgworkers that neglect to
respond to ShutdownRequestPending.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-25 00:45 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 8+ messages in thread
From: Michael Paquier @ 2022-05-25 00:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Mats Kindahl <mats@timescale.com>; pgsql-bugs@lists.postgresql.org
On Tue, May 24, 2022 at 10:15:55AM -0400, Tom Lane wrote:
> ereport() itself is just the tip of the iceberg; even if it's safe
> (which I concur it isn't), there's also the atexit/on_proc_exit
> functions that are likely to be called during shutdown. So yeah,
> this coding is not too safe. I'm not sure that getting rid of it
> would be a net win though, as we'd replace it-might-crash hazards
> with it-might-never-exit hazards, from bgworkers that neglect to
> respond to ShutdownRequestPending.
Hmm. Shouldn't we worry about FloatExceptionHandler() that gets used
on SIGFPE?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-25 01:01 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 8+ messages in thread
From: Andres Freund @ 2022-05-25 01:01 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Julien Rouhaud <rjuju123@gmail.com>; Mats Kindahl <mats@timescale.com>; pgsql-bugs@lists.postgresql.org
Hi,
On 2022-05-25 09:45:31 +0900, Michael Paquier wrote:
> On Tue, May 24, 2022 at 10:15:55AM -0400, Tom Lane wrote:
> > ereport() itself is just the tip of the iceberg; even if it's safe
> > (which I concur it isn't), there's also the atexit/on_proc_exit
> > functions that are likely to be called during shutdown. So yeah,
> > this coding is not too safe. I'm not sure that getting rid of it
> > would be a net win though, as we'd replace it-might-crash hazards
> > with it-might-never-exit hazards, from bgworkers that neglect to
> > respond to ShutdownRequestPending.
IMO the it-might-never-exit hazard is lower, and can be addressed by the
authors of bgworkers by adding the checks. Whereas the it-might-crash can't
really be addressed well (except reimplementing the signal handler).
It might be worth adding some debugging infrastructure to make it easier to
find spots that don't check various forms of interrupts. If we required a
function to check for things like ShutdownRequestPending, we could check in
that function how long it's been since the last check, subtracting time spent
in WaitLatch etc.
> Hmm. Shouldn't we worry about FloatExceptionHandler() that gets used on
> SIGFPE?
That probably is comparatively low-risk, because SIGFPE is a synchronous
signal. So it'll be triggered in the middle of a floating point math operation
that where divide-by-zero wouldn't be a bug - which we really really shouldn't
have while holding a spinlock or such.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use of signal-unsafe functions from signal handlers
@ 2022-05-25 01:25 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 8+ messages in thread
From: Tom Lane @ 2022-05-25 01:25 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Mats Kindahl <mats@timescale.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> Hmm. Shouldn't we worry about FloatExceptionHandler() that gets used
> on SIGFPE?
SIGFPE is generated synchronously, so that one's not really a problem,
I think: it can only interrupt code that is doing trap-prone arithmetic.
Anyway that code's been like that for ages and nobody has reported an
issue there.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2022-05-25 01:25 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-24 09:42 Use of signal-unsafe functions from signal handlers Mats Kindahl <mats@timescale.com>
2022-05-24 10:14 ` Julien Rouhaud <rjuju123@gmail.com>
2022-05-24 12:10 ` Mats Kindahl <mats@timescale.com>
2022-05-24 12:37 ` Julien Rouhaud <rjuju123@gmail.com>
2022-05-24 14:15 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-05-25 00:45 ` Michael Paquier <michael@paquier.xyz>
2022-05-25 01:01 ` Andres Freund <andres@anarazel.de>
2022-05-25 01:25 ` 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