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

Em dom., 11 de out. de 2020 às 14:53, David G. Johnston <
david(dot)g(dot)johnston(at)gmail(dot)com> escreveu:

> 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.
>
Ok. I will try to explain.

1. TupIsNull in fact it should be called: TupIsNullOrEmpty
2. Only Rename TupIsNull to example TupIsNullOrEmpty, improves, but it is
not the complete solution.
3. The combination:
if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);
for me it acts partly as if it were an assertion, but at runtime.
If node->group_pivot is NULL, ExecCopySlot crashes, like an assertion.
4. As it has been running for a while, without any complaints, probably the
callers have already guaranteed that node-> group_pivot is not NULL
5. We can remove the first part of the macro and rename: TupIsNull to
SlotEmpty
6. With SlotEmpty macro, each TupIsNull needs to be carefully changed.
if (SlotEmpty(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

> 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.
>
If only rename TupIsNull to TupIsNullOrEmpty:

1. Why continue testing a pointer against NULL and call ExecCopySlot and
crash at runtime.
2. Most likely, the pointer is not NULL, since it has already been well
tested.
3. The only thing that can be done, after TupIsNullOrEmpty, is return or
fail, anything else needs to be tested again.

I think that current abstraction is broken.

regards,
Ranier Vilela