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

On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston <
> david(dot)g(dot)johnston(at)gmail(dot)com> escreveu:
>
>> On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>>
>>> 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
>>>
>>
>> IMO both names are problematic, too data value centric, not semantic.
>> TupIsValid for the name and negating the existing tests would help to at
>> least clear that part up. Then, things operating on invalid tuples would
>> be expected to know about both representations. In the case of
>> ExecCopySlot there is nothing it can do with a null representation of an
>> invalid tuple so it would have to fail if presented one. An assertion
>> seems sufficient.
>>
> IHMO, assertion it is not the solution.
>
> Steven suggested looking for some NULL pointer font above the calls.
> I say that it is not necessary, there is no NULL pointer.
> Whoever guarantees this is the combination, which for me is an assertion.
>
> If (TupIsNull)
> ExecCopySlot
>
> It works as a subject, but in release mode.
> It is the equivalent of:
>
> If (TupIsNull)
> Abort
>
> The only problem for me is that we are running this assertion on the
> clients' machines.
>
>
I cannot make heads nor tails of what you are trying to communicate here.

I'll agree that TupIsNull isn't the most descriptive choice of name, and is
probably being abused throughout the code base, but the overall intent and
existing flow seems fine. My only goal would be to make it a bit easier
for unfamiliar coders to pick up on the coding pattern and assumptions and
make coding errors there more obvious. Renaming and/or an assertion fits
that goal. Breaking the current abstraction level doesn't seem desirable.

David J.