|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)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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 http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
|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)|