Cope with EINVAL and EIDRM shmat() failures in PGSharedMemoryAttach.

There's a very old race condition in our code to see whether a pre-existing
shared memory segment is still in use by a conflicting postmaster: it's
possible for the other postmaster to remove the segment in between our
shmctl() and shmat() calls.  It's a narrow window, and there's no risk
unless both postmasters are using the same port number, but that's possible
during parallelized "make check" tests.  (Note that while the TAP tests
take some pains to choose a randomized port number, pg_regress doesn't.)
If it does happen, we treated that as an unexpected case and errored out.

To fix, allow EINVAL to be treated as segment-not-present, and the same
for EIDRM on Linux.  AFAICS, the considerations here are basically
identical to the checks for acceptable shmctl() failures, so I documented
and coded it that way.

While at it, adjust PGSharedMemoryAttach's API to remove its undocumented
dependency on UsedShmemSegAddr in favor of passing the attach address
explicitly.  This makes it easier to be sure we're using a null shmaddr
when probing for segment conflicts (thus avoiding questions about what
EINVAL means).  I don't think there was a bug there, but it required
fragile assumptions about the state of UsedShmemSegAddr during
PGSharedMemoryIsInUse.

Commit c09850992 may have made this failure more probable by applying
the conflicting-segment tests more often.  Hence, back-patch to all
supported branches, as that was.

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

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index a9d7bf9a146fb82579f2c713941712e9c54e2c28..fda3442c5b5250da011c0badf5af02d4e0d92d8c 100644 (file)
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -103,6 +103,7 @@ static void *InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size);
 static void IpcMemoryDetach(int status, Datum shmaddr);
 static void IpcMemoryDelete(int status, Datum shmId);
 static IpcMemoryState PGSharedMemoryAttach(IpcMemoryId shmId,
+                                        void *attachAt,
                                         PGShmemHeader **addr);
 
 
@@ -310,7 +311,7 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
        PGShmemHeader *memAddress;
        IpcMemoryState state;
 
-       state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
+       state = PGSharedMemoryAttach((IpcMemoryId) id2, NULL, &memAddress);
        if (memAddress && shmdt(memAddress) < 0)
                elog(LOG, "shmdt(%p) failed: %m", memAddress);
        switch (state)
@@ -326,9 +327,17 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
        return true;
 }
 
