Use of signal-unsafe functions from signal handlers

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.

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.

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.

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

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

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

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