Re: PoC: VALGRIND_MAKE_MEM_NOACCESS for dynamic shared memory

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