Coverity

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

Re: Coverity

Postby saso » Mon Aug 15, 2016 9:17 am

wmayer wrote:
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.
Come and join the party Werner, you don't have to set up anything, as soon as you login you can start to browse the report :)
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 9:51 am

ian.rees wrote: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-
Well I am not trying to push for anything here, I am ok with what ever the community decides to do, I am just trying to understand the Python and LibreOffice folks, since they seem to be doing this for some time now and it does seem useful to have the CID (Coverity ID) somewhere, for example when you are reviewing the new code before merging in to master or maybe later in Coverity when you try to find out how and when something was fixed...

Or are you maybe warming up to fix all 237 Unchecked dynamic_cast issues at once? ;)
ian.rees
Posts: 696
Joined: Sun Jun 15, 2014 3:28 am
Contact:

Re: Coverity

Postby ian.rees » Mon Aug 15, 2016 10:03 am

saso wrote:Or are you maybe warming up to fix all 237 Unchecked dynamic_cast issues at once?
Well, I've done about a dozen this evening between other things :). Mainly just trying to avoid a bunch of commits for very similar few-line changes.

Do you think the CIDs are going to persist when/if we get a FreeCAD Coverity account? I'm assuming this one is just yours for experimenting, so if people like it we'll end up with a general FreeCAD Coverity account? Regardless, I'll put the CIDs in the commit messages before pushing. -Ian-
wmayer
Site Admin
Posts: 13782
Joined: Thu Feb 19, 2009 10:32 am

Re: Coverity

Postby wmayer » Mon Aug 15, 2016 12:01 pm

I had a quick look at the issues. The very first we need is a filter to ignore the components CXX (i.e. PyCXX), include, Other, zipios++ and the moc files. For CXX there is only one real issue which has been fixed recently. For all other components there is nothing we can do anyway.
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 12:49 pm

ian.rees wrote:
saso wrote:Or are you maybe warming up to fix all 237 Unchecked dynamic_cast issues at once?
Well, I've done about a dozen this evening between other things :).
Awesome 8-)
ian.rees wrote: Do you think the CIDs are going to persist when/if we get a FreeCAD Coverity account? I'm assuming this one is just yours for experimenting, so if people like it we'll end up with a general FreeCAD Coverity account?
True, it is a good question and I did look a bit around on this topic in the past...

First thing to note is that Coverity, from its workflow, is not similar to for example Travis or AppVeyor, it is not really connected to github (or other repo) and is not doing any automatic building or monitoring of the code changes by itself. It is set up at the moment under my account because I had to made an account to start with it and because I have tried to set it up with my Travis, this would make the uploading of new builds to Coverity a bit more automatic and easier, but unfortunately after first few successful builds it is now simply to slow (goes over my 90 min limit on Travis) so I am now doing it manually (in an VM to keep it clean and pulling directly from FC master). So Coverity is just this database that we login to, and I have seen with other projects, that it should be possible to just transfer the ownership of it and keep everything as it is including all the history... But I have not yet asked the Coverity support about it, so I am not 100% sure.

Second, and I am also not sure about this one :roll:, it does seem as if Coverity is tracking this issues globally, by file or function (?), because if you browse a bit around our project you will find some comments from folks that are not part of our project and they are on issues that are from 3rd party libs, so someone else that is using the same libs (or the original authors) also seems to have used Coverity and now we can see their triage. So like a global action to both automatically and manually review and fix all of the open source code? :) But this is just an observation from my side, I am not really sure about it and don't know what would happen with our current CIDs if we start another FreeCAD project on Coverity.

The general idea with Coverity however is that there should be just one project and people join to work on it together. Another thing to understand with Coverity is that originally it was created mostly for security reasons and it is why it is by default set up so that it is quite restricted to who has access to it and how much access someone has. This is somewhat less critical for a project like FreeCAD, but for example imagine if a critical vulnerability is found by it in a project like Python or LibreOffice. I have open it up now a bit more in hope that it could get a bit more used and useful for us but the general restrictions remain.

I don't code a lot, so most of the issues go over my head, but I am willing to help with creating new builds and manage it if it can be useful and I am also ok to transfer the ownership at any time :)
Last edited by saso on Mon Aug 15, 2016 5:21 pm, edited 3 times in total.
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 1:02 pm

wmayer wrote:I had a quick look at the issues. The very first we need is a filter to ignore the components CXX (i.e. PyCXX), include, Other, zipios++ and the moc files. For CXX there is only one real issue which has been fixed recently. For all other components there is nothing we can do anyway.
Yes, this "Components" is something that I have set up manually and I can change them (add, remove, ignore), it works just by adding path patterns like .*/src/App/.* for App. I could make for example subcategories for all the WB in Mod, but that seemed to me a bit to much :) and I have also set all not to be ignored for now, so that we can get a general picture of all the results, but it takes just a moment to change some of them to ignore (just need to make sure that the "Component", its path, is set up properly).

Setting up the "Modeling File" is however something that I could use some help with :roll:

https://scan.coverity.com/tune (need to login first)

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

The general guide from the Python project also talks about it https://docs.python.org/devguide/coverity.html
Last edited by saso on Mon Aug 15, 2016 5:28 pm, edited 3 times in total.
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 1:23 pm

This "Components" and "Modeling File" are used as general filtering for the project, one can however do quite a lot of filtering on a more personal level as I have described before, for example if one goes 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/*... (there are probably like thousand options to set up personal filters) :)
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 7:07 pm

wmayer wrote:I had a quick look at the issues. The very first we need is a filter to ignore the components CXX (i.e. PyCXX), include, Other, zipios++ and the moc files. For CXX there is only one real issue which has been fixed recently. For all other components there is nothing we can do anyway.
I have setup two new Component Names, "moc files" and "share". "moc files" should include all the moc files :roll: "share" now has the pattern .*/usr/share/.* and includes the netgen that was before under the "Other". "Other" is a generic component that includes all the issues that are not in any of the custom components (if no custom components are set then all issues go under "Other"). This will allow us to set ignore for the different components. I have not set ignore on any of them yet...
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Mon Aug 15, 2016 9:09 pm

@wmayer because Coverity has no idea what is going on with the code until we upload a new build to it I have followed Ian's example and set in Coverity all your fixed issues from this commit https://github.com/FreeCAD/FreeCAD/comm ... 5c9ec84363 to "Action: Fix Submitted"
User avatar
saso
Posts: 1135
Joined: Fri May 16, 2014 1:14 pm
Contact:

Re: Coverity

Postby saso » Fri Aug 26, 2016 2:13 pm

I have now set ignore on moc files, 3rdParty, CXX, zipios++, include and share. This brings down the number of outstanding issues from 1152 to 555. Also Werner, and with some help from Ian, have submitted fixes for 420 issues and marked 47 as false positives, so I will try to submit a new build in the next days. If Coverity will hopefully be happy with the new fixes this should all together bring us from 1152 to just about 88... Yes, the cavalry has arrived 8-)

PS: CID 129217 and 129216 should be the last two from the unchecked dynamic_cast type ;)
Last edited by saso on Sat Sep 03, 2016 1:42 pm, edited 1 time in total.