#29805 closed defect (fixed)
41 coverity defects on prob_distr.c
Reported by: | asn | Owned by: | |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.4.1.x-final |
Component: | Core Tor/Tor | Version: | Tor: 0.4.0.1-alpha |
Severity: | Normal | Keywords: | prob-distr coverity 040-no-backport |
Cc: | catalyst | Actual Points: | 0.4 |
Parent ID: | Points: | 0.4 | |
Reviewer: | catalyst | Sponsor: |
Description
The DIST_BASE_TYPED
macro in prob_distr.h
is causing us 41 new coverity defects. I don't think it's wrong but it's quite hacky so we should fix it in some way.
*** CID 1444029: Incorrect expression (SIZEOF_MISMATCH) /src/lib/math/prob_distr.c: 1511 in log_logistic_sf() 1505 return cdf_log_logistic(x, LL->alpha, LL->beta); 1506 } 1507 1508 static double 1509 log_logistic_sf(const struct dist *dist, double x) 1510 { >>> CID 1444029: Incorrect expression (SIZEOF_MISMATCH) >>> Adding "0UL /* 0 * sizeof (dist - &((struct log_logistic const *)((char const *)dist - __builtin_offsetof()))->base) */" to pointer >>> "(struct log_logistic const *)((char const *)dist - 0UL)" of type "struct log_logistic const *" is suspicious because adding an integral value >>> to this pointer automatically scales that value by the size, 24 bytes, of the pointed-to type, "struct log_logistic const". Most likely, the >>> multiplication by "sizeof (dist - &((struct log_logistic const *)((char const *)dist - 0UL))->base)" in this expression is extraneous and >>> should be eliminated. 1511 const struct log_logistic *LL = const_container_of(dist, 1512 struct log_logistic, base); 1513 1514 return sf_log_logistic(x, LL->alpha, LL->beta); 1515 } 1516
Child Tickets
Attachments (1)
Change History (17)
comment:1 Changed 9 months ago by
Cc: | catalyst added |
---|
comment:2 Changed 9 months ago by
Summarizing some analysis that asn and I did on IRC, plus some more comments from me:
For the DIST_BASE_TYPED
cases (not quoted here?), Coverity is apparently complaining about the multiplication of an integer by the result of a sizeof
operator, which I think is possibly a helpful warning where the integer is nonzero.
This appears to be a type-checking hack that functions by making a compile-time assertion. The type checking works by subtracting &(OBJ)
and the same pointer cast to const TYPE *
, which is a C constraint violation if the types aren't the same. At a minimum, I think we should move the type-checking hack to a helper macro, and document everything better in comments.
Maybe directly adding the result of the pointer subtraction (which should always be zero) will be enough to quiet Coverity. Discarding the subtraction result by using a comma operator probably also works (if the macro isn't used in a static initializer).
comment:3 Changed 9 months ago by
You correctly ascertained that the goal of this apparently sketchy business is to check that in container_of(P, T, F)
, the type T
has a field F
of the same type as *P
. I don't think there's any undefined behaviour here.
The type check is already factored into a separate macro, precisely because of this issue with Coverity. In NetBSD we have the following #ifdef __COVERITY__
to suppress this noise: https://nxr.netbsd.org/xref/src/sys/lib/libkern/libkern.h#339
#ifdef __COVERITY___ #define validate_container_of(P, T, F) 0 #else #define validate_container_of(P, T, F) ...as before... #endif
We considered various other workarounds but, e.g., discarding the result with a comma led to non-Coverity compiler warnings, so we settled on an #ifdef.
comment:4 Changed 9 months ago by
The same technique -- say, a VALIDATE_DIST_BASE
macro -- would work for DIST_BASE_TYPED
. I didn't do that because I was thinking about probability distributions, not Coverity; it would be entirely reasonable to copy the mechanism.
comment:5 Changed 9 months ago by
Status: | new → needs_review |
---|
Thanks for all the help catalyst and riastradh.
Here is a branch that splits the type-checking part of the macro into its own macro:
https://github.com/torproject/tor/pull/825
Let's see if CI passes!
Changed 9 months ago by
Attachment: | dist-types.patch added |
---|
patch to comment on dist type macros and placate coverity
comment:6 follow-up: 7 Changed 9 months ago by
FYI, I sent the attached patch (dist-types.patch) to catalyst on IRC the other day, but forgot about it before I had a moment to put it on trac. Sorry it overlaps a little with the one you wrote, but it also adds some explanatory comments about what's going on and the intended usage.
comment:7 follow-up: 9 Changed 9 months ago by
Replying to riastradh:
FYI, I sent the attached patch (dist-types.patch) to catalyst on IRC the other day, but forgot about it before I had a moment to put it on trac. Sorry it overlaps a little with the one you wrote, but it also adds some explanatory comments about what's going on and the intended usage.
Thanks for the patch Riastradh.
I combined it along with my own patch (which splits the type-checking into its own macro) and pushed another commit to my PR above.
Ticket remains in needs_review.
comment:8 Changed 9 months ago by
Reviewer: | → catalyst |
---|
comment:9 follow-up: 10 Changed 9 months ago by
Status: | needs_review → merge_ready |
---|
Replying to asn:
I combined it along with my own patch (which splits the type-checking into its own macro) and pushed another commit to my PR above.
Ticket remains in needs_review.
Thanks! It looks good. I suggested a small comment improvement on the pull request, which can probably be done without further review.
comment:10 Changed 9 months ago by
Replying to catalyst:
Replying to asn:
I combined it along with my own patch (which splits the type-checking into its own macro) and pushed another commit to my PR above.
Ticket remains in needs_review.
Thanks! It looks good. I suggested a small comment improvement on the pull request, which can probably be done without further review.
Pushed squash with the added comment. Thanks!
comment:11 follow-up: 12 Changed 9 months ago by
Hi! This LGTM but I get some conflicts when I go to squash and merge this -- could I please have a squashed branch?
comment:12 Changed 9 months ago by
Replying to nickm:
Hi! This LGTM but I get some conflicts when I go to squash and merge this -- could I please have a squashed branch?
Please see squashed and rebased branch here: https://github.com/torproject/tor/pull/845
Does this work better for you?
comment:13 follow-up: 14 Changed 8 months ago by
Keywords: | 040-backport-maybe added |
---|---|
Status: | merge_ready → needs_revision |
Version: | → Tor: 0.4.0.1-alpha |
asn:
- please remember to fill in points and actual points on the ticket
- please add a changes file for bugs on 0.4.0 and earlier
This is a minor refactor. There are lots of comments, but only 7 changed code lines.
Do you think we should backport it to maint-0.4.0?
comment:14 Changed 8 months ago by
Actual Points: | → 0.4 |
---|---|
Points: | → 0.4 |
Status: | needs_revision → merge_ready |
Replying to teor:
asn:
- please remember to fill in points and actual points on the ticket
- please add a changes file for bugs on 0.4.0 and earlier
This is a minor refactor. There are lots of comments, but only 7 changed code lines.
Do you think we should backport it to maint-0.4.0?
Pushed a changes file to the bug29805_rebased
!
Does coverity scan maint-0.4.0? If not, I would not backport this. If yes, perhaps we should consider it, given that the patch is simple.
comment:15 Changed 8 months ago by
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
ok, squashed and merged to 0.4.1, with no backport.
comment:16 Changed 6 months ago by
Keywords: | 040-no-backport added; 040-backport-maybe removed |
---|
Replying to asn:
It also seems dubious to me and I would need to dig to decide whether I think it's undefined behavior.