Horton

Re: An oversight in ExecInitAgg for grouping sets

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>, David Rowley <dgrowleyml@gmail.com>
Date: 2023-01-02T21:25:19Z
Lists: pgsql-hackers 
Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> If it is an empty grouping set, its length will be zero, and accessing
>> phasedata->eqfunctions[length - 1] is not right.

> Attached is a trivial patch for the fix.

Agreed, that's a latent bug.  It's only latent because the word just
before a palloc chunk will never be zero, either in our historical
palloc code or in v16's shiny new implementation.  Nonetheless it
is a horrible idea for ExecInitAgg to depend on that fact, so I
pushed your fix.

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.

			regards, tom lane

Commits

  1. Adjust Valgrind macro usage to protect chunk headers

  2. Fix typo in memutils_memorychunk.h

  3. Avoid reference to nonexistent array element in ExecInitAgg().