From 1de691bf2421c3c87fc513f4c2155f140d21e2c0 Mon Sep 17 00:00:00 2001 From: David Rowley Date: Fri, 9 Sep 2022 13:11:32 +1200 Subject: [PATCH v1 2/4] Add a magic number to all MemoryChunks and verify We only do this for MEMORY_CONTEXT_CHECKING builds. The aim here is to give us more confidence that we're dealing with chunks allocated by a MemoryContext in various cases where there's a chance that we might not be. --- src/backend/utils/mmgr/aset.c | 6 +-- src/backend/utils/mmgr/generation.c | 6 +-- src/include/utils/memutils_memorychunk.h | 48 ++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index 24ab7399a3..b435624d8f 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -1346,16 +1346,16 @@ AllocSetContains(MemoryContext context, void *pointer) AllocSet set = (AllocSet) context; /* - * We must use MemoryChunkIsExternalUnSafePointer() instead of + * We must use MemoryChunkIsExternalUnsafePointer() instead of * MemoryChunkIsExternal() here as 'chunk' may not be a MemoryChunk if * 'pointer' is not pointing to palloced memory. Below we're careful * never to dereference 'block' as it could point to memory not owned by * this process. */ - if (MemoryChunkIsExternalUnSafePointer(chunk)) + if (MemoryChunkIsExternalUnsafePointer(chunk)) block = ExternalChunkGetBlock(chunk); else - block = (AllocBlock) MemoryChunkGetBlock(chunk); + block = (AllocBlock) MemoryChunkGetBlockUnsafePointer(chunk); for (AllocBlock blk = set->blocks; blk != NULL; blk = blk->next) { diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c index 212aba6bf3..f6a3a01912 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -862,16 +862,16 @@ GenerationContains(MemoryContext context, void *pointer) dlist_iter iter; /* - * We must use MemoryChunkIsExternalUnSafePointer() instead of + * We must use MemoryChunkIsExternalUnsafePointer() instead of * MemoryChunkIsExternal() here as 'chunk' may not be a MemoryChunk if * 'pointer' is not pointing to palloced memory. Below we're careful * never to dereference 'block' as it could point to memory not owned by * this process. */ - if (MemoryChunkIsExternalUnSafePointer(chunk)) + if (MemoryChunkIsExternalUnsafePointer(chunk)) block = ExternalChunkGetBlock(chunk); else - block = (GenerationBlock *) MemoryChunkGetBlock(chunk); + block = (GenerationBlock *) MemoryChunkGetBlockUnsafePointer(chunk); dlist_foreach(iter, &set->blocks) { diff --git a/src/include/utils/memutils_memorychunk.h b/src/include/utils/memutils_memorychunk.h index 721c7d004c..9b267e9ce4 100644 --- a/src/include/utils/memutils_memorychunk.h +++ b/src/include/utils/memutils_memorychunk.h @@ -54,7 +54,7 @@ * Determine if the given MemoryChunk is externally managed, i.e. * MemoryChunkSetHdrMaskExternal() was called on the chunk. * - * MemoryChunkIsExternalUnSafePointer: + * MemoryChunkIsExternalUnsafePointer: * As MemoryChunkIsExternal but safe to use on pointers that we're * uncertain are pointers to MemoryChunks. Callers must be careful * not to put too much trust into the return value. The primary usecase @@ -68,6 +68,12 @@ * For non-external chunks, return a pointer to the block as it was set * in the call to MemoryChunkSetHdrMask(). * + * MemoryChunkGetBlockUnsafePointer: + * As MemoryChunkGetBlock but safe to use on pointers that we're + * uncertain are pointers to MemoryChunks. Callers must be careful + * not to put too much trust into the return value. The primary usecase + * for this is to implement MemoryContextContains. + * * Also exports: * MEMORYCHUNK_MAX_VALUE * MEMORYCHUNK_MAX_BLOCKOFFSET @@ -113,9 +119,18 @@ MEMORYCHUNK_VALUE_BASEBIT << \ MEMORYCHUNK_VALUE_BASEBIT) +#ifdef MEMORY_CONTEXT_CHECKING +#if SIZEOF_SIZE_T >= 8 +#define MEMORYCHUNK_MAGIC2 UINT64CONST(0xB1A8DB858EB6EFBA) +#else +#define MEMORYCHUNK_MAGIC2 0x8EB6EFBA +#endif +#endif + typedef struct MemoryChunk { #ifdef MEMORY_CONTEXT_CHECKING + Size chunk_magic; /* magic number */ Size requested_size; #endif @@ -167,6 +182,9 @@ MemoryChunkSetHdrMask(MemoryChunk *chunk, void *block, Assert(value <= MEMORYCHUNK_MAX_VALUE); Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK); +#ifdef MEMORY_CONTEXT_CHECKING + chunk->chunk_magic = MEMORYCHUNK_MAGIC2; +#endif chunk->hdrmask = (((uint64) blockoffset) << MEMORYCHUNK_BLOCKOFFSET_BASEBIT) | (((uint64) value) << MEMORYCHUNK_VALUE_BASEBIT) | methodid; @@ -183,6 +201,9 @@ MemoryChunkSetHdrMaskExternal(MemoryChunk *chunk, { Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK); +#ifdef MEMORY_CONTEXT_CHECKING + chunk->chunk_magic = MEMORYCHUNK_MAGIC2; +#endif chunk->hdrmask = MEMORYCHUNK_MAGIC | (((uint64) 1) << MEMORYCHUNK_EXTERNAL_BASEBIT) | methodid; } @@ -200,12 +221,13 @@ MemoryChunkIsExternal(MemoryChunk *chunk) */ Assert(!HdrMaskIsExternal(chunk->hdrmask) || HdrMaskCheckMagic(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return HdrMaskIsExternal(chunk->hdrmask); } /* - * MemoryChunkIsExternalUnSafePointer + * MemoryChunkIsExternalUnsafePointer * As MemoryChunkIsExternal only without any Assert checking. This * version should only be used when we're uncertain of 'chunk' is * actually a pointer to a MemoryChunk. @@ -214,7 +236,7 @@ MemoryChunkIsExternal(MemoryChunk *chunk) * value as 'chunk' may not be a MemoryChunk. */ static inline bool -MemoryChunkIsExternalUnSafePointer(MemoryChunk *chunk) +MemoryChunkIsExternalUnsafePointer(MemoryChunk *chunk) { return HdrMaskIsExternal(chunk->hdrmask); } @@ -228,6 +250,7 @@ static inline Size MemoryChunkGetValue(MemoryChunk *chunk) { Assert(!HdrMaskIsExternal(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return HdrMaskGetValue(chunk->hdrmask); } @@ -241,15 +264,34 @@ static inline void * MemoryChunkGetBlock(MemoryChunk *chunk) { Assert(!HdrMaskIsExternal(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return (void *) ((char *) chunk - HdrMaskBlockOffset(chunk->hdrmask)); } +/* + * MemoryChunkGetBlockUnsafePointer + * For non-external chunks, returns the pointer to the block as was set + * in MemoryChunkSetHdrMask. This differs from MemoryChunkGetBlock as + * the coding here does not assume 'chunk' is a valid MemoryChunk. + * We so still assume that the caller checked if the external bit was not + * set with MemoryChunkIsExternalUnsafePointer. + */ +static inline void * +MemoryChunkGetBlockUnsafePointer(MemoryChunk *chunk) +{ + Assert(!HdrMaskIsExternal(chunk->hdrmask)); + + return (void *) ((char *) chunk - HdrMaskBlockOffset(chunk->hdrmask)); +} + + /* cleanup all internal definitions */ #undef MEMORYCHUNK_EXTERNAL_BASEBIT #undef MEMORYCHUNK_VALUE_BASEBIT #undef MEMORYCHUNK_BLOCKOFFSET_BASEBIT #undef MEMORYCHUNK_MAGIC +#undef MEMORYCHUNK_MAGIC2 #undef HdrMaskIsExternal #undef HdrMaskGetValue #undef HdrMaskBlockOffset -- 2.34.1