Thread: Re: New Defects reported by Coverity Scan for PostgreSQL
Hello guys. Coverity complained about this patch as below. What, if anything, should be done about it? One solution is to mark it as a false-positive in Coverity, of course. On 2018-Jul-29, scan-admin@coverity.com wrote: > ** CID 1438146: API usage errors (SWAPPED_ARGUMENTS) > > > ________________________________________________________________________________________________________ > *** CID 1438146: API usage errors (SWAPPED_ARGUMENTS) > /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/geo_ops.c: 2647 in close_lseg() > 2641 LSEG *l1 = PG_GETARG_LSEG_P(0); > 2642 LSEG *l2 = PG_GETARG_LSEG_P(1); > 2643 Point *result; > 2644 > 2645 result = (Point *) palloc(sizeof(Point)); > 2646 > >>> CID 1438146: API usage errors (SWAPPED_ARGUMENTS) > >>> The positions of arguments in the call to "lseg_closept_lseg" do not match the ordering of the parameters: > * "l2" is passed to "l1" > * "l1" is passed to "l2" > 2647 lseg_closept_lseg(result, l2, l1); > 2648 > 2649 PG_RETURN_POINT_P(result); > 2650 } > 2651 > 2652 -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
To make coverity happy we might be able to suppress this false alarm by adding a line like below:
```
/* coverity[SWAPPED_ARGUMENTS] */
lseg_closept_lseg(result, l2, l1);
```
From my point of view it's better to also put some comments for humans to understand why we are passing l1 and l2 in reverse order.
On Wed, Aug 1, 2018 at 1:21 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Hello guys. Coverity complained about this patch as below. What, if
anything, should be done about it? One solution is to mark it as a
false-positive in Coverity, of course.
On 2018-Jul-29, scan-admin@coverity.com wrote:
> ** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
>
>
> ____________________________________________________________ ______________________________ ______________
> *** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/ adt/geo_ops.c: 2647 in close_lseg()
> 2641 LSEG *l1 = PG_GETARG_LSEG_P(0);
> 2642 LSEG *l2 = PG_GETARG_LSEG_P(1);
> 2643 Point *result;
> 2644
> 2645 result = (Point *) palloc(sizeof(Point));
> 2646
> >>> CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> >>> The positions of arguments in the call to "lseg_closept_lseg" do not match the ordering of the parameters:
> * "l2" is passed to "l1"
> * "l1" is passed to "l2"
> 2647 lseg_closept_lseg(result, l2, l1);
> 2648
> 2649 PG_RETURN_POINT_P(result);
> 2650 }
> 2651
> 2652
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Ning Yu <nyu@pivotal.io> writes:
> From my point of view it's better to also put some comments for humans to
> understand why we are passing l1 and l2 in reverse order.
The header comment for lseg_closept_lseg() is pretty far from adequate
as well. After studying it for awhile I think I've puzzled out the
indeterminacies, but I shouldn't have had to. I think it probably
should have read
* Closest point on line segment l2 to line segment l1
*
* This returns the minimum distance from l1 to the closest point on l2.
* If result is not NULL, the closest point on l2 is stored at *result.
Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
that description. But the mere fact that there is any question about
that means that the function is poorly documented and perhaps poorly
named as well. For that matter, is there a good reason why l1/l2
have those roles and not the reverse?
While Coverity is surely operating from a shaky heuristic here, it's
got a point. The fact that it makes a difference which argument is
given first means that you've got to be really careful about which
is which, and this API spec is giving no help in that regard at all.
regards, tom lane
On 08/01/2018 04:22 AM, Tom Lane wrote: > Ning Yu <nyu@pivotal.io> writes: >> From my point of view it's better to also put some comments for humans to >> understand why we are passing l1 and l2 in reverse order. > > The header comment for lseg_closept_lseg() is pretty far from adequate > as well. After studying it for awhile I think I've puzzled out the > indeterminacies, but I shouldn't have had to. I think it probably > should have read > > * Closest point on line segment l2 to line segment l1 > * > * This returns the minimum distance from l1 to the closest point on l2. > * If result is not NULL, the closest point on l2 is stored at *result. > > Or perhaps I have it backwards and "l1" and "l2" need to be swapped in > that description. But the mere fact that there is any question about > that means that the function is poorly documented and perhaps poorly > named as well. For that matter, is there a good reason why l1/l2 > have those roles and not the reverse? > > While Coverity is surely operating from a shaky heuristic here, it's > got a point. The fact that it makes a difference which argument is > given first means that you've got to be really careful about which > is which, and this API spec is giving no help in that regard at all. > Yeah, I agree. The comments don't make it very clear what is the API semantics. Will fix. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in > that description. But the mere fact that there is any question about > that means that the function is poorly documented and perhaps poorly > named as well. For that matter, is there a good reason why l1/l2 > have those roles and not the reverse? Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1, yyy *l2) functions in a way that they find the find the point on "l1".
On 08/01/2018 11:55 AM, Emre Hasegeli wrote: >> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in >> that description. But the mere fact that there is any question about >> that means that the function is poorly documented and perhaps poorly >> named as well. For that matter, is there a good reason why l1/l2 >> have those roles and not the reverse? > > Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1, > yyy *l2) functions in a way that they find the find the point on "l1". > IMHO the main issue here is that the rule is not obvious / documented anywhere. I think the best way to do that is by making it clear in a comment for each such such function. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On 08/01/2018 11:55 AM, Emre Hasegeli wrote:
>> Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1,
>> yyy *l2) functions in a way that they find the find the point on "l1".
> IMHO the main issue here is that the rule is not obvious / documented
> anywhere. I think the best way to do that is by making it clear in a
> comment for each such such function.
I think there are three different things that need to be addressed:
* Underspecified comments.
* The function names and argument names are badly chosen IMO, because even
granted a convention such as the above, it's not very obvious what roles
"l1" and "l2" play. I'm not exactly sure what would be better, but if you
used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
with an asymmetrical function name too, to make miswriting of calls less
likely.
* And lastly, are we sure there aren't actual *bugs* here? I'd initially
supposed that lseg_closept_lseg acted as Emre says above, but reading the
code makes me think it's the other way around. Its first two potential
assignments to *result are definitely assigning points on l2 not l1.
regards, tom lane
> I think there are three different things that need to be addressed: > > * Underspecified comments. I agree. My patch added comments, the next one with actual fixes adds more. I just didn't want to invest even more time on them while the code is its current shape. > * The function names and argument names are badly chosen IMO, because even > granted a convention such as the above, it's not very obvious what roles > "l1" and "l2" play. I'm not exactly sure what would be better, but if you > used names like "ofseg" and "otherseg" you'd at least be trying. I'd go > with an asymmetrical function name too, to make miswriting of calls less > likely. Good idea. I haven't though about that, but now such names makes more sense to me. I will prepare another patch to improve the naming and comments to be applied on top of my current patches. > * And lastly, are we sure there aren't actual *bugs* here? I'd initially > supposed that lseg_closept_lseg acted as Emre says above, but reading the > code makes me think it's the other way around. Its first two potential > assignments to *result are definitely assigning points on l2 not l1. Yes, it is wrong beyond understanding. The committed patch intended to keep answers as they were. The next one actually fixes those.
On 08/01/2018 04:07 PM, Emre Hasegeli wrote: >> I think there are three different things that need to be addressed: >> >> * Underspecified comments. > > I agree. My patch added comments, the next one with actual fixes adds > more. I just didn't want to invest even more time on them while the > code is its current shape. > >> * The function names and argument names are badly chosen IMO, because even >> granted a convention such as the above, it's not very obvious what roles >> "l1" and "l2" play. I'm not exactly sure what would be better, but if you >> used names like "ofseg" and "otherseg" you'd at least be trying. I'd go >> with an asymmetrical function name too, to make miswriting of calls less >> likely. > > Good idea. I haven't though about that, but now such names makes more > sense to me. I will prepare another patch to improve the naming and > comments to be applied on top of my current patches. > >> * And lastly, are we sure there aren't actual *bugs* here? I'd initially >> supposed that lseg_closept_lseg acted as Emre says above, but reading the >> code makes me think it's the other way around. Its first two potential >> assignments to *result are definitely assigning points on l2 not l1. > > Yes, it is wrong beyond understanding. The committed patch intended > to keep answers as they were. The next one actually fixes those. > OK, so I think we should not do anything about the issues reported by coverity until we commit all the patches. Which may resolve some of those (pre-existing) issues, for example. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> Good idea. I haven't though about that, but now such names makes more > sense to me. I will prepare another patch to improve the naming and > comments to be applied on top of my current patches. The patch is attached to improve the comments and variable names. I explained the functions with the same signature on the file header. I can duplicate those on the function headers if you find that better.
Attachment
On 2018-Nov-06, Emre Hasegeli wrote: > The patch is attached to improve the comments and variable names. I > explained the functions with the same signature on the file header. I > can duplicate those on the function headers if you find that better. Surely the comment in line 3839 deserves an update :-) This seems good material. I would put the detailed conventions comment separately from the head of the file, like this (where I also changed "Type1 *type1" into "Type1 *obj1", and a few "has" to "have") Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
> Surely the comment in line 3839 deserves an update :-) Done. > This seems good material. I would put the detailed conventions comment > separately from the head of the file, like this (where I also changed > "Type1 *type1" into "Type1 *obj1", and a few "has" to "have") Looks better to me. I found one more "has" and changed it.
Attachment
On 2018-Nov-06, Emre Hasegeli wrote: > > Surely the comment in line 3839 deserves an update :-) > > Done. > > > This seems good material. I would put the detailed conventions comment > > separately from the head of the file, like this (where I also changed > > "Type1 *type1" into "Type1 *obj1", and a few "has" to "have") > > Looks better to me. I found one more "has" and changed it. Pushed, with some further minor changes. I decided not to remove the redundant comments that your patch was removing, as I felt that it's better to keep the API contract together with the function definition. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services