Thread: Possible api miuse bms_next_member
Hi.
Per Coverity.
CID 1608872: (#1 of 1): Improper use of negative value (NEGATIVE_RETURNS)
32. negative_returns: bms_next_member(child_joinrel->relids, -1) is passed to a parameter that cannot be negative.[show details] CID 1608871: (#1 of 1): Out-of-bounds access (OVERRUN)
32. overrun-buffer-arg: Calling add_child_eq_member with cur_ec->ec_childmembers and bms_next_member(child_joinrel->relids, -1) is suspicious because of the very large index, 4294967294. The index may be due to a negative parameter being interpreted as unsigned.Coverity has two new reports about use of the function *bms_next_member*.
I think that he is right.
The function bms_next_member can return NEGATIVE.
So it is necessary to validate the function's return.
Attached has three patchs.
1. src/backend/optimizer/path/equivclass.c
Source of the Coverity report.
Function: add_child_join_rel_equivalences
Check the return of bms_next_member and avoid
continue if return is negative.
2. src/backend/partitioning/partprune.c
Function: make_partition_pruneinfo
Check the return of bms_next_member and avoid look if
targetpart if not found.
3. contrib/postgres_fdw/postgres_fdw.c
Function: postgresBeginForeignScan
Check the return of bms_next_member and abort if fail.
Function: postgresExplainForeignScan
Check the return of bms_next_member and abort if fail.
best regards,
Ranier Vilela
Attachment
On Wed, 9 Apr 2025 at 15:01, Ranier Vilela <ranier.vf@gmail.com> wrote: > > Hi. > > Per Coverity. > > CID 1608872: (#1 of 1): Improper use of negative value (NEGATIVE_RETURNS) > 32. negative_returns: bms_next_member(child_joinrel->relids, -1) is passed to a parameter that cannot be negative.[showdetails] > > CID 1608871: (#1 of 1): Out-of-bounds access (OVERRUN) > 32. overrun-buffer-arg: Calling add_child_eq_member with cur_ec->ec_childmembers and bms_next_member(child_joinrel->relids,-1) is suspicious because of the very large index, 4294967294. The index may be dueto a negative parameter being interpreted as unsigned. > > Coverity has two new reports about use of the function *bms_next_member*. > I think that he is right. > > The function bms_next_member can return NEGATIVE. > So it is necessary to validate the function's return. I don't know much about the planner, but I would expect a RelOptInfo's relids field to always contain at least one relid when it's not currently being constructed; thus guaranteeing a non-negative result when looking for the first bit (as indicated by "next bit after -1"). Or did I miss something? Kind regards, Matthias van de Meent Neon (https://neon.tech)
Em qua., 9 de abr. de 2025 às 10:27, Matthias van de Meent <boekewurm+postgres@gmail.com> escreveu:
On Wed, 9 Apr 2025 at 15:01, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi.
>
> Per Coverity.
>
> CID 1608872: (#1 of 1): Improper use of negative value (NEGATIVE_RETURNS)
> 32. negative_returns: bms_next_member(child_joinrel->relids, -1) is passed to a parameter that cannot be negative.[show details]
>
> CID 1608871: (#1 of 1): Out-of-bounds access (OVERRUN)
> 32. overrun-buffer-arg: Calling add_child_eq_member with cur_ec->ec_childmembers and bms_next_member(child_joinrel->relids, -1) is suspicious because of the very large index, 4294967294. The index may be due to a negative parameter being interpreted as unsigned.
>
> Coverity has two new reports about use of the function *bms_next_member*.
> I think that he is right.
>
> The function bms_next_member can return NEGATIVE.
> So it is necessary to validate the function's return.
I don't know much about the planner, but I would expect a RelOptInfo's
relids field to always contain at least one relid when it's not
currently being constructed; thus guaranteeing a non-negative result
when looking for the first bit (as indicated by "next bit after -1").
I think it is worth the effort to prevent this.
In this particular case, the function *add_child_join_rel_equivalences*,
has the following comment:
"Note that this function won't be called at all unless we have at least some
* reason to believe that the EC members it generates will be useful."
* reason to believe that the EC members it generates will be useful."
So I believe the function is not critical.
best regards,
Ranier Vilela
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> On Wed, 9 Apr 2025 at 15:01, Ranier Vilela <ranier.vf@gmail.com> wrote:
>> Coverity has two new reports about use of the function *bms_next_member*.
>> I think that he is right.
> I don't know much about the planner, but I would expect a RelOptInfo's
> relids field to always contain at least one relid when it's not
> currently being constructed; thus guaranteeing a non-negative result
> when looking for the first bit (as indicated by "next bit after -1").
Indeed. These patches are all useless IMO. I'm not sure that this
is really a great way to select a representative member of the
child-relids set, but it won't fail, and if it did we'd have way
worse problems. (For starters: if a child rel's relids set is
empty, then presumably so was the parent's, meaning we cannot
tell them apart.)
If we did want to do something about this warning, rather than
hacking up the call sites I'd be inclined to invent something like
"bms_first_member()" which does the same thing but additionally
asserts on empty input. Not really convinced it's worth the
trouble though.
regards, tom lane
On Thu, 10 Apr 2025 at 02:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If we did want to do something about this warning, rather than > hacking up the call sites I'd be inclined to invent something like > "bms_first_member()" which does the same thing but additionally > asserts on empty input. Not really convinced it's worth the > trouble though. Aha, a reincarnation! (462bb7f12). My vote too is to do nothing. David