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

Em sex., 9 de out. de 2020 às 19:45, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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.
>
The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

regards,
Ranier Vilela