Re: Missing checks when malloc returns NULL... - Mailing list pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:
> Which means that processes have an escape window when initializing
> shared memory by cleaning up the index if an entry cannot be found and
> then cannot be created properly. I don't think that it is a good idea
> to change that by forcing ShmemAlloc to fail. So I would tend to just
> have the patch attached and add those missing NULL-checks on all the
> existing ShmemAlloc() calls.

> Opinions?

I still think it'd be better to fix that as attached, because it
represents a net reduction not net addition of code, and it provides
a defense against future repetitions of the same omission.  If only
4 out of 11 existing calls were properly checked --- some of them
adjacent to calls with checks --- that should tell us that we *will*
have more instances of the same bug if we don't fix it centrally.

I also note that your patch missed checks for two ShmemAlloc calls in
InitShmemAllocation and ShmemInitStruct.  Admittedly, since those are
the very first such calls, it's highly unlikely they'd fail; but I think
this exercise is not about dismissing failures as improbable.  Almost
all of these failures are improbable, given that we precalculate the
shmem space requirement.

            regards, tom lane

diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 1efe020..cc3af2d 100644
*** a/src/backend/storage/ipc/shmem.c
--- b/src/backend/storage/ipc/shmem.c
*************** InitShmemAllocation(void)
*** 163,177 ****
  /*
   * ShmemAlloc -- allocate max-aligned chunk from shared memory
   *
!  * Assumes ShmemLock and ShmemSegHdr are initialized.
   *
!  * Returns: real pointer to memory or NULL if we are out
!  *        of space.  Has to return a real pointer in order
!  *        to be compatible with malloc().
   */
  void *
  ShmemAlloc(Size size)
  {
      Size        newStart;
      Size        newFree;
      void       *newSpace;
--- 163,194 ----
  /*
   * ShmemAlloc -- allocate max-aligned chunk from shared memory
   *
!  * Throws error if request cannot be satisfied.
   *
!  * Assumes ShmemLock and ShmemSegHdr are initialized.
   */
  void *
  ShmemAlloc(Size size)
  {
+     void       *newSpace;
+
+     newSpace = ShmemAllocNoError(size);
+     if (!newSpace)
+         ereport(ERROR,
+                 (errcode(ERRCODE_OUT_OF_MEMORY),
+                  errmsg("out of shared memory (%zu bytes requested)",
+                         size)));
+     return newSpace;
+ }
+
+ /*
+  * ShmemAllocNoError -- allocate max-aligned chunk from shared memory
+  *
+  * As ShmemAlloc, but returns NULL if out of space, rather than erroring.
+  */
+ void *
+ ShmemAllocNoError(Size size)
+ {
      Size        newStart;
      Size        newFree;
      void       *newSpace;
*************** ShmemAlloc(Size size)
*** 206,216 ****

      SpinLockRelease(ShmemLock);

!     if (!newSpace)
!         ereport(WARNING,
!                 (errcode(ERRCODE_OUT_OF_MEMORY),
!                  errmsg("out of shared memory")));
!
      Assert(newSpace == (void *) CACHELINEALIGN(newSpace));

      return newSpace;
--- 223,229 ----

      SpinLockRelease(ShmemLock);

!     /* note this assert is okay with newSpace == NULL */
      Assert(newSpace == (void *) CACHELINEALIGN(newSpace));

      return newSpace;
*************** ShmemInitHash(const char *name, /* table
*** 293,299 ****
       * The shared memory allocator must be specified too.
       */
      infoP->dsize = infoP->max_dsize = hash_select_dirsize(max_size);
!     infoP->alloc = ShmemAlloc;
      hash_flags |= HASH_SHARED_MEM | HASH_ALLOC | HASH_DIRSIZE;

      /* look it up in the shmem index */
--- 306,312 ----
       * The shared memory allocator must be specified too.
       */
      infoP->dsize = infoP->max_dsize = hash_select_dirsize(max_size);
!     infoP->alloc = ShmemAllocNoError;
      hash_flags |= HASH_SHARED_MEM | HASH_ALLOC | HASH_DIRSIZE;

      /* look it up in the shmem index */
*************** ShmemInitStruct(const char *name, Size s
*** 364,375 ****
               */
              Assert(shmemseghdr->index == NULL);
              structPtr = ShmemAlloc(size);
-             if (structPtr == NULL)
-                 ereport(ERROR,
-                         (errcode(ERRCODE_OUT_OF_MEMORY),
-                          errmsg("not enough shared memory for data structure"
-                                 " \"%s\" (%zu bytes requested)",
-                                 name, size)));
              shmemseghdr->index = structPtr;
              *foundPtr = FALSE;
          }
--- 377,382 ----
*************** ShmemInitStruct(const char *name, Size s
*** 410,416 ****
      else
      {
          /* It isn't in the table yet. allocate and initialize it */
!         structPtr = ShmemAlloc(size);
          if (structPtr == NULL)
          {
              /* out of memory; remove the failed ShmemIndex entry */
--- 417,423 ----
      else
      {
          /* It isn't in the table yet. allocate and initialize it */
!         structPtr = ShmemAllocNoError(size);
          if (structPtr == NULL)
          {
              /* out of memory; remove the failed ShmemIndex entry */
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 7cdb355..4064b20 100644
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
*************** InitPredicateLocks(void)
*** 1184,1195 ****
          requestSize = mul_size((Size) max_table_size,
                                 PredXactListElementDataSize);
          PredXact->element = ShmemAlloc(requestSize);
-         if (PredXact->element == NULL)
-             ereport(ERROR,
-                     (errcode(ERRCODE_OUT_OF_MEMORY),
-              errmsg("not enough shared memory for elements of data structure"
-                     " \"%s\" (%zu bytes requested)",
-                     "PredXactList", requestSize)));
          /* Add all elements to available list, clean. */
          memset(PredXact->element, 0, requestSize);
          for (i = 0; i < max_table_size; i++)
--- 1184,1189 ----
*************** InitPredicateLocks(void)
*** 1255,1266 ****
          requestSize = mul_size((Size) max_table_size,
                                 RWConflictDataSize);
          RWConflictPool->element = ShmemAlloc(requestSize);
-         if (RWConflictPool->element == NULL)
-             ereport(ERROR,
-                     (errcode(ERRCODE_OUT_OF_MEMORY),
-              errmsg("not enough shared memory for elements of data structure"
-                     " \"%s\" (%zu bytes requested)",
-                     "RWConflictPool", requestSize)));
          /* Add all elements to available list, clean. */
          memset(RWConflictPool->element, 0, requestSize);
          for (i = 0; i < max_table_size; i++)
--- 1249,1254 ----
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 9a758bd..33e7023 100644
*** a/src/backend/storage/lmgr/proc.c
--- b/src/backend/storage/lmgr/proc.c
*************** InitProcGlobal(void)
*** 194,207 ****
       * between groups.
       */
      procs = (PGPROC *) ShmemAlloc(TotalProcs * sizeof(PGPROC));
      ProcGlobal->allProcs = procs;
      /* XXX allProcCount isn't really all of them; it excludes prepared xacts */
      ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS;
-     if (!procs)
-         ereport(FATAL,
-                 (errcode(ERRCODE_OUT_OF_MEMORY),
-                  errmsg("out of shared memory")));
-     MemSet(procs, 0, TotalProcs * sizeof(PGPROC));

      /*
       * Also allocate a separate array of PGXACT structures.  This is separate
--- 194,203 ----
       * between groups.
       */
      procs = (PGPROC *) ShmemAlloc(TotalProcs * sizeof(PGPROC));
+     MemSet(procs, 0, TotalProcs * sizeof(PGPROC));
      ProcGlobal->allProcs = procs;
      /* XXX allProcCount isn't really all of them; it excludes prepared xacts */
      ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS;

      /*
       * Also allocate a separate array of PGXACT structures.  This is separate
diff --git a/src/include/storage/shmem.h b/src/include/storage/shmem.h
index 6468e66..2560e6c 100644
*** a/src/include/storage/shmem.h
--- b/src/include/storage/shmem.h
*************** typedef struct SHM_QUEUE
*** 35,40 ****
--- 35,41 ----
  extern void InitShmemAccess(void *seghdr);
  extern void InitShmemAllocation(void);
  extern void *ShmemAlloc(Size size);
+ extern void *ShmemAllocNoError(Size size);
  extern bool ShmemAddrIsValid(const void *addr);
  extern void InitShmemIndex(void);
  extern HTAB *ShmemInitHash(const char *name, long init_size, long max_size,