On Thu, Jan 5, 2023 at 6:18 AM David Rowley <dgrowle...@gmail.com> wrote:
> On Tue, 3 Jan 2023 at 10:25, Tom Lane <t...@sss.pgh.pa.us> wrote: > > The thing that I find really distressing here is that it's been > > like this for years and none of our automated testing caught it. > > You'd have expected valgrind testing to do so ... but it does not, > > because we've never marked that word NOACCESS. Maybe we should > > rethink that? It'd require making mcxt.c do some valgrind flag > > manipulations so it could access the hdrmask when appropriate. > > Yeah, that probably could have been improved during the recent change. > Here's a patch for it. Thanks for the patch. With it Valgrind is able to catch the invalid read discussed in the initial email of this thread. VALGRINDERROR-BEGIN Invalid read of size 8 at 0x4DB056: ExecInitAgg by 0x4C486A: ExecInitNode by 0x4B92B7: InitPlan by 0x4B81D7: standard_ExecutorStart by 0x4B7F1B: ExecutorStart I reviewed this patch and have some comments. It seems that for MemoryContextMethods in alignedalloc.c the access to the chunk header is not wrapped by VALGRIND_MAKE_MEM_DEFINED and VALGRIND_MAKE_MEM_NOACCESS. Should we do that? In GenerationFree, I think the VALGRIND_MAKE_MEM_DEFINED should be moved to the start of this function, before we call MemoryChunkIsExternal. In SlabFree, we should call MemoryChunkGetBlock after the call of VALGRIND_MAKE_MEM_DEFINED, just like how we do in SlabRealloc. In AllocSetStats, we have a call of MemoryChunkGetValue in Assert. I think we should wrap it with VALGRIND_MAKE_MEM_DEFINED and VALGRIND_MAKE_MEM_NOACCESS. Thanks Richard