From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Andy Fan <zhihuifan1213@163.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: convert libpgport's pqsignal() to a void function
Date: Tue, 14 Jan 2025 20:45:12 -0600
Message-ID: <Z4chOKfnthRH71mw@nathan> (raw)
(moving to a new thread)
On Mon, Jan 13, 2025 at 10:04:31AM -0600, Nathan Bossart wrote:
> On Sat, Jan 11, 2025 at 02:04:13PM -0500, Tom Lane wrote:
>> BTW, this decouples legacy-pqsignal.c from pqsignal.c enough
>> that we could now do what's contemplated in the comments from
>> 3b00fdba9: simplify that version by making it return void,
>> or perhaps better just a true/false success report.
>> I've not touched that point here, though.
>
> I think it should just return void since AFAICT nobody checks the return
> value. But it would probably be a good idea to add some sort of error
> checking within the function. I've attached a draft patch that adds some
> new assertions. I originally tried to use elog() where it was available,
> but besides making the code even more unreadable, I think it's unnecessary
> (since AFAICT any problems are likely coding errors).
With commit 9a45a89 in place, this is now possible. I've attached a new
version of the patch with a more fleshed-out commit message and a small
comment fix.
For background, the protections added by commit 3b00fdb introduced race
conditions to pqsignal() that could lead to bogus return values. That's of
little consequence since nobody seems to look at the return values, 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, which didn't seem worth it. Or so I
thought...
Thanks to commit 9a45a89, legacy-pqsignal.c now has its own dedicated
extern for pqsignal(), which decouples it enough that we can follow through
with changing libpqport's pqsignal() to a void function.
Thoughts?
--
nathan
Attachments:v2-0001-Convert-libpgport-s-pqsignal-to-a-void-function.patch (text/plain, 3.8K) download
From 7b6e061be774d05d888bc5eac20e9aa09fc75403 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Jan 2025 20:34:10 -0600
Subject: [PATCH v2 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 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.
---
src/include/port.h | 2 +-
src/port/pqsignal.c | 34 ++++++----------------------------
2 files changed, 7 insertions(+), 29 deletions(-)
diff --git a/src/include/port.h b/src/include/port.hindex f0e28ce5c53..4e9e5657872 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.cindex 1169de6b81e..5dd8b76bae8 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)
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: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, masao.fujii@oss.nttdata.com, zhihuifan1213@163.com, pgsql-hackers@lists.postgresql.org
Subject: Re: convert libpgport's pqsignal() to a void function
In-Reply-To: <Z4chOKfnthRH71mw@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox