From: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
---|---|
To: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c) |
Date: | 2023-09-26 05:41:51 |
Message-ID: | CAExHW5vdSVnsp98qHP4Np085-y223X3Rhdgyf9=Vp_LKFkS-tQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Sep 25, 2023 at 7:14 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> Em seg., 25 de set. de 2023 às 08:23, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> escreveu:
>>
>> On Sat, Sep 23, 2023 at 7:29 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)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 to 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 in DEBUG mode.
>> >
>> > But this does not change the fact that bms_singleton_member can return a negative value, which may occur on some production servers.
>> >
>> > 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 in production 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
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-09-26 05:46:55 | RE: pg_upgrade and logical replication |
Previous Message | vignesh C | 2023-09-26 05:28:12 | Re: pg_upgrade and logical replication |