Postmaster self-deadlock due to PLT linkage resolution - Mailing list pgsql-hackers

Buildfarm member mamba (NetBSD-current on prairiedog's former hardware)
has failed repeatedly since I set it up.  I have now run the cause of
that to ground [1], and here's what's happening: if the postmaster
receives a signal just before it first waits at the select() in
ServerLoop, it can self-deadlock.  During the postmaster's first use of
select(), the dynamic loader needs to resolve the PLT branch table entry
that the core executable uses to reach select() in libc.so, and it locks
the loader's internal data structures while doing that.  If we enter
a signal handler while the lock is held, and the handler needs to do
anything that also requires the lock, the postmaster is frozen.

The probability of this happening seems remarkably small, since there's
only one narrow window per postmaster lifetime, and there's just not
that many potential signal causes active at that time either.
Nonetheless I have traces showing it happening (1) because we receive
SIGCHLD for startup process termination and (2) because we receive
SIGUSR1 from the startup process telling us to start walreceivers.
I guess that mamba's slow single-CPU hardware interacts with the
NetBSD scheduler in just the right way to make it more probable than
you'd think.  On typical modern machines, the postmaster would almost
certainly manage to wait before the startup process is able to signal
it.  Still, "almost certainly" is not "certainly".

The attached patch seems to fix the problem, by forcing resolution of
the PLT link before we unblock signals.  It depends on the assumption
that another select() call appearing within postmaster.c will share
the same PLT link, which seems pretty safe.

I'd originally intended to make this code "#ifdef __NetBSD__",
but on looking into the FreeBSD sources I find much the same locking
logic in their dynamic loader, and now I'm wondering if such behavior
isn't pretty standard.  The added calls should have negligible cost,
so it doesn't seem unreasonable to do them everywhere.

(Of course, a much better answer is to get out of the business of
doing nontrivial stuff in signal handlers.  But even if we get that
done soon, we'd surely not back-patch it.)

Thoughts?

            regards, tom lane

[1] https://gnats.netbsd.org/56979

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1664fcee2a..f9070da574 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1728,6 +1728,33 @@ ServerLoop(void)

     nSockets = initMasks(&readmask);

+    /*
+     * Issue a dummy select() call before we unblock signals.  The point of
+     * this is that select() may be reached via a PLT indirection, and that
+     * indirection will need to be resolved on first use, and on some
+     * platforms the dynamic loader's resolution code takes a lock.  If that
+     * lock is taken while signals are unblocked then we risk self-deadlock
+     * should a signal handler do anything that also needs the loader lock.
+     * This is a low-probability failure, since it can only happen if a signal
+     * arrives with just the right timing during the first pass through the
+     * server loop.  However, it has been seen repeatedly on slow single-CPU
+     * machines.
+     *
+     * Likewise, issue a dummy pg_usleep() to ensure we've resolved any PLT
+     * indirections needed in the PM_WAIT_DEAD_END path.  (The PG_SETMASK
+     * calls could also be at hazard, had we not issued one of those when
+     * blocking signals to begin with.)
+     */
+    {
+        struct timeval timeout0;
+
+        timeout0.tv_sec = 0;
+        timeout0.tv_usec = 0;
+        (void) select(0, NULL, NULL, NULL, &timeout0);
+        pg_usleep(0);
+    }
+
+    /* Main server loop begins here */
     for (;;)
     {
         fd_set        rmask;
@@ -1750,6 +1777,7 @@ ServerLoop(void)
         {
             PG_SETMASK(&UnBlockSig);

+            /* Add nothing else in this unblocked-signals section */
             pg_usleep(100000L); /* 100 msec seems reasonable */
             selres = 0;

@@ -1765,6 +1793,7 @@ ServerLoop(void)

             PG_SETMASK(&UnBlockSig);

+            /* Add nothing else in this unblocked-signals section */
             selres = select(nSockets, &rmask, NULL, NULL, &timeout);

             PG_SETMASK(&BlockSig);