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

tglsfdc · tglsfdc · commit 803f90ab795b · 2019-05-10T14:56:41.000-04:00
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 c098509 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
@@ -104,6 +104,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);
 
 
@@ -311,7 +312,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)
@@ -327,9 +328,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;
@@ -339,8 +348,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)
 	{
@@ -373,34 +381,48 @@ 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;
 	}
 
 	/*
 	 * 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;
 
@@ -415,6 +437,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;
 }
 
@@ -633,9 +660,6 @@ PGSharedMemoryCreate(Size size, int port,
 	sysvsize = size;
 #endif
 
-	/* 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
@@ -669,7 +693,7 @@ PGSharedMemoryCreate(Size size, int port,
 			state = SHMSTATE_FOREIGN;
 		}
 		else
-			state = PGSharedMemoryAttach(shmid, &oldhdr);
+			state = PGSharedMemoryAttach(shmid, NULL, &oldhdr);
 
 		switch (state)
 		{
@@ -799,7 +823,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);
