Use _exit(2) for SIGQUIT during ProcessStartupPacket, too.
Bring the signal handling for startup-packet collection into line with the policy established in commitsbedadc732
and8e19a8264
, namely don't risk running atexit callbacks when handling SIGQUIT. Ideally, we'd not do so for SIGTERM or timeout interrupts either, but that change seems a bit too risky for the back branches. For now, just improve the comments in this area to describe the risk. Also relocate where BackendInitialize re-disables these interrupts, to minimize the code span where they're active. This doesn't buy a whole lot of safety, but it can't hurt. In passing, rename startup_die() to remove confusion about whether it is for the startup process. Like the previous commits, back-patch to all supported branches. Discussion: https://postgr.es/m/1850884.1599601164@sss.pgh.pa.us
This commit is contained in:
parent
34a947ca13
commit
58c6feccfa
|
@ -112,6 +112,7 @@
|
|||
#include "postmaster/autovacuum.h"
|
||||
#include "postmaster/bgworker_internals.h"
|
||||
#include "postmaster/fork_process.h"
|
||||
#include "postmaster/interrupt.h"
|
||||
#include "postmaster/pgarch.h"
|
||||
#include "postmaster/postmaster.h"
|
||||
#include "postmaster/syslogger.h"
|
||||
|
@ -405,7 +406,7 @@ static void SIGHUP_handler(SIGNAL_ARGS);
|
|||
static void pmdie(SIGNAL_ARGS);
|
||||
static void reaper(SIGNAL_ARGS);
|
||||
static void sigusr1_handler(SIGNAL_ARGS);
|
||||
static void startup_die(SIGNAL_ARGS);
|
||||
static void process_startup_packet_die(SIGNAL_ARGS);
|
||||
static void dummy_handler(SIGNAL_ARGS);
|
||||
static void StartupPacketTimeoutHandler(void);
|
||||
static void CleanupBackend(int pid, int exitstatus);
|
||||
|
@ -4340,22 +4341,30 @@ BackendInitialize(Port *port)
|
|||
whereToSendOutput = DestRemote; /* now safe to ereport to client */
|
||||
|
||||
/*
|
||||
* We arrange for a simple exit(1) if we receive SIGTERM or SIGQUIT or
|
||||
* timeout while trying to collect the startup packet. Otherwise the
|
||||
* postmaster cannot shutdown the database FAST or IMMED cleanly if a
|
||||
* buggy client fails to send the packet promptly. XXX it follows that
|
||||
* the remainder of this function must tolerate losing control at any
|
||||
* instant. Likewise, any pg_on_exit_callback registered before or during
|
||||
* this function must be prepared to execute at any instant between here
|
||||
* and the end of this function. Furthermore, affected callbacks execute
|
||||
* partially or not at all when a second exit-inducing signal arrives
|
||||
* after proc_exit_prepare() decrements on_proc_exit_index. (Thanks to
|
||||
* that mechanic, callbacks need not anticipate more than one call.) This
|
||||
* is fragile; it ought to instead follow the norm of handling interrupts
|
||||
* at selected, safe opportunities.
|
||||
* We arrange to do proc_exit(1) if we receive SIGTERM or timeout while
|
||||
* trying to collect the startup packet; while SIGQUIT results in
|
||||
* _exit(2). Otherwise the postmaster cannot shutdown the database FAST
|
||||
* or IMMED cleanly if a buggy client fails to send the packet promptly.
|
||||
*
|
||||
* XXX this is pretty dangerous; signal handlers should not call anything
|
||||
* as complex as proc_exit() directly. We minimize the hazard by not
|
||||
* keeping these handlers active for longer than we must. However, it
|
||||
* seems necessary to be able to escape out of DNS lookups as well as the
|
||||
* startup packet reception proper, so we can't narrow the scope further
|
||||
* than is done here.
|
||||
*
|
||||
* XXX it follows that the remainder of this function must tolerate losing
|
||||
* control at any instant. Likewise, any pg_on_exit_callback registered
|
||||
* before or during this function must be prepared to execute at any
|
||||
* instant between here and the end of this function. Furthermore,
|
||||
* affected callbacks execute partially or not at all when a second
|
||||
* exit-inducing signal arrives after proc_exit_prepare() decrements
|
||||
* on_proc_exit_index. (Thanks to that mechanic, callbacks need not
|
||||
* anticipate more than one call.) This is fragile; it ought to instead
|
||||
* follow the norm of handling interrupts at selected, safe opportunities.
|
||||
*/
|
||||
pqsignal(SIGTERM, startup_die);
|
||||
pqsignal(SIGQUIT, startup_die);
|
||||
pqsignal(SIGTERM, process_startup_packet_die);
|
||||
pqsignal(SIGQUIT, SignalHandlerForCrashExit);
|
||||
InitializeTimeouts(); /* establishes SIGALRM handler */
|
||||
PG_SETMASK(&StartupBlockSig);
|
||||
|
||||
|
@ -4411,8 +4420,8 @@ BackendInitialize(Port *port)
|
|||
port->remote_hostname = strdup(remote_host);
|
||||
|
||||
/*
|
||||
* Ready to begin client interaction. We will give up and exit(1) after a
|
||||
* time delay, so that a broken client can't hog a connection
|
||||
* Ready to begin client interaction. We will give up and proc_exit(1)
|
||||
* after a time delay, so that a broken client can't hog a connection
|
||||
* indefinitely. PreAuthDelay and any DNS interactions above don't count
|
||||
* against the time limit.
|
||||
*
|
||||
|
@ -4434,6 +4443,12 @@ BackendInitialize(Port *port)
|
|||
*/
|
||||
status = ProcessStartupPacket(port, false, false);
|
||||
|
||||
/*
|
||||
* Disable the timeout, and prevent SIGTERM/SIGQUIT again.
|
||||
*/
|
||||
disable_timeout(STARTUP_PACKET_TIMEOUT, false);
|
||||
PG_SETMASK(&BlockSig);
|
||||
|
||||
/*
|
||||
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
|
||||
* already did any appropriate error reporting.
|
||||
|
@ -4459,12 +4474,6 @@ BackendInitialize(Port *port)
|
|||
pfree(ps_data.data);
|
||||
|
||||
set_ps_display("initializing");
|
||||
|
||||
/*
|
||||
* Disable the timeout, and prevent SIGTERM/SIGQUIT again.
|
||||
*/
|
||||
disable_timeout(STARTUP_PACKET_TIMEOUT, false);
|
||||
PG_SETMASK(&BlockSig);
|
||||
}
|
||||
|
||||
|
||||
|
@ -5359,16 +5368,22 @@ sigusr1_handler(SIGNAL_ARGS)
|
|||
}
|
||||
|
||||
/*
|
||||
* SIGTERM or SIGQUIT while processing startup packet.
|
||||
* SIGTERM while processing startup packet.
|
||||
* Clean up and exit(1).
|
||||
*
|
||||
* XXX: possible future improvement: try to send a message indicating
|
||||
* why we are disconnecting. Problem is to be sure we don't block while
|
||||
* doing so, nor mess up SSL initialization. In practice, if the client
|
||||
* has wedged here, it probably couldn't do anything with the message anyway.
|
||||
* Running proc_exit() from a signal handler is pretty unsafe, since we
|
||||
* can't know what code we've interrupted. But the alternative of using
|
||||
* _exit(2) is also unpalatable, since it'd mean that a "fast shutdown"
|
||||
* would cause a database crash cycle (forcing WAL replay at restart)
|
||||
* if any sessions are in authentication. So we live with it for now.
|
||||
*
|
||||
* One might be tempted to try to send a message indicating why we are
|
||||
* disconnecting. However, that would make this even more unsafe. Also,
|
||||
* it seems undesirable to provide clues about the database's state to
|
||||
* a client that has not yet completed authentication.
|
||||
*/
|
||||
static void
|
||||
startup_die(SIGNAL_ARGS)
|
||||
process_startup_packet_die(SIGNAL_ARGS)
|
||||
{
|
||||
proc_exit(1);
|
||||
}
|
||||
|
@ -5389,7 +5404,11 @@ dummy_handler(SIGNAL_ARGS)
|
|||
|
||||
/*
|
||||
* Timeout while processing startup packet.
|
||||
* As for startup_die(), we clean up and exit(1).
|
||||
* As for process_startup_packet_die(), we clean up and exit(1).
|
||||
*
|
||||
* This is theoretically just as hazardous as in process_startup_packet_die(),
|
||||
* although in practice we're almost certainly waiting for client input,
|
||||
* which greatly reduces the risk.
|
||||
*/
|
||||
static void
|
||||
StartupPacketTimeoutHandler(void)
|
||||
|
|
Loading…
Reference in New Issue