Обсуждение: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)

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.

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.


regards

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

Ranier Vilela <ranier.vf@gmail.com> writes:
> The trap is not on the second part of expression. Is in the first.
> If the pointer is NULL, ExecCopySlot will be called.

Your initial comment indicated that you were worried about
IncrementalSortState's group_pivot slot, but that is never going
to be null in any execution function of nodeIncrementalSort.c,
because ExecInitIncrementalSort always creates it.

(The test whether it's NULL in ExecReScanIncrementalSort therefore
seems rather useless and misleading, but it's not actually a bug.)

The places that use TupIsNull are just doing so because that's
the standard way to check whether a slot is empty.  The null
test inside the macro is pointless in this context (and in a lot
of its other use-cases, too) but we don't worry about that.

            regards, tom lane

On Fri, Oct 9, 2020 at 1:28 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> 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.

No I didn't.

-- 
Peter Geoghegan

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

Ranier Vilela <ranier.vf@gmail.com> writes:
> So I said that TupIsNull was not the most appropriate.

[ shrug... ]  You're entitled to your opinion, but I see essentially
no value in running around and trying to figure out which TupIsNull
calls actually can see a null pointer and which never will.  It'd
likely introduce bugs, it would certainly not remove any, and there's
no reason to believe that any meaningful performance improvement
could be gained.

(It's possible that the compiler can remove some of the useless
tests, so I'm satisfied to leave such micro-optimization to it.)

            regards, tom lane

Greetings,

* Ranier Vilela (ranier.vf@gmail.com) wrote:
> Em sex., 9 de out. de 2020 às 18:02, Stephen Frost <sfrost@snowman.net>
> escreveu:
> > 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...

Is it though?  Tom already pointed out that the specific case you were
concerned about isn't an issue- I'd encourage you to go review the other
cases that I found and see if you can find any cases where it's actually
going to result in a crash.

If there is such a case, then perhaps we should consider changing
things, but if not, then perhaps there isn't any need to make a change.
I do wonder if maybe some of those cases don't acutally need the
TupIsNull() check at all as we could prove that it won't be by the time
we reach that point, but it depends- and requires more review.

Thanks,

Stephen

On Fri, Oct 9, 2020 at 2:04 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>> > The author of deduplication claimed that he thinks the problem may be in IncrementalSort,
>> > he did not specify which part.
>>
>> No I didn't.
>
> https://www.postgresql.org/message-id/CAH2-Wz=Ae84z0PXTBc+SSGi9EC8nGKn9D16OP-dgH47Jcrv0Ww@mail.gmail.com

That thread is obviously totally unrelated to what you're talking
about. I cannot imagine how you made the connection. The only
commonality is the term "incremental sort".

Moreover, the point that I make in the thread that you link to is that
the bug in question could not possibly be related to the incremental
sort commit. That was an initial quick guess that I made that turned
out to be wrong.

-- 
Peter Geoghegan

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

On Fri, Oct 09, 2020 at 05:50:09PM -0300, Ranier Vilela wrote:
>Em sex., 9 de out. de 2020 às 17:47, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>
>> Ranier Vilela <ranier.vf@gmail.com> writes:
>> > The trap is not on the second part of expression. Is in the first.
>> > If the pointer is NULL, ExecCopySlot will be called.
>>
>> Your initial comment indicated that you were worried about
>> IncrementalSortState's group_pivot slot, but that is never going
>> to be null in any execution function of nodeIncrementalSort.c,
>> because ExecInitIncrementalSort always creates it.
>>
>> (The test whether it's NULL in ExecReScanIncrementalSort therefore
>> seems rather useless and misleading, but it's not actually a bug.)
>>
>> The places that use TupIsNull are just doing so because that's
>> the standard way to check whether a slot is empty.  The null
>> test inside the macro is pointless in this context (and in a lot
>> of its other use-cases, too) but we don't worry about that.
>>
>So I said that TupIsNull was not the most appropriate.
>
>Doesn't it look better?
>(node->group_pivot != NULL && TTS_EMPTY(node->group_pivot))
>

My (admittedly very subjective) opinion is that it looks much worse. The
TupIsNull is pretty self-descriptive, unlike this proposed code.

That could be fixed by defining a new macro, perhaps something like
SlotIsEmpty() or so. But as was already explained, Incremental Sort
can't actually have a NULL slot here, so it makes no difference there.
And in the other places we can't just mechanically replace the macros
because it'd quite likely silently hide pre-existing bugs.


regards

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

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> My (admittedly very subjective) opinion is that it looks much worse. The
> TupIsNull is pretty self-descriptive, unlike this proposed code.

+1

> That could be fixed by defining a new macro, perhaps something like
> SlotIsEmpty() or so. But as was already explained, Incremental Sort
> can't actually have a NULL slot here, so it makes no difference there.
> And in the other places we can't just mechanically replace the macros
> because it'd quite likely silently hide pre-existing bugs.

IME, there are basically two use-cases for TupIsNull in the executor:

1. Checking whether a lower-level plan node has returned an actual
tuple or an EOF indicator.  In current usage, both parts of the
TupIsNull test are needed here, because some node types like to
return NULL pointers while others do something like
"return ExecClearTuple(myslot)".

2. Manipulating a locally-managed slot.  In just about every case
of this sort, the slot is created during the node Init function,
so that the NULL test in TupIsNull is unnecessary and what we are
really interested in is the empty-or-not state of the slot.

Thus, Ranier's concern would be valid if a node ever did anything
with a returned-from-lower-level slot after failing the TupIsNull
check on it.  But there's really no reason to do so, and furthermore
doing so would be a logic bug in itself.  (Something like ExecCopySlot
into the slot, for example, is flat out wrong, because an upper level
node is *never* entitled to scribble on the output slot of a lower-level
node.)  So I seriously, seriously doubt that there are any live bugs
of this ilk.

In principle we could invent SlotIsEmpty() and apply it in use
cases of type 2, but I don't really think that'd be a productive
activity.  In return for saving a few cycles we'd have a nontrivial
risk of new bugs from using the wrong macro for the case at hand.

I do wonder whether we should try to simplify the inter-node
API by allowing only one of the two cases for EOF indicators.
Not convinced it's worth troubling over, though.

            regards, tom lane