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 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?
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?
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.
best regards,
Ranier Vilela