Coverity
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
Coverity
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 ), 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
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 ), 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.
- wandererfan
- Veteran
- Posts: 6268
- Joined: Tue Nov 06, 2012 5:42 pm
- Contact:
Re: Coverity
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
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
Re: Coverity
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...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.
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.
Re: Coverity
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...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.
More details can be found at https://scan.coverity.com/tune (login first)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%.
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
Re: Coverity
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.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 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-
Re: Coverity
Should we agree on some standard way in regard to fixing issues from Coverity as it is for example suggested in the Python reference?
Also an example from the LibreOffice fixed bugs list https://wiki.documentfoundation.org/Rel ... .1.0/Beta1You 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.
- wandererfan
- Veteran
- Posts: 6268
- Joined: Tue Nov 06, 2012 5:42 pm
- Contact:
Re: Coverity
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.
I've always found it difficult to get coders to keep QA records up to date.
Re: Coverity
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?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.
Re: Coverity
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-
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-
Re: Coverity
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.ian.rees wrote: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.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.