From 28e46325091dfac5c6ab9ea1e047a8d09dbd16e7 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Wed, 14 Feb 2024 16:34:18 -0600 Subject: [PATCH] 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 3b00fdba9f, 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, Noah Misch Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13 --- doc/src/sgml/sources.sgml | 8 -------- 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 ++++++ 15 files changed, 6 insertions(+), 87 deletions(-) diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml index 5d1d510f8e..0dae4d9158 100644 --- a/doc/src/sgml/sources.sgml +++ b/doc/src/sgml/sources.sgml @@ -1007,18 +1007,10 @@ MemoryContextSwitchTo(MemoryContext context) static void handle_sighup(SIGNAL_ARGS) { - int save_errno = errno; - got_SIGHUP = true; SetLatch(MyLatch); - - errno = save_errno; } - errno is saved and restored because - SetLatch() might change it. If that were not done - interrupted code that's currently inspecting errno might see the wrong - value. diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 2c3099f76f..c9ce380f0f 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.c index 0646c5f859..46197d56f8 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -852,15 +852,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.c index 7c38f5fadf..eedc0980cf 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.c index 67693b0580..02814cd2c8 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.c index a066800a1c..df945a5ac4 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -2612,12 +2612,8 @@ InitProcessGlobals(void) static void handle_pm_pmsignal_signal(SIGNAL_ARGS) { - int save_errno = errno; - pending_pm_pmsignal = true; SetLatch(MyLatch); - - errno = save_errno; } /* @@ -2626,12 +2622,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; } /* @@ -2711,8 +2703,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: @@ -2729,8 +2719,6 @@ handle_pm_shutdown_request_signal(SIGNAL_ARGS) break; } SetLatch(MyLatch); - - errno = save_errno; } /* @@ -2890,12 +2878,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.c index d53c37d062..b6b53cd25f 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.c index d3b4fc2fe6..6db280e483 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.c index 4e54779a9e..e5477c1de1 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3476,12 +3476,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.c index 91ede1d0eb..6386995e6c 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.c index e84619e5a5..0f9f90d2c7 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.c index 1a34bd3715..01b5530f0b 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.c index aaaad8bb16..4055dd5f8d 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.c index 12f005818d..dcff9a8564 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.c index 92382b3c34..6ca2d4e20a 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.11.4.GIT