[postgis-devel] Static Analysis

[Even Rouault]

Le vendredi 06 mai 2016 10:55:50, Mark Cave-Ayland a écrit :
> On 05/05/16 19:06, Paul Ramsey wrote:
> > Hey Devs,
> > 
> > Are we interested in receiving static analysis reports (Coverity) on
> > the PostGIS code base?
> > 
> > The folks at CrunchyData are willing to stick-handle the bureaucracy
> > around getting Coverity account for the project and a system set up to
> > regularly pass the PostGIS code base through Coverity static analysis.
> > Coverity provides free (as in beer) accounts for open source projects,
> > so the actual Coverity "account" would be the PostGIS project's and
> > the PSC would control it.
> > 
> > Anyways, other than providing an annoying list of things we should do
> > (gah!) I see no downside to having some more information on our code
> > cleanliness/security. Unlike the transifex stuff, there'd be no
> > dependencies on a foreign system, since if Coverity ever shut off our
> > access we'd be no worse off than we are right now.
> > 
> > Thoughts?
> 
> Definitely! I suspect that there will be a number of things to check
> after the first scan which may take some time. Note that Coverity does
> produce false positives, so perhaps some thought should be given to as
> to whether we should aim for a "clean" scan to the point where we can
> add annotations to the code for cases like these.

We have gone through the Coverity route for GDAL. Dealing with the results of 
the first scan is likely to be painful (took months to clear the > 1300 initial 
stock). There are some false positives, but not that much (10% ?). In general 
you get false positives in portion of the code where the logic is convoluted 
and you can get rid of most of them by simplifying the code a bit, or adding 
extra conditions, and making it more obvious, including for humans. 
For real false positives, you can add the following annotation before the line
/* coverity[name_of_the_violated_rule] */

The largest occurence in GDAL code base is /* coverity[tainted_data] */ , that 
is due to using the value of an environment variable with no or little 
checking. Likely not going to affect PostGIS.

We've also used Clang Static Analyzer. To be honest, it is far less powerful 
than Coverity and has a high rate of false positives, and reworking the code 
to make it happy can be really hard sometimes. It did uncover a couple of 
things other tools didn't find though but the rate time invested to investigate 
reports / bug founds is prettly low. If you want to give it a try, I'd 
recommend only using it after cleaning all Coverity warnings, so as to limit 
the number of things to look at.

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com