Skip to content

Commit 0777696

Browse files
nmischnmisch
committed
Revert "Consistently test for in-use shared memory."
This reverts commits 2f932f7, 16ee6ea and 6f0e190. The buildfarm has revealed several bugs. Back-patch like the original commits. Discussion: https://postgr.es/m/20190404145319.GA1720877@rfd.leadboat.com
1 parent 4154783 commit 0777696

File tree

7 files changed

+138
-166
lines changed

7 files changed

+138
-166
lines changed

‎src/backend/port/sysv_shmem.c

Lines changed: 110 additions & 147 deletions
Original file line numberDiff line numberDiff line change
@@ -70,26 +70,6 @@
7070
typedef key_t IpcMemoryKey; /* shared memory key passed to shmget(2) */
7171
typedef int IpcMemoryId; /* shared memory ID returned by shmget(2) */
7272

73-
/*
74-
* How does a given IpcMemoryId relate to this PostgreSQL process?
75-
*
76-
* One could recycle unattached segments of different data directories if we
77-
* distinguished that case from other SHMSTATE_FOREIGN cases. Doing so would
78-
* cause us to visit less of the key space, making us less likely to detect a
79-
* SHMSTATE_ATTACHED key. It would also complicate the concurrency analysis,
80-
* in that postmasters of different data directories could simultaneously
81-
* attempt to recycle a given key. We'll waste keys longer in some cases, but
82-
* avoiding the problems of the alternative justifies that loss.
83-
*/
84-
typedef enum
85-
{
86-
SHMSTATE_ANALYSIS_FAILURE, /* unexpected failure to analyze the ID */
87-
SHMSTATE_ATTACHED, /* pertinent to DataDir, has attached PIDs */
88-
SHMSTATE_ENOENT, /* no segment of that ID */
89-
SHMSTATE_FOREIGN, /* exists, but not pertinent to DataDir */
90-
SHMSTATE_UNATTACHED /* pertinent to DataDir, no attached PIDs */
91-
} IpcMemoryState;
92-
9373

9474
unsigned long UsedShmemSegID = 0;
9575
void *UsedShmemSegAddr = NULL;
@@ -102,8 +82,8 @@ static void *AnonymousShmem = NULL;
10282
static void *InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size);
10383
static void IpcMemoryDetach(int status, Datum shmaddr);
10484
static void IpcMemoryDelete(int status, Datum shmId);
105-
static IpcMemoryState PGSharedMemoryAttach(IpcMemoryId shmId,
106-
PGShmemHeader **addr);
85+
static PGShmemHeader *PGSharedMemoryAttach(IpcMemoryKey key,
86+
IpcMemoryId *shmid);
10787

10888

10989
/*
@@ -307,36 +287,11 @@ IpcMemoryDelete(int status, Datum shmId)
307287
bool
308288
PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
309289
{
310-
PGShmemHeader *memAddress;
311-
IpcMemoryState state;
312-
313-
state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
314-
if (memAddress && shmdt(memAddress) < 0)
315-
elog(LOG, "shmdt(%p) failed: %m", memAddress);
316-
switch (state)
317-
{
318-
case SHMSTATE_ENOENT:
319-
case SHMSTATE_FOREIGN:
320-
case SHMSTATE_UNATTACHED:
321-
return false;
322-
case SHMSTATE_ANALYSIS_FAILURE:
323-
case SHMSTATE_ATTACHED:
324-
return true;
325-
}
326-
return true;
327-
}
328-
329-
/* See comment at IpcMemoryState. */
330-
static IpcMemoryState
331-
PGSharedMemoryAttach(IpcMemoryId shmId,
332-
PGShmemHeader **addr)
333-
{
290+
IpcMemoryId shmId = (IpcMemoryId) id2;
334291
struct shmid_ds shmStat;
335292
struct stat statbuf;
336293
PGShmemHeader *hdr;
337294

338-
*addr = NULL;
339-
340295
/*
341296
* We detect whether a shared memory segment is in use by seeing whether
342297
* it (a) exists and (b) has any processes attached to it.
@@ -349,15 +304,15 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
349304
* exists.
350305
*/
351306
if (errno == EINVAL)
352-
return SHMSTATE_ENOENT;
307+
return false;
353308

354309
/*
355310
* EACCES implies that the segment belongs to some other userid, which
356311
* means it is not a Postgres shmem segment (or at least, not one that
357312
* is relevant to our data directory).
358313
*/
359314
if (errno == EACCES)
360-
return SHMSTATE_FOREIGN;
315+
return false;
361316

362317
/*
363318
* Some Linux kernel versions (in fact, all of them as of July 2007)
@@ -368,47 +323,50 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
368323
*/
369324
#ifdef HAVE_LINUX_EIDRM_BUG
370325
if (errno == EIDRM)
371-
return SHMSTATE_ENOENT;
326+
return false;
372327
#endif
373328

374329
/*
375330
* Otherwise, we had better assume that the segment is in use. The
376331
* only likely case is EIDRM, which implies that the segment has been
377332
* IPC_RMID'd but there are still processes attached to it.
378333
*/
379-
return SHMSTATE_ANALYSIS_FAILURE;
334+
return true;
380335
}
381336

