Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 3b00fdb

Browse files
Check that MyProcPid == getpid() in backend signal handlers.
In commit 97550c0, we added a similar check to the SIGTERM handler for the startup process. This commit adds this check to backend signal handlers installed with pqsignal(). This is done by using a wrapper function that performs the check before calling the actual handler. The hope is that this will offer more general protection against child processes of Postgres backends inadvertently modifying shared memory due to inherited signal handlers. Another potential follow-up improvement is to use this wrapper handler function to restore errno instead of relying on each individual handler function to do so. This commit makes the changes in commit 97550c0 obsolete but leaves reverting it for a follow-up commit. Reviewed-by: Andres Freund, Noah Misch Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
1 parent 8d8afd4 commit 3b00fdb

File tree

1 file changed

+84
-2
lines changed

1 file changed

+84
-2
lines changed

src/port/pqsignal.c

+84-2
Original file line numberDiff line numberDiff line change
@@ -46,23 +46,99 @@
4646
#include "c.h"
4747

4848
#include <signal.h>
49+
#ifndef FRONTEND
50+
#include <unistd.h>
51+
#endif
4952

5053
#ifndef FRONTEND
5154
#include "libpq/pqsignal.h"
55+
#include "miscadmin.h"
56+
#endif
57+
58+
#ifdef PG_SIGNAL_COUNT /* Windows */
59+
#define PG_NSIG (PG_SIGNAL_COUNT)
60+
#elif defined(NSIG)
61+
#define PG_NSIG (NSIG)
62+
#else
63+
#define PG_NSIG (64) /* XXX: wild guess */
64+
#endif
65+
66+
/* Check a couple of common signals to make sure PG_NSIG is accurate. */
67+
StaticAssertDecl(SIGUSR2 < PG_NSIG, "SIGUSR2 >= PG_NSIG");
68+
StaticAssertDecl(SIGHUP < PG_NSIG, "SIGHUP >= PG_NSIG");
69+
StaticAssertDecl(SIGTERM < PG_NSIG, "SIGTERM >= PG_NSIG");
70+
StaticAssertDecl(SIGALRM < PG_NSIG, "SIGALRM >= PG_NSIG");
71+
72+
static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
73+
74+
/*
75+
* Except when called with SIG_IGN or SIG_DFL, pqsignal() sets up this function
76+
* as the handler for all signals. This wrapper handler function checks that
77+
* it is called within a process that the server knows about (i.e., any process
78+
* that has called InitProcessGlobals(), such as a client backend), and not a
79+
* child process forked by system(3), etc. This check ensures that such child
80+
* processes do not modify shared memory, which is often detrimental. If the
81+
* check succeeds, the function originally provided to pqsignal() is called.
82+
* Otherwise, the default signal handler is installed and then called.
83+
*/
84+
static void
85+
wrapper_handler(SIGNAL_ARGS)
86+
{
87+
#ifndef FRONTEND
88+
89+
/*
90+
* We expect processes to set MyProcPid before calling pqsignal() or
91+
* before accepting signals.
92+
*/
93+
Assert(MyProcPid);
94+
Assert(MyProcPid != PostmasterPid || !IsUnderPostmaster);
95+
96+
if (unlikely(MyProcPid != (int) getpid()))
97+
{
98+
pqsignal(postgres_signal_arg, SIG_DFL);
99+
raise(postgres_signal_arg);
100+
return;
101+
}
52102
#endif
53103

104+
(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
105+
}
106+
54107
/*
55108
* Set up a signal handler, with SA_RESTART, for signal "signo"
56109
*
57110
* Returns the previous handler.
111+
*
112+
* NB: If called within a signal handler, race conditions may lead to bogus
113+
* return values. You should either avoid calling this within signal handlers
114+
* or ignore the return value.
115+
*
116+
* XXX: Since no in-tree callers use the return value, and there is little
117+
* reason to do so, it would be nice if we could convert this to a void
118+
* function instead of providing potentially-bogus return values.
119+
* Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,
120+
* which in turn requires an SONAME bump, which is probably not worth it.
58121
*/
59122
pqsigfunc
60123
pqsignal(int signo, pqsigfunc func)
61124
{
125+
pqsigfunc orig_func = pqsignal_handlers[signo]; /* assumed atomic */
62126
#if !(defined(WIN32) && defined(FRONTEND))
63127
struct sigaction act,
64128
oact;
129+
#else
130+
pqsigfunc ret;
131+
#endif
65132

133+
Assert(signo < PG_NSIG);
134+
135+
if (func != SIG_IGN && func != SIG_DFL)
136+
{
137+
pqsignal_handlers[signo] = func; /* assumed atomic */
138+
func = wrapper_handler;
139+
}
140+
141+
#if !(defined(WIN32) && defined(FRONTEND))
66142
act.sa_handler = func;
67143
sigemptyset(&act.sa_mask);
68144
act.sa_flags = SA_RESTART;
@@ -72,9 +148,15 @@ pqsignal(int signo, pqsigfunc func)
72148
#endif
73149
if (sigaction(signo, &act, &oact) < 0)
74150
return SIG_ERR;
75-
return oact.sa_handler;
151+
else if (oact.sa_handler == wrapper_handler)
152+
return orig_func;
153+
else
154+
return oact.sa_handler;
76155
#else
77156
/* Forward to Windows native signal system. */
78-
return signal(signo, func);
157+
if ((ret = signal(signo, func)) == wrapper_handler)
158+
return orig_func;
159+
else
160+
return ret;
79161
#endif
80162
}

0 commit comments

Comments
 (0)