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.
The patchs are attempts, not definitive fixes.
best regards,
Ranier Vilela
Attachments:
avoid-miuse-api-bms_next_member-equivclass.patchapplication/octet-stream; name=avoid-miuse-api-bms_next_member-equivclass.patchDownload
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 441f12f6c5..21c4ce27e9 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -3001,6 +3001,11 @@ add_child_join_rel_equivalences(PlannerInfo *root,
/* Yes, generate transformed child version */
Expr *child_expr;
Relids new_relids;
+ int i;
+
+ i = bms_next_member(child_joinrel->relids, -1);
+ if (i < 0)
+ continue;
if (parent_joinrel->reloptkind == RELOPT_JOINREL)
{
@@ -3059,7 +3064,7 @@ add_child_join_rel_equivalences(PlannerInfo *root,
cur_em->em_jdomain,
cur_em,
cur_em->em_datatype,
- bms_next_member(child_joinrel->relids, -1));
+ i);
}
}
}
avoid-possible-overflow-partprune.patchapplication/octet-stream; name=avoid-possible-overflow-partprune.patchDownload
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 48a35f763e..9ff2ace706 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -398,25 +398,27 @@ make_partition_pruneinfo(PlannerInfo *root, RelOptInfo *parentrel,
static List *
add_part_relids(List *allpartrelids, Bitmapset *partrelids)
{
- Index targetpart;
- ListCell *lc;
+ int targetpart;
/* We can easily get the lowest set bit this way: */
targetpart = bms_next_member(partrelids, -1);
- Assert(targetpart > 0);
-
- /* Look for a matching topmost parent */
- foreach(lc, allpartrelids)
+ if (targetpart > 0)
{
- Bitmapset *currpartrelids = (Bitmapset *) lfirst(lc);
- Index currtarget = bms_next_member(currpartrelids, -1);
+ ListCell *lc;
- if (targetpart == currtarget)
+ /* Look for a matching topmost parent */
+ foreach(lc, allpartrelids)
{
- /* Found a match, so add any new RT indexes to this hierarchy */
- currpartrelids = bms_add_members(currpartrelids, partrelids);
- lfirst(lc) = currpartrelids;
- return allpartrelids;
+ Bitmapset *currpartrelids = (Bitmapset *) lfirst(lc);
+ int currtarget = bms_next_member(currpartrelids, -1);
+
+ if (targetpart == currtarget)
+ {
+ /* Found a match, so add any new RT indexes to this hierarchy */
+ currpartrelids = bms_add_members(currpartrelids, partrelids);
+ lfirst(lc) = currpartrelids;
+ return allpartrelids;
+ }
}
}
/* No match, so add the new partition hierarchy to the list */
check-possible-fail-bms_next_member-postgres_fdw.patchapplication/octet-stream; name=check-possible-fail-bms_next_member-postgres_fdw.patchDownload
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index a7e0cc9f32..2ca37e70d9 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1528,7 +1528,11 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
if (fsplan->scan.scanrelid > 0)
rtindex = fsplan->scan.scanrelid;
else
+ {
rtindex = bms_next_member(fsplan->fs_base_relids, -1);
+ if (rtindex < 0)
+ elog(ERROR, "could not find index relation");
+ }
rte = exec_rt_fetch(rtindex, estate);
/* Get info about foreign table. */
@@ -2877,6 +2881,8 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
ptr++;
}
rtoffset = bms_next_member(plan->fs_base_relids, -1) - minrti;
+ if (rtoffset < 0)
+ elog(ERROR, "could not find offset relation");
/* Now we can translate the string */
relations = makeStringInfo();
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").
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) ispassed 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 withcur_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."
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