Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)

From: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
To: Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
Date: 2020-10-09 22:12:59
Message-ID: 20201009221259.hz663wygcsuly4vc@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Oct 09, 2020 at 05:50:09PM -0300, Ranier Vilela wrote:
>Em sex., 9 de out. de 2020 às 17:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:
>> Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> writes:
>> > The trap is not on the second part of expression. Is in the first.
>> > If the pointer is NULL, ExecCopySlot will be called.
>> Your initial comment indicated that you were worried about
>> IncrementalSortState's group_pivot slot, but that is never going
>> to be null in any execution function of nodeIncrementalSort.c,
>> because ExecInitIncrementalSort always creates it.
>> (The test whether it's NULL in ExecReScanIncrementalSort therefore
>> seems rather useless and misleading, but it's not actually a bug.)
>> The places that use TupIsNull are just doing so because that's
>> the standard way to check whether a slot is empty. The null
>> test inside the macro is pointless in this context (and in a lot
>> of its other use-cases, too) but we don't worry about that.
>So I said that TupIsNull was not the most appropriate.
>Doesn't it look better?
>(node->group_pivot != NULL && TTS_EMPTY(node->group_pivot))

My (admittedly very subjective) opinion is that it looks much worse. The
TupIsNull is pretty self-descriptive, unlike this proposed code.

That could be fixed by defining a new macro, perhaps something like
SlotIsEmpty() or so. But as was already explained, Incremental Sort
can't actually have a NULL slot here, so it makes no difference there.
And in the other places we can't just mechanically replace the macros
because it'd quite likely silently hide pre-existing bugs.


Tomas Vondra
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-10-09 22:45:23 Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
Previous Message Tomas Vondra 2020-10-09 22:04:07 Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)