Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)

Em ter., 26 de set. de 2023 às 09:30, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:

> Em ter., 26 de set. de 2023 às 07:34, Ashutosh Bapat <
> ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> escreveu:
>
>> On Tue, Sep 26, 2023 at 3:32 PM David Rowley <dgrowleyml(at)gmail(dot)com>
>> wrote:
>> >
>> > find_base_rel() could be made more robust for free by just casting the
>> > relid and simple_rel_array_size to uint32 while checking that relid <
>> > root->simple_rel_array_size. The 0th element should be NULL anyway,
>> > so "if (rel)" should let relid==0 calls through and allow that to
>> > ERROR still. I see that just changes a "jle" to "jnb" vs adding an
>> > additional jump for Ranier's version. [1]
>>
>> That's a good suggestion.
>>
>> I am fine with find_base_rel() as it is today as well. But
>> future-proofing it seems to be fine too.
>>
>> >
>> > It seems worth not making find_base_rel() more expensive than it is
>> > today as commonly we just reference root->simple_rel_array[n] directly
>> > anyway because it's cheaper. It would be nice if we didn't add further
>> > overhead to find_base_rel() as this would make the case for using
>> > PlannerInfo.simple_rel_array directly even stronger.
>>
>> I am curious, is the overhead in find_base_rel() impacting overall
>> performance?
>>
> It seems to me that it adds a LEA instruction.
> https://godbolt.org/z/b4jK3PErE
>
> Although it doesn't seem like much,
> I believe the solution (casting to unsigned) seems better.
> So +1.
>
As suggested, casting is the best option that does not add any overhead and
improves the robustness of the find_base_rel function.
I propose patch v1, with the additional addition of fixing the
find_base_rel_ignore_join function,
which despite not appearing in Coverity reports, suffers from the same
problem.

Taking advantage, I also propose a scope reduction,
as well as the const of the root parameter, which is very appropriate.

best regards,
Ranier Vilela