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

Greetings,

* Ranier Vilela (ranier(dot)vf(at)gmail(dot)com) 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).

[...]

> The trap is not on the second part of expression. Is in the first.
> If the pointer is NULL, ExecCopySlot will be called.

Yeah, that's interesting, and this isn't the only place there's a risk
of that happening, in doing a bit of review of TupIsNull() callers:

src/backend/executor/nodeGroup.c:

if (TupIsNull(firsttupleslot))
{
outerslot = ExecProcNode(outerPlanState(node));
if (TupIsNull(outerslot))
{
/* empty input, so return nothing */
node->grp_done = true;
return NULL;
}
/* Copy tuple into firsttupleslot */
ExecCopySlot(firsttupleslot, outerslot);

Seems that 'firsttupleslot' could possibly be a NULL pointer at this
point?

src/backend/executor/nodeWindowAgg.c:

/* Fetch next row if we didn't already */
if (TupIsNull(agg_row_slot))
{
if (!window_gettupleslot(agg_winobj, winstate->aggregatedupto,
agg_row_slot))
break; /* must be end of partition */
}

If agg_row_slot ends up being an actual NULL pointer, this looks likely
to end up resulting in a crash too.

/*
* If this is the very first partition, we need to fetch the first input
* row to store in first_part_slot.
*/
if (TupIsNull(winstate->first_part_slot))
{
TupleTableSlot *outerslot = ExecProcNode(outerPlan);

if (!TupIsNull(outerslot))
ExecCopySlot(winstate->first_part_slot, outerslot);
else
{
/* outer plan is empty, so we have nothing to do */
winstate->partition_spooled = true;
winstate->more_partitions = false;
return;
}
}

This seems like another risky case, since we don't check that
winstate->first_part_slot is a non-NULL pointer.

if (winstate->frameheadpos == 0 &&
TupIsNull(winstate->framehead_slot))
{
/* fetch first row into framehead_slot, if we didn't already */
if (!tuplestore_gettupleslot(winstate->buffer, true, true,
winstate->framehead_slot))
elog(ERROR, "unexpected end of tuplestore");
}

There's a few of these 'framehead_slot' cases, and then some with
'frametail_slot', all a similar pattern to above.

> 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)

Right, just to try and clarify further, the issue here is with this code:

if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

With TupIsNull defined as:

((slot) == NULL || TTS_EMPTY(slot))

That means we get:

if ((node->group_pivot) == NULL || TTS_EMPTY(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

Which means that if we reach this point with node->group_pivot as NULL,
then we'll pass that to ExecCopySlot() and eventually end up
dereferencing it and crashing.

I haven't tried to run back farther up to see if it's possible that
there's other checks which prevent node->group_pivot (and the other
cases) from actually being a NULL pointer by the time we reach this
code, but I agree that it seems to be a bit concerning to have the code
written this way- TupIsNull() allows the pointer *itself* to be NULL and
callers of it need to realize that (if nothing else by at least
commenting that there's other checks in place to make sure that it can't
end up actually being a NULL pointer when we're passing it to some other
function that'll dereference it).

As a side-note, this kind of further analysis and looking for other,
similar, cases that might be problematic is really helpful and important
to do whenever you come across a case like this, and will also lend a
bit more validation that this is really an issue and something we need
to look at and not a one-off mistake (which, as much as we'd like to
think we never make mistakes, isn't typically the case...).

Thanks,

Stephen