-/* See comment at IpcMemoryState. */
+/*
+ * Test for a segment with id shmId; see comment at IpcMemoryState.
+ *
+ * If the segment exists, we'll attempt to attach to it, using attachAt
+ * if that's not NULL (but it's best to pass NULL if possible).
+ *
+ * *addr is set to the segment memory address if we attached to it, else NULL.
+ */
 static IpcMemoryState
 PGSharedMemoryAttach(IpcMemoryId shmId,
+                                        void *attachAt,
                                         PGShmemHeader **addr)
 {
        struct shmid_ds shmStat;
@@ -338,8 +347,7 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
        *addr = NULL;
 
        /*
-        * We detect whether a shared memory segment is in use by seeing whether
-        * it (a) exists and (b) has any processes attached to it.
+        * First, try to stat the shm segment ID, to see if it exists at all.
         */
        if (shmctl(shmId, IPC_STAT, &shmStat) < 0)
        {
@@ -372,9 +380,10 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
 #endif
 
                /*
-                * Otherwise, we had better assume that the segment is in use. The
-                * only likely case is EIDRM, which implies that the segment has been
-                * IPC_RMID'd but there are still processes attached to it.
+                * Otherwise, we had better assume that the segment is in use.  The
+                * only likely case is (non-Linux, assumed spec-compliant) EIDRM,
+                * which implies that the segment has been IPC_RMID'd but there are
+                * still processes attached to it.
                 */
                return SHMSTATE_ANALYSIS_FAILURE;
        }
@@ -382,24 +391,37 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
        /*
         * Try to attach to the segment and see if it matches our data directory.
         * This avoids shmid-conflict problems on machines that are running
-        * several postmasters under the same userid.
+        * several postmasters under the same userid and port number.  (That would
+        * not ordinarily happen in production, but it can happen during parallel
+        * testing.  Since our test setups don't open any TCP ports on Unix, such
+        * cases don't conflict otherwise.)
         */
        if (stat(DataDir, &statbuf) < 0)
                return SHMSTATE_ANALYSIS_FAILURE;       /* can't stat; be conservative */
 
-       /*
-        * Attachment fails if we have no write permission.  Since that will never
-        * happen with Postgres IPCProtection, such a failure shows the segment is
-        * not a Postgres segment.  If attachment fails for some other reason, be
-        * conservative.
-        */
-       hdr = (PGShmemHeader *) shmat(shmId, UsedShmemSegAddr, PG_SHMAT_FLAGS);
+       hdr = (PGShmemHeader *) shmat(shmId, attachAt, PG_SHMAT_FLAGS);
        if (hdr == (PGShmemHeader *) -1)
        {
+               /*
+                * Attachment failed.  The cases we're interested in are the same as
+                * for the shmctl() call above.  In particular, note that the owning
+                * postmaster could have terminated and removed the segment between
+                * shmctl() and shmat().
+                *
+                * If attachAt isn't NULL, it's possible that EINVAL reflects a
+                * problem with that address not a vanished segment, so it's best to
+                * pass NULL when probing for conflicting segments.
+                */
+               if (errno == EINVAL)
+                       return SHMSTATE_ENOENT; /* segment disappeared */
                if (errno == EACCES)
-                       return SHMSTATE_FOREIGN;
-               else
-                       return SHMSTATE_ANALYSIS_FAILURE;
+                       return SHMSTATE_FOREIGN;        /* must be non-Postgres */
+#ifdef HAVE_LINUX_EIDRM_BUG
+               if (errno == EIDRM)
+                       return SHMSTATE_ENOENT; /* segment disappeared */
+#endif
+               /* Otherwise, be conservative. */
+               return SHMSTATE_ANALYSIS_FAILURE;
        }
        *addr = hdr;
 
@@ -414,6 +436,11 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
                return SHMSTATE_FOREIGN;
        }
 
+       /*
+        * It does match our data directory, so now test whether any processes are
+        * still attached to it.  (We are, now, but the shm_nattch result is from
+        * before we attached to it.)
+        */
        return shmStat.shm_nattch == 0 ? SHMSTATE_UNATTACHED : SHMSTATE_ATTACHED;
 }
 
@@ -629,9 +656,6 @@ PGSharedMemoryCreate(Size size, int port,
        else
                sysvsize = size;
 
-       /* Make sure PGSharedMemoryAttach doesn't fail without need */
-       UsedShmemSegAddr = NULL;
-
        /*
         * Loop till we find a free IPC key.  Trust CreateDataDirLockFile() to
         * ensure no more than one postmaster per data directory can enter this
@@ -665,7 +689,7 @@ PGSharedMemoryCreate(Size size, int port,
                        state = SHMSTATE_FOREIGN;
                }
                else
-                       state = PGSharedMemoryAttach(shmid, &oldhdr);
+                       state = PGSharedMemoryAttach(shmid, NULL, &oldhdr);
 
                switch (state)
                {
@@ -791,7 +815,7 @@ PGSharedMemoryReAttach(void)
        if (shmid < 0)
                state = SHMSTATE_FOREIGN;
        else
-               state = PGSharedMemoryAttach(shmid, &hdr);
+               state = PGSharedMemoryAttach(shmid, UsedShmemSegAddr, &hdr);
        if (state != SHMSTATE_ATTACHED)
                elog(FATAL, "could not reattach to shared memory (key=%d, addr=%p): %m",
                         (int) UsedShmemSegID, UsedShmemSegAddr);