On Thu, 2012-01-12 at 16:14 +1100, Mark Goodwin wrote:
> Coverity is a sophisticated static code analysis tool.
> Among other things, it checks for the conditions listed here:
> https://www.securecoding.cert.org/confluence/display/seccode/Coverity+Prevent
>
> Below is the result of running a scan over the pcp src tree:
> http://people.redhat.com/mgoodwin/pcp-cov/index.html
>
> Nathan and Eric have already looked at some of these - each error
> is numbered and in most cases the error is obvious, but certainly
> not all. Perhaps we should divvy them up between ourselves, fix
> the obvious ones, and then dive into the more complicated
> cases. We'll probably get some new QA tests out of this too.
I'm all for improving the real quality of code, but ...
I've looked at a few of USE_AFTER_FREE ones also, and ...
1. the "error" is far from obvious, and
2. after a lot of analysis, I think the tool is wrong (in one case) and
chasing a combination of logical conditions that have probability close
to zero of happening in a couple of other cases, and
3. this tool is not apparently freely available, so the PCP developers
(outside RH I assume) have no way to either check if changes remove the
errors, nor prevent introduction of errors in subsequent checkins.
As another example from interp.c, consider
http://people.redhat.com/mgoodwin/pcp-cov/1/105interp.c.html#error
now ac_offset is only set in a few places ... either to an expression
involving sizeof() that must be positive, or the result of ftell() ...
if ftell returns -1 (as this particular "analysis" suggests), then the
sky has well and truly fallen in (check the ftell man page!) and calling
fseek with a negative offset will be the least of our worries ... fixing
this properly would mean adding error check and error propagation code
for _every_ use of ftell() ... I think this has a higher probability of
introducing a new catastrophic bug than it does of detecting a real
problem in production code.
This smells a lot like the "pandering to gcc" changes we made that made
zero practical difference to code robustness or correctness, but only
served to make the newly introduced (by gcc) compilation warnings go
away.
I remain unconvinced that this is the best return on effort that could
be invested in PCP ... but then again I'm an old school skeptic, and
will not be surprised to form a minority of one ... 8^)>
The same effort could be directed towards making significant
improvements in other areas, e.g. excise the proc metrics from the linux
pmda, build better authentication and access control mechanisms,
profiling to quantify the cost of making libpcp thread-safe, ...
|