From d5a9b706ea93a95d9359066488c33aee33a695bc Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 8 Aug 2018 19:08:10 +0300 Subject: [PATCH] Don't run atexit callbacks in quickdie signal handlers. exit() is not async-signal safe. Even if the libc implementation is, 3rd party libraries might have installed unsafe atexit() callbacks. After receiving SIGQUIT, we really just want to exit as quickly as possible, so we don't really want to run the atexit() callbacks anyway. The original report by Jimmy Yih was a self-deadlock in startup_die(). However, this patch doesn't address that scenario; the signal handling while waiting for the startup packet is more complicated. But at least this alleviates similar problems in the SIGQUIT handlers, like that reported by Asim R P later in the same thread. Backpatch to 9.3 (all supported versions). Discussion: https://www.postgresql.org/message-id/CAOMx_OAuRUHiAuCg2YgicZLzPVv5d9_H4KrL_OFsFP%3DVPekigA%40mail.gmail.com --- src/backend/postmaster/bgworker.c | 25 ++++++++------------- src/backend/postmaster/bgwriter.c | 24 ++++++++------------ src/backend/postmaster/checkpointer.c | 24 ++++++++------------ src/backend/postmaster/startup.c | 24 ++++++++------------ src/backend/postmaster/walwriter.c | 24 ++++++++------------ src/backend/replication/walreceiver.c | 32 +++++++++++---------------- src/backend/tcop/postgres.c | 32 ++++++++++++++++----------- 7 files changed, 77 insertions(+), 108 deletions(-) diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index f328b1d4db1..0c54b55a4ef 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -515,28 +515,21 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel) static void bgworker_quickdie(SIGNAL_ARGS) { - sigaddset(&BlockSig, SIGQUIT); /* prevent nested calls */ - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } /* diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index 780ee3bdcb0..57292929853 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -408,27 +408,21 @@ BackgroundWriterMain(void) static void bg_quickdie(SIGNAL_ARGS) { - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } /* SIGHUP: set flag to re-read config file at next convenient time */ diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 2ac3061d974..4b7c4bc6b9f 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -813,27 +813,21 @@ IsCheckpointOnSchedule(double progress) static void chkpt_quickdie(SIGNAL_ARGS) { - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } /* SIGHUP: set flag to re-read config file at next convenient time */ diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index a116d029f2d..44ca1ade3b8 100644 --- a/src/backend/postmaster/startup.c +++ b/src/backend/postmaster/startup.c @@ -68,27 +68,21 @@ static void StartupProcSigHupHandler(SIGNAL_ARGS); static void startupproc_quickdie(SIGNAL_ARGS) { - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c index 0826f8874ca..ee79305c224 100644 --- a/src/backend/postmaster/walwriter.c +++ b/src/backend/postmaster/walwriter.c @@ -328,27 +328,21 @@ WalWriterMain(void) static void wal_quickdie(SIGNAL_ARGS) { - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } /* SIGHUP: set flag to re-read config file at next convenient time */ diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index ff76dbb88ce..23c7ae41d46 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -778,27 +778,21 @@ WalRcvShutdownHandler(SIGNAL_ARGS) static void WalRcvQuickDieHandler(SIGNAL_ARGS) { - PG_SETMASK(&BlockSig); - /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we use _exit(2) not _exit(0). This is to force the postmaster + * into a system reset cycle if someone sends a manual SIGQUIT to a + * random backend. This is necessary precisely because we don't clean up + * our shared memory state. (The "dead man switch" mechanism in + * pmsignal.c should ensure the postmaster sees this as a crash, too, but + * no harm in being doubly sure.) */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random - * backend. This is necessary precisely because we don't clean up our - * shared memory state. (The "dead man switch" mechanism in pmsignal.c - * should ensure the postmaster sees this as a crash, too, but no harm in - * being doubly sure.) - */ - exit(2); + _exit(2); } /* diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 7a1b49a9275..5903b228905 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -2559,6 +2559,16 @@ quickdie(SIGNAL_ARGS) whereToSendOutput = DestNone; /* + * Notify the client before exiting, to give a clue on what happened. + * + * It's dubious to call ereport() from a signal handler. It is certainly + * not async-signal safe. But it seems better to try, than to disconnect + * abruptly and leave the client wondering what happened. It's remotely + * possible that we crash or hang while trying to send the message, but + * receiving a SIGQUIT is a sign that something has already gone badly + * wrong, so there's not much to lose. Assuming the postmaster is still + * running, it will SIGKILL us soon if we get stuck for some reason. + * * Ideally this should be ereport(FATAL), but then we'd not get control * back... */ @@ -2573,24 +2583,20 @@ quickdie(SIGNAL_ARGS) " database and repeat your command."))); /* - * We DO NOT want to run proc_exit() callbacks -- we're here because - * shared memory may be corrupted, so we don't want to try to clean up our - * transaction. Just nail the windows shut and get out of town. Now that - * there's an atexit callback to prevent third-party code from breaking - * things by calling exit() directly, we have to reset the callbacks - * explicitly to make this work as intended. - */ - on_exit_reset(); - - /* - * Note we do exit(2) not exit(0). This is to force the postmaster into a - * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random + * We DO NOT want to run proc_exit() or atexit() callbacks -- we're here + * because shared memory may be corrupted, so we don't want to try to + * clean up our transaction. Just nail the windows shut and get out of + * town. The callbacks wouldn't be safe to run from a signal handler, + * anyway. + * + * Note we do _exit(2) not _exit(0). This is to force the postmaster into + * a system reset cycle if someone sends a manual SIGQUIT to a random * backend. This is necessary precisely because we don't clean up our * shared memory state. (The "dead man switch" mechanism in pmsignal.c * should ensure the postmaster sees this as a crash, too, but no harm in * being doubly sure.) */ - exit(2); + _exit(2); } /*