[prev in list] [next in list] [prev in thread] [next in thread]
List: postgresql-general
Subject: Re: [HACKERS] Missing checks when malloc returns NULL...
From: Tom Lane <tgl () sss ! pgh ! pa ! us>
Date: 2016-08-31 15:14:33
Message-ID: 27675.1472656473 () sss ! pgh ! pa ! us
[Download RAW message or body]
------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <27617.1472656360.1@sss.pgh.pa.us>
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
------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="malloc-nulls-v7.patch"; charset="us-ascii"
Content-ID: <27617.1472656360.2@sss.pgh.pa.us>
Content-Description: malloc-nulls-v7.patch
Content-Transfer-Encoding: quoted-printable
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,
------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
------- =_aaaaaaaaaa0--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic