PoC: VALGRIND_MAKE_MEM_NOACCESS for dynamic shared memory

Hi,

On 2026-05-04 10:18:31 +0200, Tomas Vondra wrote:

A couple of days ago we got a report regarding an incorrect shmem size
calculation in btree [1], leading to a buffer overflow / memory
corruption. Which became a much less common issue in our code, thanks to
valgrind and similar tools. But it took me a while to realize valgrind
won't catch this because we only instrument private memory (palloc et
al), while shmem is left alone.

I was wondering if it's feasible to improve this. Attached is a trivial
patch that adjusts shm_toc.c to add a couple NOACCESS bytes after each
entry in the segment. It seems to do the trick - with this we get a
reasonable report (for the reproducer provided in the bug report, before
it got fixed by 748d871b7c) from valgrind, with invalid accesses. See
the attached .log for an example. It's much better than the confusing
crashes due to corrupted state.

There's an issue, though. It seems the valgrind memory markings are not
shared between processes. The leader sets the shm_toc up, marks the
ranges as NOACCESS, and then checks it while accessing the memory. But
the parallel workers don't seem to see this, and so will produce no
reports. I'm assuming this is the case, because all the reports come
from the leader, never from the workers. Maybe there's a different
explanation (e.g. maybe it's just the leader touching the memory?).

I assume the issue is just that the workers don't have the NOACCESS markers? I
think you'd need to do them in every process using the shm_toc. Either by
doing it in shm_toc_attach() or in shm_toc().

An alternative would be to do mprotect(), but unfortunately it requires
page-aligned ranges, which makes it somewhat useless for small overflows
of a couple bytes (like here). It should work cross-process, I think,

It doesn't work across processes. Every process has their own mprotect "view".

FWIW the PoC patch adds a 32-byte chunk, not just a single byte. This is
intentional, because if the state is an array, it's quite possible the
invalid access steps over a fair number of bytes. I'm actually thinking
it should be even larger.

This modifies just the dynamic shmem, but maybe we should do this even
for the "regular" shmem allocated at start. Issues in that would likely
cause crashes pretty quick (unlike the btree issue, which requires a
somewhat special reproducer), but a nice valgrind report helps.

I can tell you from experience that no, it's not necessarily quickly caught.
So yes, I think we should definitely do that.

/*
* Allocate shared memory from a segment managed by a table of contents.
*
@@ -119,9 +127,19 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
}
vtoc->toc_allocated_bytes += nbytes;

+#ifdef USE_VALGRIND
+	vtoc->toc_allocated_bytes += NUM_NOACCESS_BYTES;
+#endif
+
SpinLockRelease(&toc->toc_mutex);

-	return ((char *) toc) + (total_bytes - allocated_bytes - nbytes);
+#ifdef USE_VALGRIND
+	/* make the bytes at the end no-access */
+	VALGRIND_MAKE_MEM_NOACCESS(((char *) toc) + (total_bytes - allocated_bytes - NUM_NOACCESS_BYTES),
+							   NUM_NOACCESS_BYTES);
+#endif
+
+	return ((char *) toc) + (total_bytes - allocated_bytes - nbytes - NUM_NOACCESS_BYTES);
}

The size is already rounded up by that point:

/*
* Make sure request is well-aligned. XXX: MAXALIGN is not enough,
* because atomic ops might need a wider alignment. We don't have a
* proper definition for the minimum to make atomic ops safe, but
* BUFFERALIGN ought to be enough.
*/
nbytes = BUFFERALIGN(nbytes);

Which means that you'll often have a up to 32byte pad at the end of the
(ALIGNOF_BUFFER=32) allocation already. I don't care about the waste, but the
ALIGNOF_BUFFER padding will often prevent detecting smaller out-of-bounds
accesses.

Greetings,

Andres Freund

On 5/4/26 15:56, Andres Freund wrote:

Hi,

On 2026-05-04 10:18:31 +0200, Tomas Vondra wrote:

A couple of days ago we got a report regarding an incorrect shmem size
calculation in btree [1], leading to a buffer overflow / memory
corruption. Which became a much less common issue in our code, thanks to
valgrind and similar tools. But it took me a while to realize valgrind
won't catch this because we only instrument private memory (palloc et
al), while shmem is left alone.

I was wondering if it's feasible to improve this. Attached is a trivial
patch that adjusts shm_toc.c to add a couple NOACCESS bytes after each
entry in the segment. It seems to do the trick - with this we get a
reasonable report (for the reproducer provided in the bug report, before
it got fixed by 748d871b7c) from valgrind, with invalid accesses. See
the attached .log for an example. It's much better than the confusing
crashes due to corrupted state.

There's an issue, though. It seems the valgrind memory markings are not
shared between processes. The leader sets the shm_toc up, marks the
ranges as NOACCESS, and then checks it while accessing the memory. But
the parallel workers don't seem to see this, and so will produce no
reports. I'm assuming this is the case, because all the reports come
from the leader, never from the workers. Maybe there's a different
explanation (e.g. maybe it's just the leader touching the memory?).

I assume the issue is just that the workers don't have the NOACCESS markers? I
think you'd need to do them in every process using the shm_toc. Either by
doing it in shm_toc_attach() or in shm_toc().

Right, but it'd also need to know how long the entry is. Which AFAIK it
does not. We don't want to redo the calculation in every worker, but we
might add a "len" field to the toc entry.

An alternative would be to do mprotect(), but unfortunately it requires
page-aligned ranges, which makes it somewhat useless for small overflows
of a couple bytes (like here). It should work cross-process, I think,

It doesn't work across processes. Every process has their own mprotect "view".

Ah, OK. So I guessed wrong, and it has the same issue as NOACCESS.

FWIW the PoC patch adds a 32-byte chunk, not just a single byte. This is
intentional, because if the state is an array, it's quite possible the
invalid access steps over a fair number of bytes. I'm actually thinking
it should be even larger.

This modifies just the dynamic shmem, but maybe we should do this even
for the "regular" shmem allocated at start. Issues in that would likely
cause crashes pretty quick (unlike the btree issue, which requires a
somewhat special reproducer), but a nice valgrind report helps.

I can tell you from experience that no, it's not necessarily quickly caught.
So yes, I think we should definitely do that.

Understood.

/*
* Allocate shared memory from a segment managed by a table of contents.
*
@@ -119,9 +127,19 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
}
vtoc->toc_allocated_bytes += nbytes;

+#ifdef USE_VALGRIND
+	vtoc->toc_allocated_bytes += NUM_NOACCESS_BYTES;
+#endif
+
SpinLockRelease(&toc->toc_mutex);

-	return ((char *) toc) + (total_bytes - allocated_bytes - nbytes);
+#ifdef USE_VALGRIND
+	/* make the bytes at the end no-access */
+	VALGRIND_MAKE_MEM_NOACCESS(((char *) toc) + (total_bytes - allocated_bytes - NUM_NOACCESS_BYTES),
+							   NUM_NOACCESS_BYTES);
+#endif
+
+	return ((char *) toc) + (total_bytes - allocated_bytes - nbytes - NUM_NOACCESS_BYTES);
}

The size is already rounded up by that point:

/*
* Make sure request is well-aligned. XXX: MAXALIGN is not enough,
* because atomic ops might need a wider alignment. We don't have a
* proper definition for the minimum to make atomic ops safe, but
* BUFFERALIGN ought to be enough.
*/
nbytes = BUFFERALIGN(nbytes);

Which means that you'll often have a up to 32byte pad at the end of the
(ALIGNOF_BUFFER=32) allocation already. I don't care about the waste, but the
ALIGNOF_BUFFER padding will often prevent detecting smaller out-of-bounds
accesses.

Good point. I forgot about this alignment rounding. I don't care about
the waste either (it'd be a bit silly in a valgrind build anyway), but
not catching smaller mistakes would not be great.

regards

--
Tomas Vondra

Hi!

This looks useful so I took a stab at continuing this work plus adding
NOACCESS sentinels to shared memory allocated in the main segment with
the ShmemAllocator.

For the ShmemAllocator I had originally planned to mark all memory as
NOACCESS and then have ShmemAllocRaw() mark only the allocated regions
as accessible but that did not work with legacy allocations (e.g.
calling ShmemAlloc() directly) and EXEC_BAKCEND as there is no way to
know where those regions were as they are not in ShmemIndex. I
personally think the solution I went with instead, of jsut adding 32
NOACESS bytes at the end of each allocation in shared memory is good
enough even if it does not cover all padding bytes like my old solution
used to do.

What do you think? Is the current solution good enough? I preferred my
original idea of starting out with everything NOACCESS but EXEC_BAKCEND
made that painful to implement.

For shm_toc I fixed the issues pointed out be Andres (the padding issue
and that we can have shm_toc_lookup() set NOACCESS) and made sure we
clear all NOACCESS sentinels on dsm_deatch(). The clearing of the
NOACCESS in dsm_detach() felt a bit ugly but I am not sure there is any
better solution.

I have also attached a third patch which contains my own extension which
I used during development in case anyone would want to use it too. It
adds three functions which can do out of bounds access to an array of 10
32-bit integers. For example the function call below triggers a Valgrind
error when called with an index > 9.

SELECT valgrind_shmem_shm_toc_fg_get(10);

The extension is just a quick and dirty hack I used to test things out,
and nothing I think should get committed as we do not have any test
cases for Valgrind for anything else either.

On 5/4/26 16:21, Tomas Vondra wrote:

I assume the issue is just that the workers don't have the NOACCESS markers? I
think you'd need to do them in every process using the shm_toc. Either by
doing it in shm_toc_attach() or in shm_toc().

I do not think you can do it in shm_toc_attach() because there can be
new allocations between shm_toc_attach() and shm_toc_lookup() so my
patch adds the NOACESS marker in shm_toc_lookup() which should be safe.
So NOACCESS markers are added in both shm_toc_allocate() and
shm_toc_lookup() in my patch.

Sadly it cannot take alignment correctly into account since we do not
save that in the TOC (see below).

Right, but it'd also need to know how long the entry is. Which AFAIK it
does not. We don't want to redo the calculation in every worker, but we
might add a "len" field to the toc entry.

Not without changing the API and unifying shm_toc_allocate() and
shm_toc_insert() since by the time we create a TOC entry we have thrown
away the size of the allocation. I would not mind changing this API but
I think I would need to understand why it is like that before changing
it. It does not feel good to change an API just for Valgrind.

Any idea why these two functions are split and why they are not just one
function call?

Which means that you'll often have a up to 32byte pad at the end of the
(ALIGNOF_BUFFER=32) allocation already. I don't care about the waste, but the
ALIGNOF_BUFFER padding will often prevent detecting smaller out-of-bounds
accesses.

Good point. I forgot about this alignment rounding. I don't care about
the waste either (it'd be a bit silly in a valgrind build anyway), but
not catching smaller mistakes would not be great.

My version fixes this, but is not able to fix this in shm_toc_lookup()
as I mentioned above due to the lack of a length field for the
allocation in the TOC.

--
Andreas Karlsson
Percona

On 7/8/26 02:31, Andreas Karlsson wrote:

Hi!

This looks useful so I took a stab at continuing this work plus adding
NOACCESS sentinels to shared memory allocated in the main segment with
the ShmemAllocator.

For the ShmemAllocator I had originally planned to mark all memory as
NOACCESS and then have ShmemAllocRaw() mark only the allocated regions
as accessible but that did not work with legacy allocations (e.g.
calling ShmemAlloc() directly) and EXEC_BAKCEND as there is no way to
know where those regions were as they are not in ShmemIndex. I
personally think the solution I went with instead, of jsut adding 32
NOACESS bytes at the end of each allocation in shared memory is good
enough even if it does not cover all padding bytes like my old solution
used to do.

What do you think? Is the current solution good enough? I preferred my
original idea of starting out with everything NOACCESS but EXEC_BAKCEND
made that painful to implement.

Hmmm, I agree starting with everything marked as NOACCESS seems like a
cleaner option, but if it doesn't work with EXEC_BACKEND (and if it
can't be made to work) it's irrelevant.

So what happens with EXEC_BACKEND builds? I see the 0002 patch now says:

* When compiled with EXEC_BACKEND we need to recreate all NOACCESS
* regions in each backend, but we can only recreate those with index
* entries and not any of the regions for memory allocated directly with
* ShmemAlloc().

Does it mean EXEC_BACKEND builds won't have any valgrind markings for
the regions allocated by ShmemAlloc()? Or do I misunderstand?

If that's the case, it seems EXEC_BACKEND builds would have limited
checks no matter what. Maybe it'd make sense to use the "clean" approach
for regular builds, and something simpler / limited for EXEC_BACKEND?

It's a bit late over here, so maybe I'm missing something obvious, but
it seems to me marking everything as NOACCESS (the "clean" approach)
would catch even accesses this EXEC_BACKEND-compatible approach would
not catch, right? That does not seem like a great trade off.

For shm_toc I fixed the issues pointed out be Andres (the padding issue
and that we can have shm_toc_lookup() set NOACCESS) and made sure we
clear all NOACCESS sentinels on dsm_deatch(). The clearing of the
NOACCESS in dsm_detach() felt a bit ugly but I am not sure there is any
better solution.

I don't see Andres mentioning shm_toc_lookup explicitly, but I guess
it's what he meant when he mentioned NOACCESS and shm_toc_attach. Or did
I miss an email in this thread?

The dsm_detach business seems a bit strange to me. Can you explain why
we need to clean the NOACCESS sentinels in dsm_detach (I understand why
dsm_detach seems to be the only place to do that, but why do we need
to)? And is DEFINED the right marking? I'd probably briefly explain that
in the dsm_detach() comment.

I mean, we're about to detach the memory, right? So why should it be
accessible or defined?

But if we need to do this stuff in dsm_detach(), maybe we could do
something in dsm_attach() too?

One comments about shm_toc_lookup - the new block says:

/*
* Since we do not know the size of entries we can only use the
* start of the next entry for setting the no-access sentinel.
*/
Size nextoffset = i == 0 ? toc->toc_total_bytes : toc->toc_entry[i -
1].offset;

That seems hard to read, without the "(i == 0)". More importantly, is it
doing what the comment says? That talks about "next entry", but this
looks at the previous entry, no?

I have also attached a third patch which contains my own extension which
I used during development in case anyone would want to use it too. It
adds three functions which can do out of bounds access to an array of 10
32-bit integers. For example the function call below triggers a Valgrind
error when called with an index > 9.

SELECT valgrind_shmem_shm_toc_fg_get(10);

The extension is just a quick and dirty hack I used to test things out,
and nothing I think should get committed as we do not have any test
cases for Valgrind for anything else either.

Thanks. Seems useful for manual testing, but I agree it's not something
we'd want to commit.

On 5/4/26 16:21, Tomas Vondra wrote:

I assume the issue is just that the workers don't have the NOACCESS
markers? I
think you'd need to do them in every process using the shm_toc. 
Either by
doing it in shm_toc_attach() or in shm_toc().

I do not think you can do it in shm_toc_attach() because there can be
new allocations between shm_toc_attach() and shm_toc_lookup() so my
patch adds the NOACESS marker in shm_toc_lookup() which should be safe.
So NOACCESS markers are added in both shm_toc_allocate() and
shm_toc_lookup() in my patch.

I'm not following. Why would new allocations (between shm_toc_attach and
shm_toc_lookup) be an issue?

Sadly it cannot take alignment correctly into account since we do not
save that in the TOC (see below).

Right, but it'd also need to know how long the entry is. Which AFAIK it
does not. We don't want to redo the calculation in every worker, but we
might add a "len" field to the toc entry.

Not without changing the API and unifying shm_toc_allocate() and
shm_toc_insert() since by the time we create a TOC entry we have thrown
away the size of the allocation. I would not mind changing this API but
I think I would need to understand why it is like that before changing
it. It does not feel good to change an API just for Valgrind.

Any idea why these two functions are split and why they are not just one
function call?

I suppose it's split into two functions because we don't want to add
segments that are not fully initialized. The usual use case looks like
this (this is from brin.c):

sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);

If we added both attach+insert, we'd be adding empty string and only
then set it. I suppose that'd not be safe in some cases?

I don't think we'd want to merge these two functions into one, but I
suppose we could add the length to shm_toc_insert(). Which I suppose
simply wasn't needed until now.

But I agree it'd not be great to do this just because of valgrind.
There's a fair number of shm_toc_insert calls (~80), but not terrible.
But we'd need to be careful to protect against shm_toc_insert calls with
a different length value.

I'm not sure what to do. I was thinking we might add a shm_toc length
only in valgrind builds. And then we wouldn't need this NOACCESS_BYTES
stuff at all. Bu I didn't realize we'd need to tweak shm_toc_insert.

Which means that you'll often have a up to 32byte pad at the end of the
(ALIGNOF_BUFFER=32) allocation already. I don't care about the waste,
but the
ALIGNOF_BUFFER padding will often prevent detecting smaller out-of-
bounds
accesses.

Good point. I forgot about this alignment rounding. I don't care about
the waste either (it'd be a bit silly in a valgrind build anyway), but
not catching smaller mistakes would not be great.

My version fixes this, but is not able to fix this in shm_toc_lookup()
as I mentioned above due to the lack of a length field for the
allocation in the TOC.

Yeah.

There was a minor bitrot due to b34fd845e03a, so here's v3.

regards

--
Tomas Vondra