MIME-Version: 1.0
References: <CAMbWs4-vZuuPOZsKOYnSAaPYGKhmacxhki+vpOKk0O7rymccXQ@mail.gmail.com>
 <CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>
 <1650235.1672694719@sss.pgh.pa.us> <CAApHDvqOnG67BS8SuqT55MvfNmZicc1_aPfBk+Nr-Sw5WcPLWA@mail.gmail.com>
In-Reply-To: <CAApHDvqOnG67BS8SuqT55MvfNmZicc1_aPfBk+Nr-Sw5WcPLWA@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 5 Jan 2023 15:06:21 +0800
Message-ID: <CAMbWs4-aqpZ0t_jbGWnY9VbgAkfk8qitPeDjRC-_mjdQ_40kYA@mail.gmail.com>
Subject: Re: An oversight in ExecInitAgg for grouping sets
To: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000e0049c05f17ef0d4"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs4-aqpZ0t_jbGWnY9VbgAkfk8qitPeDjRC-_mjdQ_40kYA%40mail.gmail.com>
Precedence: bulk

--000000000000e0049c05f17ef0d4
Content-Type: text/plain; charset="UTF-8"

On Thu, Jan 5, 2023 at 6:18 AM David Rowley <dgrowleyml@gmail.com> wrote:

> On Tue, 3 Jan 2023 at 10:25, Tom Lane <tgl@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

--000000000000e0049c05f17ef0d4
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Thu, Jan 5, 2023 at 6:18 AM David Rowl=
ey &lt;<a href=3D"mailto:dgrowleyml@gmail.com">dgrowleyml@gmail.com</a>&gt;=
 wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px =
0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, =
3 Jan 2023 at 10:25, Tom Lane &lt;<a href=3D"mailto:tgl@sss.pgh.pa.us" targ=
et=3D"_blank">tgl@sss.pgh.pa.us</a>&gt; wrote:<br>
&gt; The thing that I find really distressing here is that it&#39;s been<br=
>
&gt; like this for years and none of our automated testing caught it.<br>
&gt; You&#39;d have expected valgrind testing to do so ... but it does not,=
<br>
&gt; because we&#39;ve never marked that word NOACCESS.=C2=A0 Maybe we shou=
ld<br>
&gt; rethink that?=C2=A0 It&#39;d require making mcxt.c do some valgrind fl=
ag<br>
&gt; manipulations so it could access the hdrmask when appropriate.<br>
<br>
Yeah, that probably could have been improved during the recent change.<br>
Here&#39;s a patch for it.</blockquote><div>=C2=A0</div><div>Thanks for the=
 patch.=C2=A0 With it Valgrind is able to catch the invalid<br>read discuss=
ed in the initial email of this thread.<br><br>=C2=A0VALGRINDERROR-BEGIN<br=
>=C2=A0Invalid read of size 8<br>=C2=A0 =C2=A0 at 0x4DB056: ExecInitAgg<br>=
=C2=A0 =C2=A0 by 0x4C486A: ExecInitNode<br>=C2=A0 =C2=A0 by 0x4B92B7: InitP=
lan<br>=C2=A0 =C2=A0 by 0x4B81D7: standard_ExecutorStart<br>=C2=A0 =C2=A0 b=
y 0x4B7F1B: ExecutorStart<br><br>I reviewed this patch and have some commen=
ts.<br><br>It seems that for MemoryContextMethods in alignedalloc.c the acc=
ess to<br>the chunk header is not wrapped by VALGRIND_MAKE_MEM_DEFINED and<=
br>VALGRIND_MAKE_MEM_NOACCESS.=C2=A0 Should we do that?<br><br>In Generatio=
nFree, I think the VALGRIND_MAKE_MEM_DEFINED should be moved<br>to the star=
t of this function, before we call MemoryChunkIsExternal.<br><br>In SlabFre=
e, we should call MemoryChunkGetBlock after the call of<br>VALGRIND_MAKE_ME=
M_DEFINED, just like how we do in SlabRealloc.<br><br>In AllocSetStats, we =
have a call of MemoryChunkGetValue in Assert. =C2=A0I<br>think we should wr=
ap it with VALGRIND_MAKE_MEM_DEFINED and<br>VALGRIND_MAKE_MEM_NOACCESS.<br>=
<br>Thanks<br>Richard<br></div></div></div>

--000000000000e0049c05f17ef0d4--