Fix race condition with unprotected use of a latch pointer variable.

Commit 597a87ccc introduced a latch pointer variable to replace use
of a long-lived shared latch in the shared WalRcvData structure.
This was not well thought out, because there are now hazards of the
pointer variable changing while it's being inspected by another
process.  This could obviously lead to a core dump in code like

	if (WalRcv->latch)
		SetLatch(WalRcv->latch);

and there's a more remote risk of a torn read, if we have any
platforms where reading/writing a pointer is not atomic.

An actual problem would occur only if the walreceiver process
exits (gracefully) while the startup process is trying to
signal it, but that seems well within the realm of possibility.

To fix, treat the pointer variable (not the referenced latch)
as being protected by the WalRcv->mutex spinlock.  There
remains a race condition that we could apply SetLatch to a
process latch that no longer belongs to the walreceiver, but
I believe that's harmless: at worst it'd cause an extra wakeup
of the next process to use that PGPROC structure.

Back-patch to v10 where the faulty code was added.

Discussion: https://postgr.es/m/22735.1507048202@sss.pgh.pa.us

diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ec59d6e5621b8ec6ca7e7674fb5425ac878aa337..c85ffb0908b1b789746bc7e9bd4e768980911076 100644 (file)
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -256,13 +256,14 @@ WalReceiverMain(void)
    walrcv->lastMsgSendTime =
        walrcv->lastMsgReceiptTime = walrcv->latestWalEndTime = now;
 
+   /* Report the latch to use to awaken this process */
+   walrcv->latch = &MyProc->procLatch;
+
    SpinLockRelease(&walrcv->mutex);
 
    /* Arrange to clean up at walreceiver exit */
    on_shmem_exit(WalRcvDie, 0);
 
-   walrcv->latch = &MyProc->procLatch;
-
    /* Properly accept or ignore signals the postmaster might send us */
    pqsignal(SIGHUP, WalRcvSigHupHandler);  /* set flag to read config file */
    pqsignal(SIGINT, SIG_IGN);
@@ -777,8 +778,7 @@ WalRcvDie(int code, Datum arg)
    /* Ensure that all WAL records received are flushed to disk */
    XLogWalRcvFlush(true);
 
-   walrcv->latch = NULL;
-
+   /* Mark ourselves inactive in shared memory */
    SpinLockAcquire(&walrcv->mutex);
    Assert(walrcv->walRcvState == WALRCV_STREAMING ||
           walrcv->walRcvState == WALRCV_RESTARTING ||
@@ -789,6 +789,7 @@ WalRcvDie(int code, Datum arg)
    walrcv->walRcvState = WALRCV_STOPPED;
    walrcv->pid = 0;
    walrcv->ready_to_display = false;
+   walrcv->latch = NULL;
    SpinLockRelease(&walrcv->mutex);
 
    /* Terminate the connection gracefully. */
@@ -1344,9 +1345,15 @@ ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime)
 void
 WalRcvForceReply(void)
 {
+   Latch      *latch;
+
    WalRcv->force_reply = true;
-   if (WalRcv->latch)
-       SetLatch(WalRcv->latch);
+   /* fetching the latch pointer might not be atomic, so use spinlock */
+   SpinLockAcquire(&WalRcv->mutex);
+   latch = WalRcv->latch;
+   SpinLockRelease(&WalRcv->mutex);
+   if (latch)
+       SetLatch(latch);
 }
 
 /*

diff --git a/src/backend/replication/walreceiverfuncs.c b/src/backend/replication/walreceiverfuncs.c
index 8ed7254b5c60af486ffd3e1ca1ac58dbf45bab83..48e8498d620b20dbd115907b66bbfca25db714ee 100644 (file)
--- a/src/backend/replication/walreceiverfuncs.c
+++ b/src/backend/replication/walreceiverfuncs.c
@@ -226,6 +226,7 @@ RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr, const char *conninfo,
    WalRcvData *walrcv = WalRcv;
    bool        launch = false;
    pg_time_t   now = (pg_time_t) time(NULL);
+   Latch      *latch;
 
    /*
     * We always start at the beginning of the segment. That prevents a broken
@@ -274,12 +275,14 @@ RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr, const char *conninfo,
    walrcv->receiveStart = recptr;
    walrcv->receiveStartTLI = tli;
 
+   latch = walrcv->latch;
+
    SpinLockRelease(&walrcv->mutex);
 
    if (launch)
        SendPostmasterSignal(PMSIGNAL_START_WALRECEIVER);
-   else if (walrcv->latch)
-       SetLatch(walrcv->latch);
+   else if (latch)
+       SetLatch(latch);
 }
 
 /*

diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index 9a8b2e207ec3e2eb62cd909cb7eee3b2e90baae8..742ab6be0003dd8d3701a20d9b95a2d530cacc24 100644 (file)
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -121,9 +121,10 @@ typedef struct
 
    /*
     * force walreceiver reply?  This doesn't need to be locked; memory
-    * barriers for ordering are sufficient.
+    * barriers for ordering are sufficient.  But we do need atomic fetch and
+    * store semantics, so use sig_atomic_t.
     */
-   bool        force_reply;
+   sig_atomic_t force_reply;   /* used as a bool */
 
    /*
     * Latch used by startup process to wake up walreceiver after telling it