Coverity

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
User avatar
saso
Posts: 1305
Joined: Fri May 16, 2014 1:14 pm
Contact:

Coverity

Postby saso » Thu Aug 11, 2016 8:18 pm

Below are some statistic for the latest Coverity scan that was done a few days ago for build 0.17.8158 - 6135ec5 and I would like to invite anyone that has interest in this, has contributed some code to FreeCAD in the past and is active here on the forum to join the project and have a closer look at it (recommended to sign in with your GitHub account).

https://scan.coverity.com/projects/saso ... ac-freecad

The static analysis by Coverity is done on the C++ code (so, specially C++ gurus are welcome :roll: ), but the latest version also started to support python versions 2.7.x, so in the future we could have more or less all of the FreeCAD code covered.

Analysis Metrics
Version: 0.17.8158
Last Analyzed: Aug 07, 2016
Lines of Code Analyzed: 1,630,108
Defect Density: 0.71

Defect changes since previous build dated Mar 22, 2016
Newly detected: 377
Eliminated: 90

Defects by status for current build
Total defects: 1,299
Outstanding: 1,152
Dismissed: 16
Fixed: 131
wandererfan
Posts: 2878
Joined: Tue Nov 06, 2012 5:42 pm

Re: Coverity

Postby wandererfan » Sun Aug 14, 2016 12:12 am

Just took a quick look.

Is there a way to limit the size of the defect list? Maybe only the defects for /Mod/Part or /Mod/TechDraw? It's a big list. I tried a few filter variations, but was unsuccessful.

There's a lot of false positives for "unchecked dynamic cast". In all the cases I looked at there was a "->isDerivedFrom" just before the dynamic cast, so no real need to check if the cast worked. The Python project page mentions a way to flag known false positives.

Could be useful, but not very friendly to an untrained visitor.

wf
User avatar
saso
Posts: 1305
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Sun Aug 14, 2016 8:05 am

wandererfan wrote:Is there a way to limit the size of the defect list? Maybe only the defects for /Mod/Part or /Mod/TechDraw? It's a big list. I tried a few filter variations, but was unsuccessful.
True, it is quite a big list, but as you suggested, I also agree that the way to go for it is to chop it down in to smaller manageable portions :) I was thinking that maybe taking a quick look at all the high impact defect (71 of them at the moment) would maybe make sense since they potentially represent more critical security issues but could also potentially improve the stability of the program...

If you go to "View Defects" and click on the menu (the three horizontal lines icon in the top left corner) and open the general "Outstanding Defects" view, then clicking on the gear icon to change the filter and tick the "save as a copy" to create a new one, change its name, and under "Filters -> File" you set */Mod/Part/*, you should hopefully get what you are looking for... */Mod/Part/* - 61 issues match, */Mod/TechDraw/* - 60 issues match :)
Last edited by saso on Sun Aug 14, 2016 8:30 am, edited 2 times in total.
User avatar
saso
Posts: 1305
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Sun Aug 14, 2016 8:18 am

wandererfan wrote:There's a lot of false positives for "unchecked dynamic cast". In all the cases I looked at there was a "->isDerivedFrom" just before the dynamic cast, so no real need to check if the cast worked. The Python project page mentions a way to flag known false positives.
Yes, I did manage to set up some of the basic configurations for the project, but indeed the next important step would be to create this "Modeling File" that helps to lower the number of false positives...
From Coverity wrote:Static code analysis has some limitations in its ability to understand certain dynamic operations. This limitation may result in falsely detecting defects. Since most false-positive defects are caused by few functions in your code base, Coverity allows you to tell the analysis engine to treat these functions differently. This is called a Modeling File. By providing a modeling file, most projects reduce their false-positive rate to the ballpark of 10%.
More details can be found at https://scan.coverity.com/tune (login first)

An example modeling file from the Python project https://bitbucket.org/mirror/cpython/sr ... ew-default

And some general instructions from the Python project on how to work with Coverity that I have found quite usefull for the first steps https://docs.python.org/devguide/coverity.html
ian.rees
Posts: 696
Joined: Sun Jun 15, 2014 3:28 am
Contact:

Re: Coverity

Postby ian.rees » Sun Aug 14, 2016 11:03 am

wandererfan wrote:There's a lot of false positives for "unchecked dynamic cast". In all the cases I looked at there was a "->isDerivedFrom" just before the dynamic cast, so no real need to check if the cast worked.
I think using static_cast() is a cleaner way to handle situations where we're using something like ->isDerivedFrom to ensure the casted object is the right type. The intent of the code is clearer that way, and no need to maintain a list of exceptions for coverity or whatever other tools come along.

I did find a possible real issue in the Coverity results for TechDraw relating to this (in App/DrawProjGroup.cpp), will try to submit a fix tomorrow. -Ian-
User avatar
saso
Posts: 1305
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Sun Aug 14, 2016 12:22 pm

Should we agree on some standard way in regard to fixing issues from Coverity as it is for example suggested in the Python reference?
You should always create an issue unless it’s really a trivial case. Please add the full url to the ticket under Ext. Reference and add the CID (Coverity ID) to both the ticket and the checkin message. It makes it much easier to understand the relation between tickets, fixes and Coverity issues.
Also an example from the LibreOffice fixed bugs list https://wiki.documentfoundation.org/Rel ... .1.0/Beta1
wandererfan
Posts: 2878
Joined: Tue Nov 06, 2012 5:42 pm

Re: Coverity

Postby wandererfan » Sun Aug 14, 2016 2:08 pm

Don't suppose there is an automagic connection between Mantis and Coverity? Like how changesets get linked to tickets?
I've always found it difficult to get coders to keep QA records up to date.
User avatar
saso
Posts: 1305
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Sun Aug 14, 2016 2:39 pm

wandererfan wrote:Don't suppose there is an automagic connection between Mantis and Coverity? Like how changesets get linked to tickets?
I've always found it difficult to get coders to keep QA records up to date.
Would a manageable minimum be to at least name all the related commits / pull requests with a summary starting with "Coverity CID ..." and a link to it in Coverity report when the issue is fixed? And opening a new related ticket in mantis only in case the issue would need some further discussion / review?
ian.rees
Posts: 696
Joined: Sun Jun 15, 2014 3:28 am
Contact:

Re: Coverity

Postby ian.rees » Sun Aug 14, 2016 9:05 pm

I agree with Saso that it would be nice to keep discussion about bugs in the tracker or forum, instead of adding Coverity discussions in to the mix.

Not sure how much value we'll get from adding Coverity CID in to every commit message like the LibreOffice example - seems like a lot of their messages say essentially the same thing, and I presume the code changes are quite similar. Would be OK to have a group of fixes with a message like "Coverity Null Pointer Dereference warnings in TechDraw"? -Ian-
wmayer
Site Admin
Posts: 14603
Joined: Thu Feb 19, 2009 10:32 am

Re: Coverity

Postby wmayer » Sun Aug 14, 2016 9:24 pm

ian.rees wrote:
wandererfan wrote:There's a lot of false positives for "unchecked dynamic cast". In all the cases I looked at there was a "->isDerivedFrom" just before the dynamic cast, so no real need to check if the cast worked.
I think using static_cast() is a cleaner way to handle situations where we're using something like ->isDerivedFrom to ensure the casted object is the right type. The intent of the code is clearer that way, and no need to maintain a list of exceptions for coverity or whatever other tools come along.
Exactly! Using isDerivedFrom and doing a dynamic_cast is a tautological check. Either you use isDerivedFrom together with static_cast or you use dynamic_cast alone where you then have to check the returned pointer.