Re: PoC: VALGRIND_MAKE_MEM_NOACCESS for dynamic shared memory

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