Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 80ef926

Browse files
tglsfdctglsfdc
committed
Improve our ability to detect bogus pointers passed to pfree et al.
Commit c6e0fe1 was a shade too trusting that any pointer passed to pfree, repalloc, etc will point at a valid chunk. Notably, passing a pointer that was actually obtained from malloc tended to result in obscure assertion failures, if not worse. (On FreeBSD I've seen such mistakes take down the entire cluster, seemingly as a result of clobbering shared memory.) To improve matters, extend the mcxt_methods[] array so that it has entries for every possible MemoryContextMethodID bit-pattern, with the currently unassigned ID codes pointing to error-reporting functions. Then, fiddle with the ID assignments so that patterns likely to be associated with bad pointers aren't valid ID codes. In particular, we should avoid assigning bit patterns 000 (zeroed memory) and 111 (wipe_mem'd memory). It turns out that on glibc (Linux), malloc uses chunk headers that have flag bits in the same place we keep MemoryContextMethodID, and that the bit patterns 000, 001, 010 are the only ones we'll see as long as the backend isn't threaded. So we can have very robust detection of pfree'ing a malloc-assigned block on that platform, at least so long as we can refrain from using up those ID codes. On other platforms, we don't have such a good guarantee, but keeping 000 reserved will be enough to catch many such cases. While here, make GetMemoryChunkMethodID() local to mcxt.c, as there seems no need for it to be exposed even in memutils_internal.h. Patch by me, with suggestions from Andres Freund and David Rowley. Discussion: https://postgr.es/m/2910981.1665080361@sss.pgh.pa.us
1 parent e555565 commit 80ef926

File tree

2 files changed

+124
-28
lines changed

2 files changed

+124
-28
lines changed

‎src/backend/utils/mmgr/mcxt.c

Lines changed: 110 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@
3232
#include "utils/memutils_internal.h"
3333

3434

35+
static void BogusFree(void *pointer);
36+
static void *BogusRealloc(void *pointer, Size size);
37+
static MemoryContext BogusGetChunkContext(void *pointer);
38+
static Size BogusGetChunkSpace(void *pointer);
39+
3540
/*****************************************************************************
3641
* GLOBAL MEMORY *
3742
*****************************************************************************/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
7479
[MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
7580
[MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
7681
[MCTX_SLAB_ID].is_empty = SlabIsEmpty,
77-
[MCTX_SLAB_ID].stats = SlabStats
82+
[MCTX_SLAB_ID].stats = SlabStats,
7883
#ifdef MEMORY_CONTEXT_CHECKING
79-
,[MCTX_SLAB_ID].check = SlabCheck
84+
[MCTX_SLAB_ID].check = SlabCheck,
8085
#endif
86+
87+
/*
88+
* Unused (as yet) IDs should have dummy entries here. This allows us to
89+
* fail cleanly if a bogus pointer is passed to pfree or the like. It
90+
* seems sufficient to provide routines for the methods that might get
91+
* invoked from inspection of a chunk (see MCXT_METHOD calls below).
92+
*/
93+
94+
[MCTX_UNUSED1_ID].free_p = BogusFree,
95+
[MCTX_UNUSED1_ID].realloc = BogusRealloc,
96+
[MCTX_UNUSED1_ID].get_chunk_context = BogusGetChunkContext,
97+
[MCTX_UNUSED1_ID].get_chunk_space = BogusGetChunkSpace,
98+
99+
[MCTX_UNUSED2_ID].free_p = BogusFree,
100+
[MCTX_UNUSED2_ID].realloc = BogusRealloc,
101+
[MCTX_UNUSED2_ID].get_chunk_context = BogusGetChunkContext,
102+
[MCTX_UNUSED2_ID].get_chunk_space = BogusGetChunkSpace,
103+
104+
[MCTX_UNUSED3_ID].free_p = BogusFree,
105+
[MCTX_UNUSED3_ID].realloc = BogusRealloc,
106+
[MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
107+
[MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
108+
109+
[MCTX_UNUSED4_ID].free_p = BogusFree,
110+
[MCTX_UNUSED4_ID].realloc = BogusRealloc,
111+
[MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
112+
[MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
113+
114+
[MCTX_UNUSED5_ID].free_p = BogusFree,
115+
[MCTX_UNUSED5_ID].realloc = BogusRealloc,
116+
[MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
117+
[MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
81118
};
82119

83120
/*
@@ -125,6 +162,77 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
125162
#define MCXT_METHOD(pointer, method) \
126163
mcxt_methods[GetMemoryChunkMethodID(pointer)].method
127164

165+
/*
166+
* GetMemoryChunkMethodID
167+
* Return the MemoryContextMethodID from the uint64 chunk header which
168+
* directly precedes 'pointer'.
169+
*/
170+
static inline MemoryContextMethodID
171+
GetMemoryChunkMethodID(const void *pointer)
172+
{
173+
uint64 header;
174+
175+
/*
176+
* Try to detect bogus pointers handed to us, poorly though we can.
177+
* Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
178+
* allocated chunk.
179+
*/
180+
Assert(pointer == (const void *) MAXALIGN(pointer));
181+
182+
header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
183+
184+
return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
185+
}
186+
187+
/*
188+
* GetMemoryChunkHeader
189+
* Return the uint64 chunk header which directly precedes 'pointer'.
190+
*
191+
* This is only used after GetMemoryChunkMethodID, so no need for error checks.
192+
*/
193+
static inline uint64
194+
GetMemoryChunkHeader(const void *pointer)
195+
{
196+
return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
197+
}
198+
199+
/*
200+
* Support routines to trap use of invalid memory context method IDs
201+
* (from calling pfree or the like on a bogus pointer). As a possible
202+
* aid in debugging, we report the header word along with the pointer
203+
* address (if we got here, there must be an accessible header word).
204+
*/
205+
static void
206+
BogusFree(void *pointer)
207+
{
208+
elog(ERROR, "pfree called with invalid pointer %p (header 0x%016llx)",
209+
pointer, (long long) GetMemoryChunkHeader(pointer));
210+
}
211+
212+
static void *
213+
BogusRealloc(void *pointer, Size size)
214+
{
215+
elog(ERROR, "repalloc called with invalid pointer %p (header 0x%016llx)",
216+
pointer, (long long) GetMemoryChunkHeader(pointer));
217+
return NULL; /* keep compiler quiet */
218+
}
219+
220+
static MemoryContext
221+
BogusGetChunkContext(void *pointer)
222+
{
223+
elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p (header 0x%016llx)",
224+
pointer, (long long) GetMemoryChunkHeader(pointer));
225+
return NULL; /* keep compiler quiet */
226+
}
227+
228+
static Size
229+
BogusGetChunkSpace(void *pointer)
230+
{
231+
elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p (header 0x%016llx)",
232+
pointer, (long long) GetMemoryChunkHeader(pointer));
233+
return 0; /* keep compiler quiet */
234+
}
235+
128236

129237
/*****************************************************************************
130238
* EXPORTED ROUTINES *

‎src/include/utils/memutils_internal.h

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,26 @@ extern void SlabCheck(MemoryContext context);
7474
* MemoryContextMethodID
7575
* A unique identifier for each MemoryContext implementation which
7676
* indicates the index into the mcxt_methods[] array. See mcxt.c.
77-
* The maximum value for this enum is constrained by
78-
* MEMORY_CONTEXT_METHODID_MASK. If an enum value higher than that is
79-
* required then MEMORY_CONTEXT_METHODID_BITS will need to be increased.
77+
*
78+
* For robust error detection, ensure that MemoryContextMethodID has a value
79+
* for each possible bit-pattern of MEMORY_CONTEXT_METHODID_MASK, and make
80+
* dummy entries for unused IDs in the mcxt_methods[] array. We also try
81+
* to avoid using bit-patterns as valid IDs if they are likely to occur in
82+
* garbage data, or if they could falsely match on chunks that are really from
83+
* malloc not palloc. (We can't tell that for most malloc implementations,
84+
* but it happens that glibc stores flag bits in the same place where we put
85+
* the MemoryContextMethodID, so the possible values are predictable for it.)
8086
*/
8187
typedef enum MemoryContextMethodID
8288
{
89+
MCTX_UNUSED1_ID, /* 000 occurs in never-used memory */
90+
MCTX_UNUSED2_ID, /* glibc malloc'd chunks usually match 001 */
91+
MCTX_UNUSED3_ID, /* glibc malloc'd chunks > 128kB match 010 */
8392
MCTX_ASET_ID,
8493
MCTX_GENERATION_ID,
8594
MCTX_SLAB_ID,
95+
MCTX_UNUSED4_ID, /* available */
96+
MCTX_UNUSED5_ID /* 111 occurs in wipe_mem'd memory */
8697
} MemoryContextMethodID;
8798

8899
/*
@@ -104,27 +115,4 @@ extern void MemoryContextCreate(MemoryContext node,
104115
MemoryContext parent,
105116
const char *name);
106117

107-
/*
108-
* GetMemoryChunkMethodID
109-
* Return the MemoryContextMethodID from the uint64 chunk header which
110-
* directly precedes 'pointer'.
111-
*/
112-
static inline MemoryContextMethodID
113-
GetMemoryChunkMethodID(void *pointer)
114-
{
115-
uint64 header;
116-
117-
/*
118-
* Try to detect bogus pointers handed to us, poorly though we can.
119-
* Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
120-
* allocated chunk.
121-
*/
122-
Assert(pointer != NULL);
123-
Assert(pointer == (void *) MAXALIGN(pointer));
124-
125-
header = *((uint64 *) ((char *) pointer - sizeof(uint64)));
126-
127-
return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
128-
}
129-
130118
#endif /* MEMUTILS_INTERNAL_H */

0 commit comments

Comments
 (0)