public inbox for [email protected]  

help / color / mirror / Atom feed

From: Richard Guo <[email protected]>
To: David Rowley <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: An oversight in ExecInitAgg for grouping sets
Date: Thu, 5 Jan 2023 15:06:21 +0800
Message-ID: <CAMbWs4-aqpZ0t_jbGWnY9VbgAkfk8qitPeDjRC-_mjdQ_40kYA@mail.gmail.com> (raw)
In-Reply-To: <CAApHDvqOnG67BS8SuqT55MvfNmZicc1_aPfBk+Nr-Sw5WcPLWA@mail.gmail.com>
References: <CAMbWs4-vZuuPOZsKOYnSAaPYGKhmacxhki+vpOKk0O7rymccXQ@mail.gmail.com>
	<CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>
	<[email protected]>
	<CAApHDvqOnG67BS8SuqT55MvfNmZicc1_aPfBk+Nr-Sw5WcPLWA@mail.gmail.com>

On Thu, Jan 5, 2023 at 6:18 AM David Rowley <[email protected]> wrote:

> On Tue, 3 Jan 2023 at 10:25, Tom Lane <[email protected]> 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

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: An oversight in ExecInitAgg for grouping sets
  In-Reply-To: <CAMbWs4-aqpZ0t_jbGWnY9VbgAkfk8qitPeDjRC-_mjdQ_40kYA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox