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

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