Date: Wed, 15 Jan 2025 17:47:51 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Fujii Masao <masao.fujii@oss.nttdata.com>,
	Andy Fan <zhihuifan1213@163.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: convert libpgport's pqsignal() to a void function
Message-ID: <Z4hJJwITseaKq_81@nathan>
References: <Z4chOKfnthRH71mw@nathan>
 <1164438.1736910166@sss.pgh.pa.us>
 <Z4cyRX-hqfXREYKC@nathan>
 <1171432.1736914085@sss.pgh.pa.us>
 <Z4gJTtG4ZX9vXl-y@nathan>
 <CA+hUKGL0NyB03uKrNqN0z1M=cwju_b_RJLgxtVPU4f0h8-8sZg@mail.gmail.com>
 <Z4gQxiLQrf5OLf18@nathan>
 <CA+hUKG+grkc4+e9EtegZcBn9oyTxUCEE9CFZvuPtvsXTuW=A=A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="EYHoee8f6wFuErPc"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKG+grkc4+e9EtegZcBn9oyTxUCEE9CFZvuPtvsXTuW=A=A@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z4hJJwITseaKq_81%40nathan>
Precedence: bulk


--EYHoee8f6wFuErPc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jan 16, 2025 at 11:07:41AM +1300, Thomas Munro wrote:
> +1 for your idea of not defining them at all outside the backend, it's
> just confusing noise.

I tried that, but these extra signals are needed even in the frontend for
pgkill(), etc.  My next thought was to simply ignore signal() errors for
the extra signals, but I don't think that's very nice because it just masks
broken code.  Finally, I settled on modifying initdb's setup_signals() to
use WIN32 checks instead of checking for the signals themselves.  This is
how it's done elsewhere, and this is apparently all that's needed to make
cfbot's Windows run happy.

I've also attached a 0002 that removes the redefinitions of SIG_ERR and
friends.  That looks good on cfbot, but we'll see what the buildfarm has to
say...

-- 
nathan

--EYHoee8f6wFuErPc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Convert-libpgport-s-pqsignal-to-a-void-function.patch"

From 15613d15ea907fa1aceee84d3a2e8b3708f95b4f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Jan 2025 17:30:23 -0600
Subject: [PATCH v4 1/2] Convert libpgport's pqsignal() to a void function.

The protections added by commit 3b00fdba9f introduced race
conditions to this function that can lead to bogus return values.
Since nobody seems to inspect the return value, this is of little
consequence, but it would have been nice to convert it to a void
function to avoid any possibility of a bogus return value.
Unfortunately, doing so would have required also modifying
legacy-pqsignal.c's version of the function, which would've
required an SONAME bump.  Or so I thought...

Thanks to commit 9a45a89c38, legacy-pqsignal.c now has its own
dedicated extern for pqsignal(), which decouples it enough that we
can follow through with changing libpgport's pqsignal() to a void
function.  This commit also adds a bit of error checking in the
form of assertions for the return value of sigaction()/signal().
Since a failure most likely indicates a coding error, and nobody
has ever bothered to check pqsignal()'s return value, it's probably
not worth doing anything fancier.

While at it, modify initdb's setup_signals() to check for WIN32
instead of the signals themselves.  win32_port.h defines many extra
signals, so the previous checks were insufficient to avoid calling
pqsignal() with invalid signals on Windows.  It'd be nice to avoid
defining these extra signals in the frontend altogether, but they
are used for pgkill(), etc., and we already use WIN32 checks for
invalid signals in various places.
---
 src/bin/initdb/initdb.c | 19 +++++--------------
 src/include/port.h      |  2 +-
 src/port/pqsignal.c     | 34 ++++++----------------------------
 3 files changed, 12 insertions(+), 43 deletions(-)

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 101c780012..ea4b66b3bf 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2874,27 +2874,18 @@ setup_text_search(void)
 void
 setup_signals(void)
 {
-	/* some of these are not valid on Windows */
-#ifdef SIGHUP
-	pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
 	pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
-	pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
 	pqsignal(SIGTERM, trapsig);
-#endif
+
+	/* the following are not valid on Windows */
+#ifndef WIN32
+	pqsignal(SIGHUP, trapsig);
+	pqsignal(SIGQUIT, trapsig);
 
 	/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
 	pqsignal(SIGPIPE, SIG_IGN);
-#endif
 
 	/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
 	pqsignal(SIGSYS, SIG_IGN);
 #endif
 }
diff --git a/src/include/port.h b/src/include/port.h
index f0e28ce5c5..4e9e565787 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -520,7 +520,7 @@ extern int	pg_mkdir_p(char *path, int omode);
 #define pqsignal pqsignal_be
 #endif
 typedef void (*pqsigfunc) (SIGNAL_ARGS);
-extern pqsigfunc pqsignal(int signo, pqsigfunc func);
+extern void pqsignal(int signo, pqsigfunc func);
 
 /* port/quotes.c */
 extern char *escape_single_quotes_ascii(const char *src);
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 1169de6b81..5dd8b76bae 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -112,31 +112,15 @@ wrapper_handler(SIGNAL_ARGS)
 /*
  * 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.
- *
  * Note: the actual name of this function is either pqsignal_fe when
  * compiled with -DFRONTEND, or pqsignal_be when compiled without that.
  * This is to avoid a name collision with libpq's legacy-pqsignal.c.
  */
-pqsigfunc
+void
 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;
+	struct sigaction act;
 #endif
 
 	Assert(signo < PG_NSIG);
@@ -155,17 +139,11 @@ pqsignal(int signo, pqsigfunc func)
 	if (signo == SIGCHLD)
 		act.sa_flags |= SA_NOCLDSTOP;
 #endif
-	if (sigaction(signo, &act, &oact) < 0)
-		return SIG_ERR;
-	else if (oact.sa_handler == wrapper_handler)
-		return orig_func;
-	else
-		return oact.sa_handler;
+	if (sigaction(signo, &act, NULL) < 0)
+		Assert(false);			/* probably indicates coding error */
 #else
 	/* Forward to Windows native signal system. */
-	if ((ret = signal(signo, func)) == wrapper_handler)
-		return orig_func;
-	else
-		return ret;
+	if (signal(signo, func) == SIG_ERR)
+		Assert(false);			/* probably indicates coding error */
 #endif
 }
-- 
2.39.5 (Apple Git-154)


--EYHoee8f6wFuErPc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0002-Remove-redeclarations-of-signal-functions-on-Wind.patch"

From e99d35bb7ee3370a5a8688be320edd1df3dd0cbc Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Jan 2025 17:36:09 -0600
Subject: [PATCH v4 2/2] Remove redeclarations of signal functions on Windows.

---
 src/include/port/win32_port.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 7900272e8d..ff7028bdc8 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -154,14 +154,6 @@
 
 #define sigmask(sig) ( 1 << ((sig)-1) )
 
-/* Signal function return values */
-#undef SIG_DFL
-#undef SIG_ERR
-#undef SIG_IGN
-#define SIG_DFL ((pqsigfunc)0)
-#define SIG_ERR ((pqsigfunc)-1)
-#define SIG_IGN ((pqsigfunc)1)
-
 /* Some extra signals */
 #define SIGHUP				1
 #define SIGQUIT				3
-- 
2.39.5 (Apple Git-154)


--EYHoee8f6wFuErPc--