Hi Ken,
----- Original Message -----
> 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
I've found similar things in some cases too ... but in others, blatant
bugs - though often on error paths or otherwise very uncommon paths.
Still, some of them are doozies - like the incorrect memset sizes in the
linux PMDA init code (BAD_SIZEOF) and the "if ((sts == pmFooBar()) < 0)"
cases (CONSTANT_EXPRESSION_RESULT)...
So, some gems are lurking in there.
> 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.
That is indeed the case unfortunately, although Eric has said he's happy
to spin some new results whenever we have updates worth trying.
>
> 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 also don't think we should touch the "pandering" type ones (in the
commercial product, AIUI there is a way to say "I looked at it, its
fine, ignore it") which seems to be their preferred model rather than
working around it with code changes.
> 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, ...
Certainly these things are all needed & priority ... but there are a few
easy wins in here (off-by-one buffer overflows in libpcp_http, etc) as
counter-examples, so I think I'll keep looking through it as time permits
to see if there's much else worth fixing.
cheers.
--
Nathan
|