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

Em qua., 27 de set. de 2023 às 04:35, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> On Wed, 27 Sept 2023 at 06:10, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> > 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.
>
> Can you confirm that this silences the Converity warning?
>
CID#1518088
This is a historical version of the file displaying the issue before it was
in the Fixed state.

> I think it probably warrants a comment to mention why we cast to uint32.
>
> e.g. /* perform an unsigned comparison so that we also catch negative
> relid values */
>
I'm ok.

>
> > Taking advantage, I also propose a scope reduction,
> > as well as the const of the root parameter, which is very appropriate.
>
> Can you explain why adding the const qualifier is "very appropriate"
> to catching negative relids?
>
Of course that has nothing to do with it.

> Please check [1] for the mention of:
>
> "The fastest way to get your patch rejected is to make unrelated
> changes. Reformatting lines that haven't changed, changing unrelated
> comments you felt were poorly worded, touching code not necessary to
> your change, etc. Each patch should have the minimum set of changes
> required to work robustly. If you do not follow the code formatting
> suggestions above, expect your patch to be returned to you with the
> feedback of "follow the code conventions", quite likely without any
> other review."
>
Forgive my impulsiveness, anyone who loves perfect, well written code,
would understand.

Do you have an objection to fixing the function find_base_rel_ignore_join?
Or is it included in unrelated changes?

Ranier Vilela