Avoid calling proc_exit() in processes forked by system().

The SIGTERM handler for the startup process immediately calls
proc_exit() for the duration of the restore_command, i.e., a call
to system().  This system() call forks a new process to execute the
shell command, and this child process inherits the parent's signal
handlers.  If both the parent and child processes receive SIGTERM,
both will attempt to call proc_exit().  This can end badly.  For
example, both processes will try to remove themselves from the
PGPROC shared array.

To fix this problem, this commit adds a check in
StartupProcShutdownHandler() to see whether MyProcPid == getpid().
If they match, this is the parent process, and we can proc_exit()
like before.  If they do not match, this is a child process, and we
just emit a message to STDERR (in a signal safe manner) and
_exit(), thereby skipping any problematic exit callbacks.

This commit also adds checks in proc_exit(), ProcKill(), and
AuxiliaryProcKill() that verify they are not being called within
such child processes.

Suggested-by: Andres Freund
Reviewed-by: Thomas Munro, Andres Freund
Discussion: https://postgr.es/m/Y9nGDSgIm83FHcad%40paquier.xyz
Discussion: https://postgr.es/m/20230223231503.GA743455%40nathanxps13
Backpatch-through: 11

diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index 5c856c5261bb8aeaf60343e310282356d89d9cf0..4f8f3975a5cb1bc199cfddedb6f091707d6fe94a 100644 (file)
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
  */
 #include "postgres.h"
 
+#include <unistd.h>
+
 #include "access/xlog.h"
 #include "access/xlogrecovery.h"
 #include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
        int                     save_errno = errno;
 
        if (in_restore_command)
-               proc_exit(1);
+       {
+               /*
+                * If we are in a child process (e.g., forked by system() in
+                * RestoreArchivedFile()), we don't want to call any exit callbacks.
+                * The parent will take care of that.
+                */
+               if (MyProcPid == (int) getpid())
+                       proc_exit(1);
+               else
+               {
+                       write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+                       _exit(1);
+               }
+       }
        else
                shutdown_requested = true;
        WakeupRecovery();

diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 4a4e3f302c5e12a6d5bfba45ac0272707c6e608c..e9787264d4b6f8b6a3a340f52e7831f71741aad8 100644 (file)
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,10 @@ static int on_proc_exit_index,
 void
 proc_exit(int code)
 {
+       /* not safe if forked by system(), etc. */
+       if (MyProcPid != (int) getpid())
+               elog(PANIC, "proc_exit() called in child process");
+
        /* Clean up everything that must be cleaned up */
        proc_exit_prepare(code);
 

diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b4c0625d3654b43b64acfa5d207db86c435c5b9f..1b514f44e3349d802735a649729b72ee454d5978 100644 (file)
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -807,6 +807,10 @@ ProcKill(int code, Datum arg)
 
        Assert(MyProc != NULL);
 
+       /* not safe if forked by system(), etc. */
+       if (MyProc->pid != (int) getpid())
+               elog(PANIC, "ProcKill() called in child process");
+
        /* Make sure we're out of the sync rep lists */
        SyncRepCleanupAtProcExit();
 
@@ -929,6 +933,10 @@ AuxiliaryProcKill(int code, Datum arg)
 
        Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
 
+       /* not safe if forked by system(), etc. */
+       if (MyProc->pid != (int) getpid())
+               elog(PANIC, "AuxiliaryProcKill() called in child process");
+
        auxproc = &AuxiliaryProcs[proctype];
 
        Assert(MyProc == auxproc);

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 55ee5423afbc9966486f604409588ea466742a76..6fa4f2c3424155259e109f9ddb91e7aa8e38c2b7 100644 (file)
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3404,6 +3404,34 @@ write_stderr(const char *fmt,...)
 }
 
 
+/*
+ * Write a message to STDERR using only async-signal-safe functions.  This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *str)
+{
+       int                     nwritten = 0;
+       int                     ntotal = strlen(str);
+
+       while (nwritten < ntotal)
+       {
+               int                     rc;
+
+               rc = write(STDERR_FILENO, str + nwritten, ntotal - nwritten);
+
+               /* Just give up on error.  There isn't much else we can do. */
+               if (rc == -1)
+                       return;
+
+               nwritten += rc;
+       }
+}
+
+
 /*
  * Adjust the level of a recovery-related message per trace_recovery_messages.
  *

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f5c6cd904de925db6819e56e4869a27b620b090c..561c15e7d11822eeb2cc8b2c453d7db3fbea1d6f 100644 (file)
--- 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);
 
+/*
+ * Write a message to STDERR using only async-signal-safe functions.  This can
+ * be used to safely emit a message from a signal handler.
+ */
+extern void write_stderr_signal_safe(const char *fmt);
+
 #endif                                                 /* ELOG_H */