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

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.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services