337+
/* If it has no attached processes, it's not in use */
338+
if (shmStat.shm_nattch == 0)
339+
return false;
340+
382341
/*
383342
* Try to attach to the segment and see if it matches our data directory.
384343
* This avoids shmid-conflict problems on machines that are running
385344
* several postmasters under the same userid.
386345
*/
387346
if (stat(DataDir, &statbuf) < 0)
388-
return SHMSTATE_ANALYSIS_FAILURE; /* can't stat; be conservative */
347+
return true; /* if can't stat, be conservative */
348+
349+
hdr = (PGShmemHeader *) shmat(shmId, NULL, PG_SHMAT_FLAGS);
389350

390-
/*
391-
* If we can't attach, be conservative. This may fail if postmaster.pid
392-
* furnished the shmId and another user created a world-readable segment
393-
* of the same shmId.
394-
*/
395-
hdr = (PGShmemHeader *) shmat(shmId, UsedShmemSegAddr, PG_SHMAT_FLAGS);
396351
if (hdr == (PGShmemHeader *) -1)
397-
return SHMSTATE_ANALYSIS_FAILURE;
398-
*addr = hdr;
352+
return true; /* if can't attach, be conservative */
399353

400354
if (hdr->magic != PGShmemMagic ||
401355
hdr->device != statbuf.st_dev ||
402356
hdr->inode != statbuf.st_ino)
403357
{
404358
/*
405359
* It's either not a Postgres segment, or not one for my data
406-
* directory.
360+
* directory. In either case it poses no threat.
407361
*/
408-
return SHMSTATE_FOREIGN;
362+
shmdt((void *) hdr);
363+
return false;
409364
}
410365

411-
return shmStat.shm_nattch == 0 ? SHMSTATE_UNATTACHED : SHMSTATE_ATTACHED;
366+
/* Trouble --- looks a lot like there's still live backends */
367+
shmdt((void *) hdr);
368+
369+
return true;
412370
}
413371

