On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandboss...@gmail.com> writes:
>> My guess is that this has something to do with redefining SIG_ERR in
>> win32_port.h. We might be able to use push_macro/pop_macro to keep the old
>> value around, but at the moment I'm leaning towards just removing the
>> assertion in that path.
>
> I wonder why we redefine those values?
I wondered the same. Those redefines have been there since commit 5049196,
but I haven't been able to find any real discussion in the archives about
it. Maybe I will bug Magnus about it sometime, in case he happens to
remember the reason.
> But I tend to agree that just
> removing the test is sufficient for now. Given the lack of failure
> checks in the existing code, and the lack of trouble reports
> suggesting any problem, it's hard to muster enthusiasm for spending
> a lot of effort on this.
Assuming cfbot likes this new version of the patch, I'll commit it shortly.
Thanks for reviewing.
--
nathan
>From c7d182c41aa518f3b6479ee4bd6fdf6e8f2484c7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 15 Jan 2025 13:04:58 -0600
Subject: [PATCH v3 1/1] 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 an assertion for the return value of sigaction(). 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.
---
src/include/port.h | 2 +-
src/port/pqsignal.c | 39 ++++++++++-----------------------------
2 files changed, 11 insertions(+), 30 deletions(-)
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..9777b3a930 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,14 @@ 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;
+
+ /*
+ * Forward to Windows native signal system. Ideally, we'd check for
+ * SIG_ERR here, but win32_port.h redefines it for some reason.
+ */
+ (void) signal(signo, func);
#endif
}
--
2.39.5 (Apple Git-154)
