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

From Ranier Vilela
Subject Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)
Date
Msg-id CAEudQArQ_5ak=JihexAFN0SwqBZh_0xwsn6ReV_-hEtVw7+pVA@mail.gmail.com
Whole thread Raw
In response to Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)  (Ranier Vilela <ranier.vf@gmail.com>)
Responses Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)
List pgsql-hackers
Em ter., 26 de set. de 2023 às 09:30, Ranier Vilela <ranier.vf@gmail.com> escreveu:
Em ter., 26 de set. de 2023 às 07:34, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> escreveu:
On Tue, Sep 26, 2023 at 3:32 PM David Rowley <dgrowleyml@gmail.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.

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
Attachment

pgsql-hackers by date:

Previous
From: Jeff Davis
Date:
Subject: Re: Questions about the new subscription parameter: password_required
Next
From: "Karl O. Pinc"
Date:
Subject: Re: [PGdocs] fix description for handling pf non-ASCII characters