Please consider using Coverity Scan (free for open source)

[tomreyn]

I'm a contributor to MegaGlest (an open source RTS game), which recently started using Coverity Scan for static source code analysis. https://en.wikipedia.org/wiki/Static_code_analysis

While we surely would prefer to only rely on cppcheck (which is already pretty useful!), clang/llvm's source code analysis and similar open source tools to produce open source software, Coverity Scan has helped us find a notable amount of flaws which the other tools did not, at a very low false positive rate. While this may sound like spam, I have no ties to Coverity other than agreeing to their TOS (which do not require me to advertise their product), nor NSA (other than everyone else has, I hope).

https://scan.coverity.com/about

Since you are approaching a v1.0 release, it might be nice to run it once or twice before you release and fix some of the bugs it would most likely find (I'm not trying to insult you here), as it did for many other projects including several popular open source games such as Wesnoth and 0 A.D., but also many fundamental open source software projects in general: https://scan.coverity.com/projects

The sign-up process as well as the scan workflow are discussed at: https://communities.coverity.com/videos/1114

Several questions, including which projects may be scanned on this platform and how often you may submit a new build for review are discussed here: https://scan.coverity.com/faq

To sign up (you may do so using your Github account), start here: https://scan.coverity.com/

[stefanbeller]

Thanks for this hint. :)

We have been using Cppcheck successfully for a while now to find flaws in flare. I also had a look at http://css.csail.mit.edu/stack/ which is another frontend for clang to find bugs. However using the scan-build builtin we get indeed some warnings:

/home/sb/OSS/flare-engine/src/PowerManager.cpp:939:44: warning: Dereference of null pointer
        float theta = calcTheta(src_stats->pos.x, src_stats->pos.y, target.x, target.y);

In file included from /home/sb/OSS/flare-engine/src/MenuPowers.cpp:25:
In file included from /home/sb/OSS/flare-engine/src/CommonIncludes.h:22:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/algorithm:61:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algo.h:62:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_tempbuf.h:60:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_construct.h:61:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ext/alloc_traits.h:37:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/allocator.h:46:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8/bits/c++allocator.h:33:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ext/new_allocator.h:133:30: warning: Called C++ object pointer is null
      destroy(pointer __p) { __p->~_Tp(); }

/home/sb/OSS/flare-engine/src/TileSet.cpp:119:33: warning: Dereference of null pointer
                                        anim[TILE_ID].pos[frame].x = toInt(repeat_val);

/home/sb/OSS/flare-engine/src/MenuInventory.cpp:182:30: warning: Division by zero
                                int random_item = rand() % removable_items.size();
In file included from /home/sb/OSS/flare-engine/src/LootManager.cpp:38:
/home/sb/OSS/flare-engine/src/UtilsMath.h:74:26: warning: Division by zero
        return minVal + (rand() % (d + signum(d)));

In file included from /home/sb/OSS/flare-engine/src/MapRenderer.cpp:30:
/home/sb/OSS/flare-engine/src/UtilsMath.h:74:26: warning: Division by zero
        return minVal + (rand() % (d + signum(d)));
/home/sb/OSS/flare-engine/src/EffectManager.cpp:49:3: warning: Called C++ object pointer is null
                effect_list[i].id = emSource.effect_list[i].id;
/home/sb/OSS/flare-engine/src/Animation.cpp:82:39: warning: Dereference of null pointer
                        render_offset[base_index + kind].x = _render_offset.x;
/home/sb/OSS/flare-engine/src/UtilsMath.h:74:26: warning: Division by zero
        return minVal + (rand() % (d + signum(d)));

[tomreyn]

So what I'm saying is that after we (MegaGlest) had 0 relevant bugs reported by cppcheck and clang's scan-build builtin, Coverity still found >200 real bugs.

I talked to Coverity Scan staff yesterday and was told I could register Flare there (even without being a project contributor) for the purpose of running tests and providing the results to you. This would allow you to get an idea of how using the service would be useful to you without requiring you to review and agree to any TOS. If you later prefer to take ownership of this project on this platform both Coverity staff and I will be happy to transfer it to you.

Does this sound like an acceptable approach to you for now?

[stefanbeller]

I started to register with coverity, but I am a little confused as you'd need to upload a bunch of files. I'd be happy to invite you to help here.

[tomreyn]

Actually you just upload one (tar.gz) file which contains a single directory ("cov-int" which is created by their scanning tool which you need to download initially). I'm happy to help, my identity there is tomreyn at megaglest.org which is also where you'd send the invite to.

[tomreyn]

Thanks for setting us up, Stefan. I've just committed a first build and the result is now live on https://scan.coverity.com

64 possible bugs were detected, one of which was automatically dismissed based on other users' reports that this detection mechanism is broken. So you have 63 left, 9 of which are marked as "high impact". Not much, but your code base isn't that large, and then Clint and everyone else seem to have simply written good code, too. ;)

Now the next thing you may want to do after reviewing the results, is to add yourself to the "email notification" area on the "project settings" tab, and to review the video I linked in the initial post here in preparation to commit your own build (to get an idea of how this process works). According to the Coverity Scan FAQ projects with <= 100K SLOC (I think you fit into this category) may submit up to 7 builds per week (it's not clear when exactly this counter resets, though) for analysis.

In case you have any related questions which don't fit into this issue tracker: I'm a regular on your IRC channel irc://irc.freenode.net/#flarerpg (but may be AFK) and will happily respond to personal e-mail addresses on this matter for a while, too.

[stefanbeller]

We're now down to 15 defects thanks to @dorkster So I think we're getting there.

[stefanbeller]

I would like to bump this issue. We should automate the coverity scan for each build, the build recipe is found at https://github.com/bjorn/tiled/commit/c0fac49b6f9e96716a5554cb7d9a990bddbe7c29

[tomreyn]

Note that in today's Coverity Scan newsletter it is stated that:

Due to the increase demand for the Scan service, we have implemented a new guideline that each project will be able to submit one build per week.

However, this just means that additional submissions during the same week will fail / be rejected. So automating submissions should not cause trouble as long as you don't trigger build job failures on your continuous integration (if any).

[stefanbeller]

There is a possibility to trigger a run of coverity scan at travis if a certain branch is merged back into master. Maybe we want to implement this technique, so not everybody needs to download and setup the coverity software.

[tomreyn]

Is this documented somewhere (it's news to me)? And does it work with the free variant of Travis CI?

[stefanbeller]

https://scan.coverity.com/travis_ci

I was inspired to do this by https://github.com/bjorn/tiled/commit/c0fac49b6f9e96716a5554cb7d9a990bddbe7c29

[stefanbeller]

So for now I have just a script inside a virtual machine instead of the proposed way to trigger coverity by travis.

[tomreyn]

Coverity has a new release (7.5) of their scan engine available for download now: http://blog.coverity.com/2014/06/17/coveritys-next-generation-software-testing-platform/

Your VM is currently using v7.0.2, so (while the major feature additions are probably of no use to you, but bug fixes may be) you may wish to upgrade.

[stefanbeller]

updated, running a scan now.

[tomreyn]

Thanks. Looks like it wasn't necessarily worth the effort - It's mostly the newly added "we-did-not-detect-heartbeat-embarrassment" detections. ;-)

[dorkster]

The latest master put us at 3 outstanding defects. All of which I determined to be false positives/intentional.