From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com
Subject: Re: common signal handler protection
Date: Mon, 18 Dec 2023 11:32:47 -0600
Message-ID: <20231218173247.GA582324@nathanxps13> (raw)
In-Reply-To: <20231129031652.GB479372@nathanxps13>
References: <20231121212008.GA3742740@nathanxps13>
<20231121224006.GA3747740@nathanxps13>
<20231122215944.GA3797360@nathanxps13>
<20231122225945.3kgclsgz5lqmtnan@awork3.anarazel.de>
<20231127221625.GB140989@nathanxps13>
<20231128213955.GA474877@nathanxps13>
<20231129023750.xf5c5psirsqhhy2t@awork3.anarazel.de>
<20231129031652.GB479372@nathanxps13>
rebased for cfbot
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.comAttachments:download all (3 files, .tar.gz)v5-0001-Check-that-MyProcPid-getpid-in-all-signal-handler.patch (text/x-diff, 4.7K) download
From c5fc2186960c483d53789f27fcf84771e98c5ca3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 28 Nov 2023 14:58:20 -0600
Subject: [PATCH v5 1/3] Check that MyProcPid == getpid() in all signal
handlers.
In commit 97550c0711, we added a similar check to the SIGTERM
handler for the startup process. This commit adds this check to
all 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
Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
---
src/port/pqsignal.c | 86 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 84 insertions(+), 2 deletions(-)
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.cindex 23e7f685c1..f04a7153de 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
}
--
2.25.1
v5-0002-Centralize-logic-for-restoring-errno-in-signal-ha.patch (text/x-diff, 10.3K) download
From 44bae78090d44f2ecdc9fbaea43d9adc1827c445 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 28 Nov 2023 15:18:13 -0600
Subject: [PATCH v5 2/3] Centralize logic for restoring errno in signal
handlers.
Presently, we rely on each individual signal handler to save the
initial value of errno and then restore it before returning if
needed. This is easily forgotten and, if missed, often goes
undetected for a long time.
In commit XXXXXXXXXX, we introduced a wrapper signal handler
function that checks whether MyProcPid matches getpid(). This
commit moves the aforementioned errno restoration code from the
individual signal handlers to the new wrapper handler so that we no
longer need to worry about missing it.
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
---
src/backend/postmaster/autovacuum.c | 4 ----
src/backend/postmaster/checkpointer.c | 4 ----
src/backend/postmaster/interrupt.c | 8 --------
src/backend/postmaster/pgarch.c | 4 ----
src/backend/postmaster/postmaster.c | 16 ----------------
src/backend/postmaster/startup.c | 12 ------------
src/backend/postmaster/syslogger.c | 4 ----
src/backend/replication/walsender.c | 4 ----
src/backend/storage/ipc/latch.c | 4 ----
src/backend/storage/ipc/procsignal.c | 4 ----
src/backend/tcop/postgres.c | 8 --------
src/backend/utils/misc/timeout.c | 4 ----
src/fe_utils/cancel.c | 3 ---
src/port/pqsignal.c | 6 ++++++
14 files changed, 6 insertions(+), 79 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.cindex b04fcfc8c8..56067ea4d3 100644--- a/src/backend/postmaster/autovacuum.c+++ b/src/backend/postmaster/autovacuum.c@@ -1410,12 +1410,8 @@ AutoVacWorkerFailed(void)
static void
avl_sigusr2_handler(SIGNAL_ARGS)
{
- int save_errno = errno;-
got_SIGUSR2 = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.cindex dc2da5a2cd..b8e002074d 100644--- a/src/backend/postmaster/checkpointer.c+++ b/src/backend/postmaster/checkpointer.c@@ -834,15 +834,11 @@ IsCheckpointOnSchedule(double progress)
static void
ReqCheckpointHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
/*
* The signaling process should have set ckpt_flags nonzero, so all we
* need do is ensure that our main loop gets kicked out of any wait.
*/
SetLatch(MyLatch);
-- errno = save_errno;
}
diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.cindex 6d4bd76bf8..fa7833f673 100644--- a/src/backend/postmaster/interrupt.c+++ b/src/backend/postmaster/interrupt.c@@ -60,12 +60,8 @@ HandleMainLoopInterrupts(void)
void
SignalHandlerForConfigReload(SIGNAL_ARGS)
{
- int save_errno = errno;-
ConfigReloadPending = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
@@ -108,10 +104,6 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
void
SignalHandlerForShutdownRequest(SIGNAL_ARGS)
{
- int save_errno = errno;-
ShutdownRequestPending = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.cindex a2555e8578..7d9e26bbe5 100644--- a/src/backend/postmaster/pgarch.c+++ b/src/backend/postmaster/pgarch.c@@ -283,13 +283,9 @@ PgArchWakeup(void)
static void
pgarch_waken_stop(SIGNAL_ARGS)
{
- int save_errno = errno;-
/* set flag to do a final cycle and shut down afterwards */
ready_to_stop = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.cindex 651b85ea74..a1d3a46434 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -2610,12 +2610,8 @@ InitProcessGlobals(void)
static void
handle_pm_pmsignal_signal(SIGNAL_ARGS)
{
- int save_errno = errno;-
pending_pm_pmsignal = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
@@ -2624,12 +2620,8 @@ handle_pm_pmsignal_signal(SIGNAL_ARGS)
static void
handle_pm_reload_request_signal(SIGNAL_ARGS)
{
- int save_errno = errno;-
pending_pm_reload_request = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
@@ -2707,8 +2699,6 @@ process_pm_reload_request(void)
static void
handle_pm_shutdown_request_signal(SIGNAL_ARGS)
{
- int save_errno = errno;-
switch (postgres_signal_arg)
{
case SIGTERM:
@@ -2725,8 +2715,6 @@ handle_pm_shutdown_request_signal(SIGNAL_ARGS)
break;
}
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
@@ -2886,12 +2874,8 @@ process_pm_shutdown_request(void)
static void
handle_pm_child_exit_signal(SIGNAL_ARGS)
{
- int save_errno = errno;-
pending_pm_child_exit = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/*
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.cindex 082c870e03..234817cd54 100644--- a/src/backend/postmaster/startup.c+++ b/src/backend/postmaster/startup.c@@ -95,32 +95,22 @@ static void StartupProcExit(int code, Datum arg);
static void
StartupProcTriggerHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
promote_signaled = true;
WakeupRecovery();
-- errno = save_errno;
}
/* SIGHUP: set flag to re-read config file at next convenient time */
static void
StartupProcSigHupHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
got_SIGHUP = true;
WakeupRecovery();
-- errno = save_errno;
}
/* SIGTERM: set flag to abort redo and exit */
static void
StartupProcShutdownHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
if (in_restore_command)
{
/*
@@ -139,8 +129,6 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
else
shutdown_requested = true;
WakeupRecovery();
-- errno = save_errno;
}
/*
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.cindex 96dd03d9e0..a47838d505 100644--- a/src/backend/postmaster/syslogger.c+++ b/src/backend/postmaster/syslogger.c@@ -1642,10 +1642,6 @@ RemoveLogrotateSignalFiles(void)
static void
sigUsr1Handler(SIGNAL_ARGS)
{
- int save_errno = errno;-
rotation_requested = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.cindex 3bc9c82389..a809c2202b 100644--- a/src/backend/replication/walsender.c+++ b/src/backend/replication/walsender.c@@ -3245,12 +3245,8 @@ HandleWalSndInitStopping(void)
static void
WalSndLastCycleHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
got_SIGUSR2 = true;
SetLatch(MyLatch);
-- errno = save_errno;
}
/* Set up signal handlers */
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.cindex d8a69990b3..359f648703 100644--- a/src/backend/storage/ipc/latch.c+++ b/src/backend/storage/ipc/latch.c@@ -2243,12 +2243,8 @@ GetNumRegisteredWaitEvents(WaitEventSet *set)
static void
latch_sigurg_handler(SIGNAL_ARGS)
{
- int save_errno = errno;-
if (waiting)
sendSelfPipeByte();
-- errno = save_errno;
}
/* Send one byte to the self-pipe, to wake up WaitLatch */
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.cindex b7427906de..8e65009a89 100644--- a/src/backend/storage/ipc/procsignal.c+++ b/src/backend/storage/ipc/procsignal.c@@ -638,8 +638,6 @@ CheckProcSignal(ProcSignalReason reason)
void
procsignal_sigusr1_handler(SIGNAL_ARGS)
{
- int save_errno = errno;-
if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT))
HandleCatchupInterrupt();
@@ -683,6 +681,4 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
SetLatch(MyLatch);
-- errno = save_errno;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.cindex 7298a187d1..c3360343ac 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -2970,8 +2970,6 @@ quickdie(SIGNAL_ARGS)
void
die(SIGNAL_ARGS)
{
- int save_errno = errno;-
/* Don't joggle the elbow of proc_exit */
if (!proc_exit_inprogress)
{
@@ -2993,8 +2991,6 @@ die(SIGNAL_ARGS)
*/
if (DoingCommandRead && whereToSendOutput != DestRemote)
ProcessInterrupts();
-- errno = save_errno;
}
/*
@@ -3004,8 +3000,6 @@ die(SIGNAL_ARGS)
void
StatementCancelHandler(SIGNAL_ARGS)
{
- int save_errno = errno;-
/*
* Don't joggle the elbow of proc_exit
*/
@@ -3017,8 +3011,6 @@ StatementCancelHandler(SIGNAL_ARGS)
/* If we're still here, waken anything waiting on the process latch */
SetLatch(MyLatch);
-- errno = save_errno;
}
/* signal handler for floating point exception */
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.cindex 8ab755d363..c9790f8cdb 100644--- a/src/backend/utils/misc/timeout.c+++ b/src/backend/utils/misc/timeout.c@@ -363,8 +363,6 @@ schedule_alarm(TimestampTz now)
static void
handle_sig_alarm(SIGNAL_ARGS)
{
- int save_errno = errno;-
/*
* Bump the holdoff counter, to make sure nothing we call will process
* interrupts directly. No timeout handler should do that, but these
@@ -452,8 +450,6 @@ handle_sig_alarm(SIGNAL_ARGS)
}
RESUME_INTERRUPTS();
-- errno = save_errno;
}
diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.cindex 10c0cd4554..bffef24d63 100644--- a/src/fe_utils/cancel.c+++ b/src/fe_utils/cancel.c@@ -152,7 +152,6 @@ ResetCancelConn(void)
static void
handle_sigint(SIGNAL_ARGS)
{
- int save_errno = errno;
char errbuf[256];
CancelRequested = true;
@@ -173,8 +172,6 @@ handle_sigint(SIGNAL_ARGS)
write_stderr(errbuf);
}
}
-- errno = save_errno; /* just in case the write changed it */
}
/*
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.cindex f04a7153de..775f2a40af 100644--- a/src/port/pqsignal.c+++ b/src/port/pqsignal.c@@ -80,10 +80,14 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
* 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.
+ *+ * This wrapper also handles restoring the value of errno.
*/
static void
wrapper_handler(SIGNAL_ARGS)
{
+ int save_errno = errno;+
#ifndef FRONTEND
/*
@@ -102,6 +106,8 @@ wrapper_handler(SIGNAL_ARGS)
#endif
(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
++ errno = save_errno;
}
/*
--
2.25.1
v5-0003-Revert-Avoid-calling-proc_exit-in-processes-forke.patch (text/x-diff, 3.3K) download
From f91530c7bb1590530f9174a62c4c73cf1544aca9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 28 Nov 2023 15:25:03 -0600
Subject: [PATCH v5 3/3] Revert "Avoid calling proc_exit() in processes forked
by system()."
Thanks to commit XXXXXXXXXX, this check in the SIGTERM handler for
the startup process is now obsolete and can be removed. Instead of
leaving around the dead function write_stderr_signal_safe(), I've
opted to just remove it for now. Thanks to modern version control
software, it will be trivial to dig it up if it is ever needed in
the future.
This partially reverts commit 97550c0711.
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
---
src/backend/postmaster/startup.c | 17 +----------------
src/backend/utils/error/elog.c | 28 ----------------------------
src/include/utils/elog.h | 6 ------
3 files changed, 1 insertion(+), 50 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.cindex 234817cd54..dfdbb56adb 100644--- a/src/backend/postmaster/startup.c+++ b/src/backend/postmaster/startup.c@@ -19,8 +19,6 @@
*/
#include "postgres.h"
-#include <unistd.h>-
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -112,20 +110,7 @@ static void
StartupProcShutdownHandler(SIGNAL_ARGS)
{
if (in_restore_command)
- {- /*- * 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);- }- }+ proc_exit(1);
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.cindex 6ea575a53b..2e86dd44d9 100644--- a/src/backend/utils/error/elog.c+++ b/src/backend/utils/error/elog.c@@ -3731,31 +3731,3 @@ write_stderr(const char *fmt,...)
#endif
va_end(ap);
}
---/*- * 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;- }-}diff --git a/src/include/utils/elog.h b/src/include/utils/elog.hindex 0971d5ce33..b25ea65dff 100644--- a/src/include/utils/elog.h+++ b/src/include/utils/elog.h@@ -536,10 +536,4 @@ extern void write_jsonlog(ErrorData *edata);
*/
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 */
--
2.25.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, andres@anarazel.de, noah@leadboat.com
Subject: Re: common signal handler protection
In-Reply-To: <20231218173247.GA582324@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox