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

diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 9576251780c26e999f19011759ea8b8580cf726e..c19070c23d231b40f25468d6fc2ed649c68af212 100644 (file)
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -345,27 +345,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 1dbd23cb77304b702b5c6ebc3de8f74961d7b318..0f4cbf4b6545edbf30ddedef3637d4e7638c622f 100644 (file)
--- 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 b0104d384ea0d791e754824eda8a13dedec2b2f4..e41dbc6800f8caebde08d5cd4c3001fe01bce0b9 100644 (file)
--- 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 9e7ad189cd12c699288fe141b6b3ecd08a956e04..32eeee25659e1bd7f0fb576012f28ab4b67e5972 100644 (file)
--- 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 5f181d118da9098959ba38d7850923047471f6a4..13054340c7972377972cf508f64caa109efdaded 100644 (file)
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -775,27 +775,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.
-    */
-   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.)
+    * 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.)
     */
-   exit(2);
+   _exit(2);
 }
 
 /*

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 538f9f3d32d966df644d26a8983815d85ef15e40..cf4d4ec356b6f654c45f6338689b6f3232266db9 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2568,6 +2568,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...
     */
@@ -2582,24 +2592,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);
 }
 
 /*