Don't run atexit callbacks in quickdie signal handlers.
authorHeikki Linnakangas
Wed, 8 Aug 2018 16:08:10 +0000 (19:08 +0300)
committerHeikki Linnakangas
Wed, 8 Aug 2018 16:09:30 +0000 (19:09 +0300)
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
src/backend/postmaster/bgwriter.c
src/backend/postmaster/checkpointer.c
src/backend/postmaster/startup.c
src/backend/postmaster/walwriter.c
src/backend/replication/walreceiver.c
src/backend/tcop/postgres.c

index b690b1b0b85e905572919f86255c0f6c09f54b16..46a2329e1fd191b0adfd600d246cd299ad99e099 100644 (file)
@@ -636,28 +636,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);
 }
 
 /*
index 9ad74ee977f18a0c8c5a4c7dd5bf6414d24b38ae..418be9725f01f01e88d05e59baae294d92cbe0d3 100644 (file)
@@ -409,27 +409,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 */
index e48ebd557ffb2b1571d61f44d9295b4d9f0234f3..6ad2427973bd1b686aac40b0883619fe68e1b6df 100644 (file)
@@ -822,27 +822,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 */
index 4693a1bb86f526aeefc5c0b4f5097e6e8947f26f..4734c3693082bea1cce6603fec5d8b0c60cc1141 100644 (file)
@@ -69,27 +69,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);
 }
 
 
index 7b89e02428bf3b8d1d38b0100c3256823148aed0..380f3f2741fa7feda1e5f09995bb1cb0517d9b09 100644 (file)
@@ -319,27 +319,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 */
index 8c90d6ce1dd393a1c63390648bbf43811d33683d..bde03978c9664623e341c9542132e24f0ebbab63 100644 (file)
@@ -846,27 +846,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);
 }
 
 /*
index 63a1994e61ed3372414dc6f16d109c1706ad64c6..a10f996fc8eec87056de757a94820eba8ad25b78 100644 (file)
@@ -2575,6 +2575,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...
     */
@@ -2589,24 +2599,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);
 }
 
 /*