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

On Fri, Oct 09, 2020 at 05:25:02PM -0300, Ranier Vilela wrote:
>Em sex., 9 de out. de 2020 às 14:05, Tomas Vondra <
>tomas(dot)vondra(at)2ndquadrant(dot)com> escreveu:
>
>> On Fri, Oct 09, 2020 at 12:24:16PM -0300, Ranier Vilela wrote:
>> >I think that TupIsNull macro is no longer appropriate, to protect
>> >ExecCopySlot.
>> >
>> >See at tuptable.h:
>> >#define TupIsNull(slot) \
>> >((slot) == NULL || TTS_EMPTY(slot))
>> >
>> >If var node->group_pivot is NULL, ExecCopySlot will
>> >dereference a null pointer (first arg).
>> >
>>
>> No. The C standard says there's a "sequence point" [1] between the left
>> and right arguments of the || operator, and that the expressions are
>> evaluated from left to right. So the program will do the first check,
>> and if the pointer really is NULL it won't do the second one (because
>> that is not necessary for determining the result). Similarly for the &&
>> operator, of course.
>>
>Really.
>The trap is not on the second part of expression. Is in the first.
>If the pointer is NULL, ExecCopySlot will be called.
>

Ah, OK. Now I see what you meant. Well, yeah - calling ExecCopySlot with
NULL would be bad, but as others pointed out most of the call sites
don't really have the issue for other reasons.

regards

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