Skip to content

Commit 414d662

Browse files
david-rowleydavid-rowley
committed
Adjust Valgrind macro usage to protect chunk headers
Prior to this commit we only ever protected MemoryChunk's requested_size field with Valgrind NOACCESS. This means that if the hdrmask field is ever accessed accidentally then we're not going to get any warnings from Valgrind about it. Valgrind would have warned us about the problem fixed in 92957ed had we already been doing this. Per suggestion from Tom Lane Reviewed-by: Richard Guo Discussion: https://postgr.es/m/1650235.1672694719@sss.pgh.pa.us Discussion: https://postgr.es/m/CAApHDvr=FZNGbj252Z6M9BSFKoq6BMxgkQ2yEAGUYoo7RquqZg@mail.gmail.com
1 parent 43a33ef commit 414d662

File tree

5 files changed

+170
-74
lines changed

5 files changed

+170
-74
lines changed

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

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ AlignedAllocFree(void *pointer)
3131
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
3232
void *unaligned;
3333

34+
VALGRIND_MAKE_MEM_DEFINED(chunk, sizeof(MemoryChunk));
35+
3436
Assert(!MemoryChunkIsExternal(chunk));
3537

3638
/* obtain the original (unaligned) allocated pointer */
@@ -58,12 +60,17 @@ void *
5860
AlignedAllocRealloc(void *pointer, Size size)
5961
{
6062
MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
61-
Size alignto = MemoryChunkGetValue(redirchunk);
62-
void *unaligned = MemoryChunkGetBlock(redirchunk);
63+
Size alignto;
64+
void *unaligned;
6365
MemoryContext ctx;
6466
Size old_size;
6567
void *newptr;
6668

69+
VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
70+
71+
alignto = MemoryChunkGetValue(redirchunk);
72+
unaligned = MemoryChunkGetBlock(redirchunk);
73+
6774
/* sanity check this is a power of 2 value */
6875
Assert((alignto & (alignto - 1)) == 0);
6976

@@ -110,11 +117,18 @@ AlignedAllocRealloc(void *pointer, Size size)
110117
MemoryContext
111118
AlignedAllocGetChunkContext(void *pointer)
112119
{
113-
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
120+
MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
121+
MemoryContext cxt;
114122

115-
Assert(!MemoryChunkIsExternal(chunk));
123+
VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
116124

117-
return GetMemoryChunkContext(MemoryChunkGetBlock(chunk));
125+
Assert(!MemoryChunkIsExternal(redirchunk));
126+
127+
cxt = GetMemoryChunkContext(MemoryChunkGetBlock(redirchunk));
128+
129+
VALGRIND_MAKE_MEM_NOACCESS(redirchunk, sizeof(MemoryChunk));
130+
131+
return cxt;
118132
}
119133

120134
/*
@@ -126,7 +140,15 @@ Size
126140
AlignedAllocGetChunkSpace(void *pointer)
127141
{
128142
MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
129-
void *unaligned = MemoryChunkGetBlock(redirchunk);
143+
void *unaligned;
144+
Size space;
145+
146+
VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
147+
148+
unaligned = MemoryChunkGetBlock(redirchunk);
149+
space = GetMemoryChunkSpace(unaligned);
150+
151+
VALGRIND_MAKE_MEM_NOACCESS(redirchunk, sizeof(MemoryChunk));
130152

131-
return GetMemoryChunkSpace(unaligned);
153+
return space;
132154
}

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

Lines changed: 45 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -188,14 +188,6 @@ typedef struct AllocBlockData
188188
char *endptr; /* end of space in this block */
189189
} AllocBlockData;
190190

191-
/*
192-
* Only the "hdrmask" field should be accessed outside this module.
193-
* We keep the rest of an allocated chunk's header marked NOACCESS when using
194-
* valgrind. But note that chunk headers that are in a freelist are kept
195-
* accessible, for simplicity.
196-
*/
197-
#define ALLOCCHUNK_PRIVATE_LEN offsetof(MemoryChunk, hdrmask)
198-
199191
/*
200192
* AllocPointerIsValid
201193
* True iff pointer is valid allocation pointer.
@@ -777,8 +769,8 @@ AllocSetAlloc(MemoryContext context, Size size)
777769
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
778770
chunk_size - size);
779771

780-
/* Disallow external access to private part of chunk header. */
781-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
772+
/* Disallow access to the chunk header. */
773+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
782774

783775
return MemoryChunkGetPointer(chunk);
784776
}
@@ -801,6 +793,9 @@ AllocSetAlloc(MemoryContext context, Size size)
801793
{
802794
AllocFreeListLink *link = GetFreeListLink(chunk);
803795

796+
/* Allow access to the chunk header. */
797+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
798+
804799
Assert(fidx == MemoryChunkGetValue(chunk));
805800

806801
/* pop this chunk off the freelist */
@@ -823,8 +818,8 @@ AllocSetAlloc(MemoryContext context, Size size)
823818
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
824819
GetChunkSizeFromFreeListIdx(fidx) - size);
825820

826-
/* Disallow external access to private part of chunk header. */
827-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
821+
/* Disallow access to the chunk header. */
822+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
828823

829824
return MemoryChunkGetPointer(chunk);
830825
}
@@ -989,8 +984,8 @@ AllocSetAlloc(MemoryContext context, Size size)
989984
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
990985
chunk_size - size);
991986

