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

Em sex., 9 de out. de 2020 às 18:02, Stephen Frost <sfrost(at)snowman(dot)net>
escreveu:

> 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...).
>
Several places.
TupIsNull it looks like a minefield...

regards,
Ranier Vilela