From 9f4022f16be5a482da95f608afe686803ebba0d4 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Mon, 8 Jun 2026 09:18:23 +0900 Subject: [PATCH v1] Do not use signal unsafe functions in pgpool main process signal handler. The pgpool main process SIGTERM/SIGINT/SIGQUIT handler did the full shutdown work inline: ereport() (twice), pool_semaphore_lock(MAIN_EXIT_HANDLER_SEM), terminate_all_childrens() with a blocking waitpid(-1, ..., 0) and more ereport() calls, kill() of the follow-child group, and finally exit(3) which runs atexit handlers and stdio flush. None of these are async-signal-safe (POSIX 2024 section 2.4.3 [1]). In particular, SIGTERM arriving while pgpool is mid-ereport() / mid-palloc() / mid-semop() could crash, hang, or corrupt heap state. Because the pgpool main process is the parent, a crash here takes down every session and the pgpool cluster. This commit restricts the handler to async-signal-safe calls only: capture the signal number into a new volatile sig_atomic_t main_exit_request, write one byte to the existing self-pipe to wake the main loop, restore errno, and return. The actual shutdown is performed synchronously by a new do_shutdown() function called from the pgpol main loop at the top of every iteration (via check_requests()) and also right after the inner pool_pause() returns, so a signal arriving during the 2-second select() sleep is acted on without an extra tick of latency. do_shutdown() carries the previous body verbatim - the non-async-safe calls (ereport, pool_semaphore_lock, terminate_all_childrens with its blocking waitpid, exit(3)) are now invoked from normal context where they are safe. [1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03 Reported-by: Emond Papegaaij Reported-by: Claude code Author: Tatsuo Ishii Discussion: Backpatch-through: v4.3 --- src/main/pgpool_main.c | 111 +++++++++++++++++++++++++++++++++++------ 1 file changed, 95 insertions(+), 16 deletions(-) diff --git a/src/main/pgpool_main.c b/src/main/pgpool_main.c index dbf1bfd14..5bb33e8de 100644 --- a/src/main/pgpool_main.c +++ b/src/main/pgpool_main.c @@ -171,6 +171,7 @@ static void kill_all_children(int sig); static pid_t fork_follow_child(int old_main_node, int new_primary, int old_primary); static int read_status_file(bool discard_status); static RETSIGTYPE exit_handler(int sig); +static void do_shutdown(int sig); static RETSIGTYPE reap_handler(int sig); static RETSIGTYPE sigusr1_handler(int sig); static void sigusr1_interrupt_processor(void); @@ -253,6 +254,12 @@ volatile sig_atomic_t reload_config_request = 0; static volatile sig_atomic_t sigusr1_request = 0; static volatile sig_atomic_t sigchld_request = 0; static volatile sig_atomic_t wakeup_request = 0; +static volatile sig_atomic_t main_exit_request = 0; /* set by exit_handler; + * carries the captured signal + * number (SIGTERM/INT/QUIT) + * so the main loop can + * perform the (non-async-safe) + * shutdown work itself. */ static int pipe_fds[2]; /* for delivering signals */ @@ -708,6 +715,13 @@ PgpoolMain(bool discard_status, bool clear_memcache_oidmaps) r = pool_pause(&t); POOL_SETMASK(&BlockSig); + /* + * If a SIGTERM/SIGINT/SIGQUIT was queued by exit_handler, + * service it immediately rather than waiting another tick. + */ + if (main_exit_request) + do_shutdown(main_exit_request); + if (pool_config->process_management == PM_DYNAMIC) service_child_processes(); @@ -1290,48 +1304,104 @@ terminate_all_childrens(int sig) /* - * Pgpool main process exit handler + * Pgpool main process exit handler. + * + * This handler runs in async-signal context: it MUST only call + * async-signal-safe primitives (POSIX 2024 2.4.3). ereport(), + * pool_semaphore_lock(), MemoryContext operations, blocking waitpid() + * and exit(3) (atexit chain + stdio flush) are all unsafe here. In a + * previous version this body did the full shutdown inline, which could + * deadlock or corrupt heap state when SIGTERM/SIGINT/SIGQUIT arrived + * while PT_MAIN was inside ereport(), palloc(), or another semop. + * + * The handler now only records the requested signal number into a + * sig_atomic_t flag and writes one byte to the self-pipe so the main + * loop's select() returns. The actual shutdown is performed + * synchronously by do_shutdown() from the main loop, where calling + * non-async-safe code is permitted. + * + * If the same signal is delivered to a forked child that has not yet + * reset this handler, fall through to proc_exit() unchanged. */ static RETSIGTYPE exit_handler(int sig) { - int *walk; int save_errno = errno; - ereport(LOG, - (errmsg("exit handler called (signal: %d)", sig))); - - POOL_SETMASK(&AuthBlockSig); - /* * this could happen in a child process if a signal has been sent before - * resetting signal handler + * resetting signal handler. proc_exit() ultimately reduces to _exit() + * for non-PT_MAIN processes; calling it from a handler in a child that + * hasn't yet rewired its own handlers is the historical behaviour. */ if (getpid() != mypid) { - POOL_SETMASK(&UnBlockSig); proc_exit(0); + errno = save_errno; + return; } if (sig != SIGTERM && sig != SIGINT && sig != SIGQUIT) { - POOL_SETMASK(&UnBlockSig); errno = save_errno; return; } /* - * Check if another exit handler instance is already running. It is - * possible that exit_handler is interrupted in the middle by other - * signal. + * Record the signal for the main loop. Repeated signals coalesce; the + * first one wins, which matches the previous "exiting" guard. + */ + if (main_exit_request == 0) + main_exit_request = sig; + + /* Wake up the main loop if the self-pipe is set up. */ + if (pipe_fds[1]) + { + /* + * write() is async-signal-safe. We deliberately ignore the + * return value: if the pipe is full the main loop will pick the + * flag up on its next select() wake-up anyway. + * + * We do not use "(void) write()" here because it warns "warning: + * ignoring return value of ˇwrite˘ declared with attribute + * ˇwarn_unused_result˘ [-Wunused-result] on gcc -O2 -Wall and some + * platforms enabling _FORTIFY_SOURCE. + */ + ssize_t w = write(pipe_fds[1], "\0", 1); + + (void) w; + } + + errno = save_errno; +} + +/* + * Synchronous shutdown body, invoked from the PT_MAIN main loop when + * main_exit_request has been set by exit_handler(). All the work that used + * to live inside the signal handler (ereport, pool_semaphore_lock, + * waitpid, kill of child group, exit(3)) lives here, in normal context + * where it is safe. + */ +static void +do_shutdown(int sig) +{ + int *walk; + + ereport(LOG, + (errmsg("exit handler called (signal: %d)", sig))); + + POOL_SETMASK(&AuthBlockSig); + + /* + * Check if another exit handler instance is already running. Since the + * shutdown now runs serially from the main loop this is mostly a + * belt-and-braces guard, but cheap and harmless to keep. */ if (exiting) { POOL_SETMASK(&UnBlockSig); - errno = save_errno; return; } - /* Check to make sure that other exit handler is not running */ pool_semaphore_lock(MAIN_EXIT_HANDLER_SEM); if (exiting == 0) { @@ -1344,7 +1414,6 @@ static RETSIGTYPE exit_handler(int sig) ereport(LOG, (errmsg("exit handler (signal: %d) called. but exit handler is already in progress", sig))); POOL_SETMASK(&UnBlockSig); - errno = save_errno; return; } @@ -5031,6 +5100,16 @@ check_requests(void) { sigset_t sig; + /* + * Shutdown request? exit_handler() set main_exit_request to the captured + * signal number; perform the actual (non-async-safe) shutdown here, in + * normal context. do_shutdown() does not return. + */ + if (main_exit_request) + { + do_shutdown(main_exit_request); + } + /* * Waking child request? */ -- 2.43.0