Обсуждение: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)

Hi,

On Sat, Sep 23, 2023 at 9:59 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Per Coverity.
> CID 1518088 (#2 of 2): Improper use of negative value (NEGATIVE_RETURNS)
>
> The function bms_singleton_member can returns a negative number.
>
> /*
> * Get a child rel for rel2 with the relids.  See above comments.
> */
> if (rel2_is_simple)
> {
> int varno = bms_singleton_member(child_relids2);
>
> child_rel2 = find_base_rel(root, varno);
> }
>
> It turns out that in the get_matching_part_pairs function (joinrels.c), the return of bms_singleton_member is passed
tothe find_base_rel function, which cannot receive a negative value. 
>
> find_base_rel is protected by an Assertion, which effectively indicates that the error does not occur in tests and in
DEBUGmode. 
>
> But this does not change the fact that bms_singleton_member can return a negative value, which may occur on some
productionservers. 
>
> Fix by changing the Assertion into a real test, to protect the simple_rel_array array.

Thanks for the report and patch!  I will review the patch.

Best regards,
Etsuro Fujita

On Sat, Sep 23, 2023 at 7:29 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi,
>
> Per Coverity.
> CID 1518088 (#2 of 2): Improper use of negative value (NEGATIVE_RETURNS)
>
> The function bms_singleton_member can returns a negative number.
>
> /*
> * Get a child rel for rel2 with the relids.  See above comments.
> */
> if (rel2_is_simple)
> {
> int varno = bms_singleton_member(child_relids2);
>
> child_rel2 = find_base_rel(root, varno);
> }
>
> It turns out that in the get_matching_part_pairs function (joinrels.c), the return of bms_singleton_member is passed
tothe find_base_rel function, which cannot receive a negative value. 
>
> find_base_rel is protected by an Assertion, which effectively indicates that the error does not occur in tests and in
DEBUGmode. 
>
> But this does not change the fact that bms_singleton_member can return a negative value, which may occur on some
productionservers. 
>
> Fix by changing the Assertion into a real test, to protect the simple_rel_array array.

Do you have a scenario where bms_singleton_member() actually returns a
-ve number OR it's just a possibility. bms_make_singleton() has an
assertion at the end: Assert(result >= 0);
bms_make_singleton(), bms_add_member() explicitly forbids negative
values. It looks like we have covered all the places which can add a
negative value to a bitmapset. May be we are missing a place or two.
It will be good to investigate it.

What you are suggesting may be ok, as is as well.

--
Best Wishes,
Ashutosh Bapat

On Mon, Sep 25, 2023 at 7:14 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em seg., 25 de set. de 2023 às 08:23, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> escreveu:
>>
>> On Sat, Sep 23, 2023 at 7:29 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>> >
>> > Hi,
>> >
>> > Per Coverity.
>> > CID 1518088 (#2 of 2): Improper use of negative value (NEGATIVE_RETURNS)
>> >
>> > The function bms_singleton_member can returns a negative number.
>> >
>> > /*
>> > * Get a child rel for rel2 with the relids.  See above comments.
>> > */
>> > if (rel2_is_simple)
>> > {
>> > int varno = bms_singleton_member(child_relids2);
>> >
>> > child_rel2 = find_base_rel(root, varno);
>> > }
>> >
>> > It turns out that in the get_matching_part_pairs function (joinrels.c), the return of bms_singleton_member is
passedto the find_base_rel function, which cannot receive a negative value. 
>> >
>> > find_base_rel is protected by an Assertion, which effectively indicates that the error does not occur in tests and
inDEBUG mode. 
>> >
>> > But this does not change the fact that bms_singleton_member can return a negative value, which may occur on some
productionservers. 
>> >
>> > Fix by changing the Assertion into a real test, to protect the simple_rel_array array.
>>
>> Do you have a scenario where bms_singleton_member() actually returns a
>> -ve number OR it's just a possibility.
>
> Just a possibility.
>
>>
>> bms_make_singleton() has an
>> assertion at the end: Assert(result >= 0);
>> bms_make_singleton(), bms_add_member() explicitly forbids negative
>> values. It looks like we have covered all the places which can add a
>> negative value to a bitmapset. May be we are missing a place or two.
>> It will be good to investigate it.
>
> I try to do the research, mostly, with runtime compilation.
> As previously stated, the error does not appear in the tests.
> That said, although Assert protects in most cases, that doesn't mean it can't occur in a query, running on a server
inproduction mode. 
>
> Now thinking about what you said about Assertion in bms_make_singleton.
> I think it's nonsense, no?

Sorry, I didn't write it correctly. bms_make_singleton() doesn't
accept a negative integer and bms_get_singleton_member() and
bms_singleton_member() has assertion at the end. Since there is no
possibility of a negative integer making itself a part of bitmapset,
the two functions Asserting instead of elog'ing is better. Assert are
cheaper in production.

> Why design a function that in DEBUG mode prohibits negative returns, but in runtime mode allows it?
> After all, why allow a negative return, if for all practical purposes this is prohibited?

You haven't given any proof that there's a possibility that a negative
value may be returned. We are not allowing negative value being
returned at all.

> Regarding the find_base_rel function, it is nonsense to protect the array with Assertion.
> After all, we have already protected the upper limit with a real test, why not also protect the lower limit.
> The additional testing is cheap and makes perfect sense, making the function more robust in production mode.
> As an added bonus, modern compilers will probably be able to remove the additional test if it deems it not necessary.
> Furthermore, all protections that were added to protect find_base_real calls can eventually be removed,
> since find_base_real will accept parameters with negative values.

However, I agree that changing find_base_rel() the way you have done
in your patch is fine and mildly future-proof. +1 to that idea
irrespective of what bitmapset functions do.

--
Best Wishes,
Ashutosh Bapat

On Tue, 26 Sept 2023 at 21:45, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> However, I agree that changing find_base_rel() the way you have done
> in your patch is fine and mildly future-proof. +1 to that idea
> irrespective of what bitmapset functions do.

I'm not a fan of adding additional run-time overhead for this
theoretical problem.

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]

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.

David

[1] https://godbolt.org/z/qrxKTbvva

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?

--
Best Wishes,
Ashutosh Bapat

On Wed, 27 Sept 2023 at 01:31, Ranier Vilela <ranier.vf@gmail.com> wrote:
> It seems to me that it adds a LEA instruction.
> https://godbolt.org/z/b4jK3PErE

There's a fairly significant difference in the optimisability of a
comparison with a compile-time constant vs a variable. For example,
would you expect the compiler to emit assembly for each side of the
boolean AND in: if (a > 12 && a > 20),  or how about if (a > 12 && a >
y)?  No need to answer. Just consider it.

I suggest keeping your experiments as close to the target code as
practical. You might be surprised by what the compiler can optimise.

David

On Wed, 27 Sept 2023 at 06:10, Ranier Vilela <ranier.vf@gmail.com> wrote:
> As suggested, casting is the best option that does not add any overhead and improves the robustness of the
find_base_relfunction.
 
> 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?

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 */

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

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

David

[1] https://wiki.postgresql.org/wiki/Submitting_a_Patch

At 2023-09-27 10:37:45 -0300, ranier.vf@gmail.com wrote:
>
> Forgive my impulsiveness, anyone who loves perfect, well written code,
> would understand.

I actually find this characterisation offensive.

Being scrupulous about not grouping random drive-by changes together
with the primary change is right up there in importance with writing
good commit messages to help future readers to reduce their WTFs/minute
score one, five, seven, or ten years later.

Ignoring that concern once is thoughtless. Ignoring it over and over
again is disrespectful. Casually deriding it by equating it to hating
"perfect, well-written code" is gross.

-- Abhijit

On Thu, 28 Sept 2023 at 02:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
>> 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.

Perhaps, but the committers on this project seem to be against the
blunderbuss approach to committing patches.  You might meet less
resistance around here if you assume all of their weapons have a scope
and that they like to aim for something before pulling the trigger.

Personally, this seems like a good idea to me and I'd like to follow
it too.  If you'd like to hold off you a committer whose weapon of
choice is the blunderbuss then I can back off and let you wait for one
to come along. Just let me know.

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

Well, the topic seems to be adding additional safety to prevent
accessing negative entries for simple_rel_array.  I can't think why
fixing the same theoretical hazard in find_base_rel_ignore_join()
would be unrelated.  I hope you can see the difference here. Randomly
adjusting function signatures because you happen to be changing some
code within that function does not, in my book, seem related.  You
seem to agree with this given you mentioned "Of course that has
nothing to do with it."

David

On Fri, 29 Sept 2023 at 01:00, Ranier Vilela <ranier.vf@gmail.com> wrote:
> Perhaps, and using your own words, the leaders on this project seem
> to be against reviewers armed with blunderbuss, too.

I don't have any ideas on what you're talking about here, but if this
is a valid concern that you think is unfair then maybe the code of
conduct team is a more relevant set of people to raise it with.

My mention of the scattergun approach to writing patches was aimed at
helping you have more success here.  I'd recommend you resist the urge
to change unrelated code in your patches. Your experience here might
improve if you're able to aim your patches at resolving specific
issues.  I know you'd love to see commit messages that include "In
passing, adjust a random assortment of changes contributed by Ranier
Vilela".  That's just not going to happen. You might think the
committer's job is just to commit patches contributed by contributors,
but what you might not realise is that we're also trying to maintain a
codebase that's over 3 decades old and will probably outlive most of
its current contributors.  Making things easy both for ourselves in
several years time and for our future counterparts is something that
we do need to consider when discussing and making changes.  The mail
archives and commit messages are an audit trail for our future selves
and future counterparts.  That's one of the main reasons why we tend
to not like you trying to sneak in assortments of random changes along
with your change. If you can understand this and adapt to this way of
thinking then you're more likely to find yourself feeling like you're
working with committers and other contributors rather than against
them.

I say this hoping you take it constructively, but I find that you're
often very defensive about your patches and often appear to take
change requests in a very negative way.  On this thread you've
demonstrated to me that by me requesting you to remove an unrelated
change in the patch that I must not care about code quality in
PostgreSQL.  I personally find these sorts of responses quite draining
and it makes me not want to touch your work. I would imagine I'm not
the only person that feels this. So there's a real danger here that if
you continue to have too many negative responses in emails, you'll
find yourself ignored and you're likely to find that frustrating and
that will lead to you having a worse experience here.   This does not
mean you have to become passive to all requests for changes to your
patches, but if you're going to argue or resist, then it pays to
politely explain your reasoning so that the topic can be discussed
constructively.

> I confess that some "in pass", "while there", and some desire to enrich the patch, clouded my judgment.

I'm glad to see you're keen to make improvements to PostgreSQL,
however, I'd like to suggest that you channel that energy and aim to
produce patches targeted in those areas that attempt to resolve a
series or perhaps all problems of that particular category in
isolation.  If it's a large patch then it's likely best to get
consensus first as having lots of work rejected is always more
painful.

> So it seems that we have some consensus, I propose version 2 of the patch,
> which I hope we will have to correct, perhaps, the words of the comments.

I've pushed this patch after making an adjustment to the comment to
shorten it to a single line.

Thank you for working on this.

David