public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: Fix signal handler in pgpool main
Date: Mon, 08 Jun 2026 10:33:12 +0900 (JST)
Message-ID: <[email protected]> (raw)
I got a bug report regarding pgpool main process from Emond Papegaaij:
its exit/reload signal handler uses non signal safe functions, and it
could lead to a crash of the process.
Attached is the patch from him generated by Claude Code, with slight
modifications by me.
Basically it changes the signal 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.
I think this patch is important and worth to apply to all supported
branches.
However, since it touches one of the most critical part of pgpool, I
would like to apply to master branch first and see how our build-farm
acts before back patching (it will take a few days).
Comments?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[text/x-patch] v1-0001-Do-not-use-signal-unsafe-functions-in-pgpool-main.patch (8.1K, ../[email protected]/2-v1-0001-Do-not-use-signal-unsafe-functions-in-pgpool-main.patch)
download | inline diff:
From 9f4022f16be5a482da95f608afe686803ebba0d4 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
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 <[email protected]>
Reported-by: Claude code
Author: Tatsuo Ishii <[email protected]>
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
view thread (3+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Fix signal handler in pgpool main
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox