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

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.

For convenience, I will reproduce it:
static inline TupleTableSlot *
ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
Assert(!TTS_EMPTY(srcslot));
AssertArg(srcslot != dstslot);

dstslot->tts_ops->copyslot(dstslot, srcslot);

return dstslot;
}

The second arg is not empty? Yes.
The second arg is different from the first arg (NULL)? Yes.

dstslot->tts_ops->copyslot(dstslot, srcslot); // dereference dstslot (which
is NULL)

>
> Had this been wrong, surely some of the the other places TupIsNull would
> be wrong too (and there are hundreds of them).
>
> >Maybe, this can be related to a bug reported in the btree deduplication.
> >
>
> Not sure which bug you mean, but this piece of code is pretty unrelated
> to btree in general, so I don't see any link.
>
Sorry, can't find the thread.
The author of deduplication claimed that he thinks the problem may be in
IncrementalSort,
he did not specify which part.

regards,
Ranier Vilela