Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
От | Stephen Frost |
---|---|
Тема | Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c) |
Дата | |
Msg-id | 20201009210246.GH19056@tamriel.snowman.net обсуждение исходный текст |
Ответ на | Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c) (Ranier Vilela <ranier.vf@gmail.com>) |
Ответы |
Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
(Ranier Vilela <ranier.vf@gmail.com>)
|
Список | pgsql-hackers |
Greetings, * Ranier Vilela (ranier.vf@gmail.com) wrote: > Em sex., 9 de out. de 2020 às 14:05, Tomas Vondra < > tomas.vondra@2ndquadrant.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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Ranier VilelaДата:
Сообщение: Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
Следующее
От: Tom LaneДата:
Сообщение: Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)