Coverity

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
saso
Veteran
Posts: 1920
Joined: Fri May 16, 2014 1:14 pm
Contact:

Coverity

Post by saso »

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

A general post about the security and code quality tools https://forum.freecadweb.org/viewtopic. ... 02#p274069
Last edited by saso on Sat Jul 18, 2020 7:30 am, edited 2 times in total.
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Coverity

Post by wandererfan »

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
Veteran
Posts: 1920
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Post by saso »

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
Veteran
Posts: 1920
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Post by saso »

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

Post by ian.rees »

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
Veteran
Posts: 1920
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Post by saso »

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
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Coverity

Post by wandererfan »

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
Veteran
Posts: 1920
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Post by saso »

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

Post by ian.rees »

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
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Coverity

Post by wmayer »

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.
Post Reply