992-
/* Disallow external access to private part of chunk header. */
993-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
987+
/* Disallow access to the chunk header. */
988+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
994989

995990
return MemoryChunkGetPointer(chunk);
996991
}
@@ -1005,8 +1000,8 @@ AllocSetFree(void *pointer)
10051000
AllocSet set;
10061001
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
10071002

1008-
/* Allow access to private part of chunk header. */
1009-
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
1003+
/* Allow access to the chunk header. */
1004+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
10101005

10111006
if (MemoryChunkIsExternal(chunk))
10121007
{
@@ -1115,8 +1110,8 @@ AllocSetRealloc(void *pointer, Size size)
11151110
Size oldchksize;
11161111
int fidx;
11171112

1118-
/* Allow access to private part of chunk header. */
1119-
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
1113+
/* Allow access to the chunk header. */
1114+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
11201115

11211116
if (MemoryChunkIsExternal(chunk))
11221117
{
@@ -1164,8 +1159,8 @@ AllocSetRealloc(void *pointer, Size size)
11641159
block = (AllocBlock) realloc(block, blksize);
11651160
if (block == NULL)
11661161
{
1167-
/* Disallow external access to private part of chunk header. */
1168-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
1162+
/* Disallow access to the chunk header. */
1163+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
11691164
return NULL;
11701165
}
11711166

@@ -1232,8 +1227,8 @@ AllocSetRealloc(void *pointer, Size size)
12321227
/* Ensure any padding bytes are marked NOACCESS. */
12331228
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
12341229

1235-
/* Disallow external access to private part of chunk header. */
1236-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
1230+
/* Disallow access to the chunk header . */
1231+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
12371232

12381233
return pointer;
12391234
}
@@ -1305,8 +1300,8 @@ AllocSetRealloc(void *pointer, Size size)
13051300
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
13061301
#endif
13071302

1308-
/* Disallow external access to private part of chunk header. */
1309-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
1303+
/* Disallow access to the chunk header. */
1304+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
13101305

13111306
return pointer;
13121307
}
@@ -1332,8 +1327,8 @@ AllocSetRealloc(void *pointer, Size size)
13321327
/* leave immediately if request was not completed */
13331328
if (newPointer == NULL)
13341329
{
1335-
/* Disallow external access to private part of chunk header. */
1336-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
1330+
/* Disallow access to the chunk header. */
1331+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
13371332
return NULL;
13381333
}
13391334

@@ -1374,11 +1369,17 @@ AllocSetGetChunkContext(void *pointer)
13741369
AllocBlock block;
13751370
AllocSet set;
13761371

1372+
/* Allow access to the chunk header. */
1373+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
1374+
13771375
if (MemoryChunkIsExternal(chunk))
13781376
block = ExternalChunkGetBlock(chunk);
13791377
else
13801378
block = (AllocBlock) MemoryChunkGetBlock(chunk);
13811379

1380+
/* Disallow access to the chunk header. */
1381+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
1382+
13821383
Assert(AllocBlockIsValid(block));
13831384
set = block->aset;
13841385

@@ -1396,16 +1397,27 @@ AllocSetGetChunkSpace(void *pointer)
13961397
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
13971398
int fidx;
13981399

1400+
/* Allow access to the chunk header. */
1401+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
1402+
13991403
if (MemoryChunkIsExternal(chunk))
14001404
{
14011405
AllocBlock block = ExternalChunkGetBlock(chunk);
14021406

1407+
/* Disallow access to the chunk header. */
1408+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
1409+
14031410
Assert(AllocBlockIsValid(block));
1411+
14041412
return block->endptr - (char *) chunk;
14051413
}
14061414

14071415
fidx = MemoryChunkGetValue(chunk);
14081416
Assert(FreeListIdxIsValid(fidx));
1417+
1418+
/* Disallow access to the chunk header. */
1419+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
1420+
14091421
return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
14101422
}
14111423

@@ -1471,7 +1483,10 @@ AllocSetStats(MemoryContext context,
14711483
{
14721484
AllocFreeListLink *link = GetFreeListLink(chunk);
14731485

1486+
/* Allow access to the chunk header. */
1487+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
14741488
Assert(MemoryChunkGetValue(chunk) == fidx);
1489+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
14751490

14761491
freechunks++;
14771492
freespace += chksz + ALLOC_CHUNKHDRSZ;
@@ -1566,8 +1581,8 @@ AllocSetCheck(MemoryContext context)
15661581
Size chsize,
15671582
dsize;
15681583

1569-
/* Allow access to private part of chunk header. */
1570-
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
1584+
/* Allow access to the chunk header. */
1585+
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
15711586

15721587
if (MemoryChunkIsExternal(chunk))
15731588
{
@@ -1617,12 +1632,9 @@ AllocSetCheck(MemoryContext context)
16171632
elog(WARNING, "problem in alloc set %s: detected write past chunk end in block %p, chunk %p",
16181633
name, block, chunk);
16191634

1620-
/*
1621-
* If chunk is allocated, disallow external access to private part
1622-
* of chunk header.
1623-
*/
1635+
/* if chunk is allocated, disallow access to the chunk header */
16241636
if (dsize != InvalidAllocSize)
1625-
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
1637+
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
16261638

16271639
blk_data += chsize;
16281640
nchunks++;

0 commit comments

Comments
 (0)