414372
#ifdef USE_ANONYMOUS_SHMEM
@@ -584,21 +542,25 @@ AnonymousShmemDetach(int status, Datum arg)
584542
* standard header. Also, register an on_shmem_exit callback to release
585543
* the storage.
586544
*
587-
* Dead Postgres segments pertinent to this DataDir are recycled if found, but
588-
* we do not fail upon collision with foreign shmem segments. The idea here
589-
* is to detect and re-use keys that may have been assigned by a crashed
590-
* postmaster or backend.
545+
* Dead Postgres segments are recycled if found, but we do not fail upon
546+
* collision with non-Postgres shmem segments. The idea here is to detect and
547+
* re-use keys that may have been assigned by a crashed postmaster or backend.
548+
*
549+
* makePrivate means to always create a new segment, rather than attach to
550+
* or recycle any existing segment.
591551
*
592552
* The port number is passed for possible use as a key (for SysV, we use
593-
* it to generate the starting shmem key).
553+
* it to generate the starting shmem key). In a standalone backend,
554+
* zero will be passed.
594555
*/
595556
PGShmemHeader *
596-
PGSharedMemoryCreate(Size size, int port,
557+
PGSharedMemoryCreate(Size size, bool makePrivate, int port,
597558
PGShmemHeader **shim)
598559
{
599560
IpcMemoryKey NextShmemSegID;
600561
void *memAddress;
601562
PGShmemHeader *hdr;
563+
IpcMemoryId shmid;
602564
struct stat statbuf;
603565
Size sysvsize;
604566

@@ -629,92 +591,70 @@ PGSharedMemoryCreate(Size size, int port,
629591
/* Make sure PGSharedMemoryAttach doesn't fail without need */
630592
UsedShmemSegAddr = NULL;
631593

632-
/*
633-
* Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
634-
* ensure no more than one postmaster per data directory can enter this
635-
* loop simultaneously. (CreateDataDirLockFile() does not ensure that,
636-
* but prefer fixing it over coping here.)
637-
*/
638-
NextShmemSegID = 1 + port * 1000;
594+
/* Loop till we find a free IPC key */
595+
NextShmemSegID = port * 1000;
639596

640-
for (;;)
597+
for (NextShmemSegID++;; NextShmemSegID++)
641598
{
642-
IpcMemoryId shmid;
643-
PGShmemHeader *oldhdr;
644-
IpcMemoryState state;
645-
646599
/* Try to create new segment */
647600
memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
648601
if (memAddress)
649602
break; /* successful create and attach */
650603

651604
/* Check shared memory and possibly remove and recreate */
652605

606+
if (makePrivate) /* a standalone backend shouldn't do this */
607+
continue;
608+
609+
if ((memAddress = PGSharedMemoryAttach(NextShmemSegID, &shmid)) == NULL)
610+
continue; /* can't attach, not one of mine */
611+
653612
/*
654-
* shmget() failure is typically EACCES, hence SHMSTATE_FOREIGN.
655-
* ENOENT, a narrow possibility, implies SHMSTATE_ENOENT, but one can
656-
* safely treat SHMSTATE_ENOENT like SHMSTATE_FOREIGN.
613+
* If I am not the creator and it belongs to an extant process,
614+
* continue.
657615
*/
658-
shmid = shmget(NextShmemSegID, sizeof(PGShmemHeader), 0);
659-
if (shmid < 0)
616+
hdr = (PGShmemHeader *) memAddress;
617+
if (hdr->creatorPID != getpid())
660618
{
661-
oldhdr = NULL;
662-
state = SHMSTATE_FOREIGN;
619+
if (kill(hdr->creatorPID, 0) == 0 || errno != ESRCH)
620+
{
621+
shmdt(memAddress);
622+
continue; /* segment belongs to a live process */
623+
}
663624
}
664-
else
665-
state = PGSharedMemoryAttach(shmid, &oldhdr);
666-
667-
switch (state)
668-
{
669-
case SHMSTATE_ANALYSIS_FAILURE:
670-
case SHMSTATE_ATTACHED:
671-
ereport(FATAL,
672-
(errcode(ERRCODE_LOCK_FILE_EXISTS),
673-
errmsg("pre-existing shared memory block (key %lu, ID %lu) is still in use",
674-
(unsigned long) NextShmemSegID,
675-
(unsigned long) shmid),
676-
errhint("Terminate any old server processes associated with data directory \"%s\".",
677-
DataDir)));
678-
break;
679-
case SHMSTATE_ENOENT:
680625

681-
/*
682-
* To our surprise, some other process deleted since our last
683-
* InternalIpcMemoryCreate(). Moments earlier, we would have
684-
* seen SHMSTATE_FOREIGN. Try that same ID again.
685-
*/
686-
elog(LOG,
687-
"shared memory block (key %lu, ID %lu) deleted during startup",
688-
(unsigned long) NextShmemSegID,
689-
(unsigned long) shmid);
690-
break;
691-
case SHMSTATE_FOREIGN:
692-
NextShmemSegID++;
693-
break;
694-
case SHMSTATE_UNATTACHED:
626+
/*
627+
* The segment appears to be from a dead Postgres process, or from a
628+
* previous cycle of life in this same process. Zap it, if possible,
629+
* and any associated dynamic shared memory segments, as well. This
630+
* probably shouldn't fail, but if it does, assume the segment belongs
631+
* to someone else after all, and continue quietly.
632+
*/
633+
if (hdr->dsm_control != 0)
634+
dsm_cleanup_using_control_segment(hdr->dsm_control);
635+
shmdt(memAddress);
636+
if (shmctl(shmid, IPC_RMID, NULL) < 0)
637+
continue;
695638

696-
/*
697-
* The segment pertains to DataDir, and every process that had
698-
* used it has died or detached. Zap it, if possible, and any
699-
* associated dynamic shared memory segments, as well. This
700-
* shouldn't fail, but if it does, assume the segment belongs
701-
* to someone else after all, and try the next candidate.
702-
* Otherwise, try again to create the segment. That may fail
703-
* if some other process creates the same shmem key before we
704-
* do, in which case we'll try the next key.
705-
*/
706-
if (oldhdr->dsm_control != 0)
707-
dsm_cleanup_using_control_segment(oldhdr->dsm_control);
708-
if (shmctl(shmid, IPC_RMID, NULL) < 0)
709-
NextShmemSegID++;
710-
break;
711-
}
639+
/*
640+
* Now try again to create the segment.
641+
*/
642+
memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
643+
if (memAddress)
644+
break; /* successful create and attach */
712645

713-
if (oldhdr && shmdt(oldhdr) < 0)
714-
elog(LOG, "shmdt(%p) failed: %m", oldhdr);
646+
/*
647+
* Can only get here if some other process managed to create the same
648+
* shmem key before we did. Let him have that one, loop around to try
649+
* next key.
650+
*/
715651
}
716652

717-
/* Initialize new segment. */
653+
/*
654+
* OK, we created a new segment. Mark it as created by this process. The
655+
* order of assignments here is critical so that another Postgres process
656+
* can't see the header as valid but belonging to an invalid PID!
657+
*/
718658
hdr = (PGShmemHeader *) memAddress;
719659
hdr->creatorPID = getpid();
720660
hdr->magic = PGShmemMagic;
@@ -774,8 +714,7 @@ void
774714
PGSharedMemoryReAttach(void)
775715
{
776716
IpcMemoryId shmid;
777-
PGShmemHeader *hdr;
778-
IpcMemoryState state;
717+
void *hdr;
779718
void *origUsedShmemSegAddr = UsedShmemSegAddr;
780719

781720
Assert(UsedShmemSegAddr != NULL);
@@ -788,18 +727,14 @@ PGSharedMemoryReAttach(void)
788727
#endif
789728

790729
elog(DEBUG3, "attaching to %p", UsedShmemSegAddr);
791-
shmid = shmget(UsedShmemSegID, sizeof(PGShmemHeader), 0);
792-
if (shmid < 0)
793-
state = SHMSTATE_FOREIGN;
794-
else
795-
state = PGSharedMemoryAttach(shmid, &hdr);
796-
if (state != SHMSTATE_ATTACHED)
730+
hdr = (void *) PGSharedMemoryAttach((IpcMemoryKey) UsedShmemSegID, &shmid);
731+
if (hdr == NULL)
797732
elog(FATAL, "could not reattach to shared memory (key=%d, addr=%p): %m",
798733
(int) UsedShmemSegID, UsedShmemSegAddr);
799734
if (hdr != origUsedShmemSegAddr)
800735
elog(FATAL, "reattaching to shared memory returned unexpected address (got %p, expected %p)",
801736
hdr, origUsedShmemSegAddr);
802-
dsm_set_control_handle(hdr->dsm_control);
737+
dsm_set_control_handle(((PGShmemHeader *) hdr)->dsm_control);
803738

804739
UsedShmemSegAddr = hdr; /* probably redundant */
805740
}
@@ -875,3 +810,31 @@ PGSharedMemoryDetach(void)
875810
}
876811
#endif
877812
}
813+
814+
815+
/*
816+
* Attach to shared memory and make sure it has a Postgres header
817+
*
818+
* Returns attach address if OK, else NULL
819+
*/
820+
static PGShmemHeader *
821+
PGSharedMemoryAttach(IpcMemoryKey key, IpcMemoryId *shmid)
822+
{
823+
PGShmemHeader *hdr;
824+
825+
if ((*shmid = shmget(key, sizeof(PGShmemHeader), 0)) < 0)
826+
return NULL;
827+
828+
hdr = (PGShmemHeader *) shmat(*shmid, UsedShmemSegAddr, PG_SHMAT_FLAGS);
829+
830+
if (hdr == (PGShmemHeader *) -1)
831+
return NULL; /* failed: must be some other app's */
832+
833+
if (hdr->magic != PGShmemMagic)
834+
{
835+
shmdt((void *) hdr);
836+
return NULL; /* segment belongs to a non-Postgres app */
837+
}
838+
839+
return hdr;
840+
}

0 commit comments

Comments
 (0)