41 coverity defects on prob_distr.c

[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.

*** 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     

[catalyst]

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.

[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).

[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.

[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.

[asn]

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!

[riastradh]

patch to comment on dist type macros and placate coverity

[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.

[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.

[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.

[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!

[nickm]

Hi! This LGTM but I get some conflicts when I go to squash and merge this -- could I please have a squashed branch?

[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?

[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?

[asn]

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.

[nickm]

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