From 3b00fdba9f20b641d5d3c2b781cd435b23540e61 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 14 Feb 2024 14:52:14 -0600
Subject: Check that MyProcPid == getpid() in backend signal handlers.

In commit 97550c0711, we added a similar check to the SIGTERM
handler for the startup process.  This commit adds this check to
backend signal handlers installed with pqsignal().  This is done by
using a wrapper function that performs the check before calling the
actual handler.

The hope is that this will offer more general protection against
child processes of Postgres backends inadvertently modifying shared
memory due to inherited signal handlers.  Another potential
follow-up improvement is to use this wrapper handler function to
restore errno instead of relying on each individual handler
function to do so.

This commit makes the changes in commit 97550c0711 obsolete but
leaves reverting it for a follow-up commit.

Reviewed-by: Andres Freund, Noah Misch
Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
---
 src/port/pqsignal.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 2 deletions(-)

(limited to 'src')

diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 9b884890e6c..92382b3c348 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -46,23 +46,99 @@
 #include "c.h"
 
 #include <signal.h>
+#ifndef FRONTEND
+#include <unistd.h>
+#endif
 
 #ifndef FRONTEND
 #include "libpq/pqsignal.h"
+#include "miscadmin.h"
+#endif
+
+#ifdef PG_SIGNAL_COUNT			/* Windows */
+#define PG_NSIG (PG_SIGNAL_COUNT)
+#elif defined(NSIG)
+#define PG_NSIG (NSIG)
+#else
+#define PG_NSIG (64)			/* XXX: wild guess */
+#endif
+
+/* Check a couple of common signals to make sure PG_NSIG is accurate. */
+StaticAssertDecl(SIGUSR2 < PG_NSIG, "SIGUSR2 >= PG_NSIG");
+StaticAssertDecl(SIGHUP < PG_NSIG, "SIGHUP >= PG_NSIG");
+StaticAssertDecl(SIGTERM < PG_NSIG, "SIGTERM >= PG_NSIG");
+StaticAssertDecl(SIGALRM < PG_NSIG, "SIGALRM >= PG_NSIG");
+
+static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
+
+/*
+ * Except when called with SIG_IGN or SIG_DFL, pqsignal() sets up this function
+ * as the handler for all signals.  This wrapper handler function checks that
+ * it is called within a process that the server knows about (i.e., any process
+ * that has called InitProcessGlobals(), such as a client backend), and not a
+ * child process forked by system(3), etc.  This check ensures that such child
+ * processes do not modify shared memory, which is often detrimental.  If the
+ * check succeeds, the function originally provided to pqsignal() is called.
+ * Otherwise, the default signal handler is installed and then called.
+ */
+static void
+wrapper_handler(SIGNAL_ARGS)
+{
+#ifndef FRONTEND
+
+	/*
+	 * We expect processes to set MyProcPid before calling pqsignal() or
+	 * before accepting signals.
+	 */
+	Assert(MyProcPid);
+	Assert(MyProcPid != PostmasterPid || !IsUnderPostmaster);
+
+	if (unlikely(MyProcPid != (int) getpid()))
+	{
+		pqsignal(postgres_signal_arg, SIG_DFL);
+		raise(postgres_signal_arg);
+		return;
+	}
 #endif
 
+	(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
+}
+
 /*
  * Set up a signal handler, with SA_RESTART, for signal "signo"
  *
  * Returns the previous handler.
+ *
+ * NB: If called within a signal handler, race conditions may lead to bogus
+ * return values.  You should either avoid calling this within signal handlers
+ * or ignore the return value.
+ *
+ * XXX: Since no in-tree callers use the return value, and there is little
+ * reason to do so, it would be nice if we could convert this to a void
+ * function instead of providing potentially-bogus return values.
+ * Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,
+ * which in turn requires an SONAME bump, which is probably not worth it.
  */
 pqsigfunc
 pqsignal(int signo, pqsigfunc func)
 {
+	pqsigfunc	orig_func = pqsignal_handlers[signo];	/* assumed atomic */
 #if !(defined(WIN32) && defined(FRONTEND))
 	struct sigaction act,
 				oact;
+#else
+	pqsigfunc	ret;
+#endif
 
+	Assert(signo < PG_NSIG);
+
+	if (func != SIG_IGN && func != SIG_DFL)
+	{
+		pqsignal_handlers[signo] = func;	/* assumed atomic */
+		func = wrapper_handler;
+	}
+
+#if !(defined(WIN32) && defined(FRONTEND))
 	act.sa_handler = func;
 	sigemptyset(&act.sa_mask);
 	act.sa_flags = SA_RESTART;
@@ -72,9 +148,15 @@ pqsignal(int signo, pqsigfunc func)
 #endif
 	if (sigaction(signo, &act, &oact) < 0)
 		return SIG_ERR;
-	return oact.sa_handler;
+	else if (oact.sa_handler == wrapper_handler)
+		return orig_func;
+	else
+		return oact.sa_handler;
 #else
 	/* Forward to Windows native signal system. */
-	return signal(signo, func);
+	if ((ret = signal(signo, func)) == wrapper_handler)
+		return orig_func;
+	else
+		return ret;
 #endif
 }
-- 
cgit v1.2.3