Opened 6 months ago

Closed 6 months ago

#30342 closed defect (fixed)

9 dephects on prob_distr.c (April 2019)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prob-distr coverity
Cc: catalyst, riastradh Actual Points:
Parent ID: Points: 0.4
Reviewer: Sponsor:

Description

Similar to #29805, coverity is again furious at prob distr:

We got a bunch of these:

_____________________________________________________________________________________________
*** CID 1444030:  Incorrect expression  (SIZEOF_MISMATCH)
/src/test/test_prob_distr.c: 1254 in test_stochastic_weibull_impl()
1248     }
1249     
1250     static bool
1251     test_stochastic_weibull_impl(double lambda, double k)
1252     {
1253       const struct weibull dist = {
>>>     CID 1444030:  Incorrect expression  (SIZEOF_MISMATCH)
>>>     Adding "0UL /* 0 * sizeof (&dist - (struct weibull const *)&dist) */" to pointer "&weibull_ops" of type "struct dist_ops const *" is
>>> suspicious because adding an integral value to this pointer automatically scales that value by the size, 48 bytes, of the pointed-to type,
>>> "struct dist_ops const".  Most likely, the multiplication by "sizeof (0L)" in this expression is extraneous and should be eliminated.
1254         .base = WEIBULL(dist),
1255         .lambda = lambda,
1256         .k = k,
1257       };
1258     
1259     /*

and a bunch of these:

________________________________________________________________________________________________
*** CID 1415723:  Incorrect expression  (DIVIDE_BY_ZERO)
/src/feature/client/circpathbias.c: 194 in pathbias_get_scale_ratio()
188       (void) options;
189       /**
190        * The mult factor is the numerator for our scaling
191        * of circuit counts for our path bias window. It
192        * allows us to scale by fractions.
193        */
>>>     CID 1415723:  Incorrect expression  (DIVIDE_BY_ZERO)
>>>     In expression "networkstatus_get_param(NULL, "pb_multfactor", 1, 1, denominator) / (double)denominator", division by expression
>>> "denominator" which may be zero has undefined behavior.
194       return networkstatus_get_param(NULL, "pb_multfactor",
195                                   1, 1, denominator)/((double)denominator);
196     }
197     
198     /** The minimum number of circuit usage attempts before we start
199       * thinking about warning about path use bias and dropping guards */

Child Tickets

Change History (7)

comment:1 Changed 6 months ago by asn

Weird stuff all around.

The SIZEOF_MISMATCH errors are the ones we fixed in #29805.

And CID 1415723 has a tor_assert(denominator > 0); right above it....

comment:2 Changed 6 months ago by asn

Cc: catalyst riastradh added

comment:3 Changed 6 months ago by catalyst

in src/lib/math/prob_distr.h:

     53  *  We define this conditionally to suppress false positives from
     54  *  Coverity, which gets confused by the sizeof business.
     55  */
     56 #ifdef __COVERITY___
     57 #define TYPE_CHECK_OBJ(OPS, OBJ, TYPE) 0

There seems to be an extra underscore at the end of __COVERITY___. I think all other occurrences end with two underscores, not three.

comment:4 in reply to:  1 Changed 6 months ago by catalyst

Replying to asn:

And CID 1415723 has a tor_assert(denominator > 0); right above it....

I'm not seeing that tor_assert either in scan.coverity.com or in the file on master.

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

Replying to catalyst:

in src/lib/math/prob_distr.h:

     53  *  We define this conditionally to suppress false positives from
     54  *  Coverity, which gets confused by the sizeof business.
     55  */
     56 #ifdef __COVERITY___
     57 #define TYPE_CHECK_OBJ(OPS, OBJ, TYPE) 0

There seems to be an extra underscore at the end of __COVERITY___. I think all other occurrences end with two underscores, not three.

Yes, you are right. Indeed, coverity seems to have a revision of the codebase with 3 underscores, but that's not the case in master (since 48a574604bef): https://github.com/torproject/tor/blob/master/src/lib/math/prob_distr.h#L56

CID 1444641 was also fixed in #30180 and yet it still appears in the web interface.

Finally, wrt CID 1415723 (mentioned in the comment above), the CID does not appear in the web interface anymore (but i did receive it in an email on the 30th of April). The assert I was refering to is https://github.com/torproject/tor/blob/master/src/feature/client/circpathbias.c#L189

tl;dr coverity has a dated version of our codebase, and also sends emails about fixed issues. Not sure what we could do here, since I dont see a button that pulls the latest git rev.

comment:6 Changed 6 months ago by nickm

I've tried manually kicking one off. (Our current coverity setup is a cronjob on my development machine.) I'm still having to tell it to ignore certificate verification, since curl still doesn't like scan.coverity.com.

comment:7 Changed 6 months ago by asn

Resolution: fixed
Status: newclosed

Oook. Just got 16 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan. in the latest email, and all of these seem to be fixed. Thanks!

Created #30361 for two new ones.

Note: See TracTickets for help on using tickets.