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

On Sun, Oct 11, 2020 at 6:27 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

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

Ok, but for me it's not an assertion, it's a higher-level check that the
variable that is expected to hold data on subsequent loops is, at the
beginning of the loop, uninitialized. TupIsUninitialized comes to mind as
better reflecting that fact.

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 don't have a problem with introducing a SlotEmpty macro, and agree that
when it is followed by "ExecCopySlot" it is an meaningful improvement (the
blurring of the lines between a slot and its pointed-to-tuple bothers me as
I get my first exposure this to code).

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

I'm willing to agree that the abstraction is broken even if the end result
of its use, in the existing codebase, hasn't resulted in any known bugs
(again, the null pointer dereferencing seems like it should be picked up
during routine testing). That said, there are multiple solutions here that
would improve matters in varying degrees each having a proportional effort
and risk profile in writing a patch (including the status-quo option).

For me, while I see room for improvement here, my total lack of actually
writing code using these interfaces means I defer to Tom Lane's final two
conclusions in his last email regarding how productive this line of work
would be. I also read that to mean if there was a complete and thorough
patch submitted it would be given a fair look. I would hope so since there
is a meaningful decision to make with regards to making changes purely to
benefit future inexperienced coders. But it seems like worthy material for
an inexperienced coder to compile and present and having the experienced
coders evaluate and critique, as Stephen Frost's post seemed to allude to.

David J.