Re: An oversight in ExecInitAgg for grouping sets

Поиск
Список
Период
Сортировка
От Richard Guo
Тема Re: An oversight in ExecInitAgg for grouping sets
Дата
Msg-id CAMbWs4-aqpZ0t_jbGWnY9VbgAkfk8qitPeDjRC-_mjdQ_40kYA@mail.gmail.com
обсуждение исходный текст
Ответ на Re: An oversight in ExecInitAgg for grouping sets  (David Rowley <dgrowleyml@gmail.com>)
Ответы Re: An oversight in ExecInitAgg for grouping sets
Список pgsql-hackers
Дерево обсуждения

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

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Amit Kapila
Дата:
Сообщение: Re: pgsql: Delay commit status checks until freezing executes.
Следующее
От: "David G. Johnston"
Дата:
Сообщение: Re: Resolve iso-8859-1 type to relevant type instead of text type while bulk update using values