Opened 5 months ago

Closed 5 months ago

Last modified 2 months ago

#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)

dist-types.patch (3.5 KB) - added by riastradh 5 months ago.
patch to comment on dist type macros and placate coverity

Download all attachments as: .zip

Change History (17)

comment:1 in reply to:  description Changed 5 months ago by catalyst

Cc: catalyst added

Replying to asn:

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.

It also seems dubious to me and I would need to dig to decide whether I think it's undefined behavior.

comment:2 Changed 5 months ago by catalyst

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 5 months ago by riastradh

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 5 months ago by riastradh

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 5 months ago by asn

Status: newneeds_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 5 months ago by riastradh

Attachment: dist-types.patch added

patch to comment on dist type macros and placate coverity

comment:6 Changed 5 months ago by 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.

comment:7 in reply to:  6 ; Changed 5 months ago by asn

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 5 months ago by asn

Reviewer: catalyst

comment:9 in reply to:  7 ; Changed 5 months ago by catalyst

Status: needs_reviewmerge_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 in reply to:  9 Changed 5 months ago by asn

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 Changed 5 months ago by nickm

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 in reply to:  11 Changed 5 months ago by asn

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 Changed 5 months ago by teor

Keywords: 040-backport-maybe added
Status: merge_readyneeds_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 in reply to:  13 Changed 5 months ago by asn

Actual Points: 0.4
Points: 0.4
Status: needs_revisionmerge_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 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

ok, squashed and merged to 0.4.1, with no backport.

comment:16 Changed 2 months ago by teor

Keywords: 040-no-backport added; 040-backport-maybe removed
Note: See TracTickets for help on using tickets.