Closed alexw91 closed 7 years ago
An alternative here would be to pull in a static analysis tool and run it in our travis build. cppcheck and the Clang static analysis tool(scan-build?) come to mind.
I think it's awesome that we've added clang scan-build
as a build requirement to s2n, but I also believe we should be going for defense in depth here as well, and try to add as many free, low false-positive rate, static code analyzers as possible. Having multiple different static analyzers running during our builds protects us from any blind spots that may be present in any particular analyzer. To give an example, the maintainer of curl
wrote a blog post about Coverity
vs clang
vs Fortify
analyzers, and found that Coverity
found about double the security issues that clang
did, while Fortify
output a ton of noisy false positives.
I've looked around a little bit and ended up finding SANS Institute - Secure Software Development and Code Analysis Tools which has some relevant recommendations for static analysis tools for C (even though it's almost 15 years out of date). You can look at Appendix A of that document to see example output of the different analyzer output to get a feel for what types of issues are found.
It may make sense to add one (or more) of the following as make
targets and eventually add them as Travis build requirements once we're sufficiently confident that they won't generate false positives (or never add them as build requirements if they generate too much noise, and only manually review and verify their output):
A "List of Lists" of C Code Static Analyzers:
We've integrated cppcheck
, clang scan-build
, and KWStyle
. We may want to add others in the future (Coverity
and PVS-Studio
), but we can open individual issues for those if we want to add them in the future.
Just in case we want to allow this stack to be used on embedded devices in an automotive context, compliance with these guidelines will be mandatory: https://en.wikipedia.org/wiki/MISRA_C
Coverity is a popular static code analyzer integrated into a lot of Github projects, but this issue could apply to any similar static code analyzer.
Overview
I think it would be a good idea to integrate Coverity Static Code analysis into the Travis build to catch any errors that might be missed during human review. It is currently used by the Linux Kernel, OpenSSL, and Python, all of which seem to have good experiences with it, and all saying that it has helped their defect rate go down.
If it were to be integrated, any issues found by Coverity Static analysis would be kept private and only accessible to developers of s2n until the issues are fixed.
There is some rate limiting on the number of scans that are allowed, but it seems high enough for the number of changes/commits that s2n is getting. From their website: