tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: Coverity static analysis scanning
Date Tue, 26 Aug 2014 21:26:34 GMT
Mark,

On 8/26/14, 5:20 AM, Mark Thomas wrote:
> I have been pinged off-list by Coverity to say that they have set up
> Tomcat with a free account with their static code analysis service.
> 
> I think I have the ability to send invitations so if anyone wants to
> take a look at the results, just reply here.
> 
> I have taken a quick look and they do appear to have found some valid
> threading issues. There are ~350 issues in total and I don't yet have a
> feel for the false positive rate.

Wow, this is great. I've used FindBugs before both inside and outside of
ASF projects, but this is ... just amazing.

It does catch a lot of sanity-checks and complains about them. I get
DEAD CODE warnings all the time (in FindBugs) for especially JDBC code
when I've caught all possible exceptions and yet still have a finally
block that, for example, checks a Connection reference for null and
closes it in that case. While the code is technically dead, it's
future-proof against someone adding another call that throws a different
type of exception, re-ordering some of the operations, or making some
other change and forgetting to modify the finally block, etc.

It would be nice to know what the consensus is amongst the team about
what to do in these cases: should all dead code segments be considered
logical oversights and corrected? Or is additional sanity-checking and
future-proofing a good idea?

A good example is issue 45040
(https://scan3.coverity.com:8443/reports.htm#v16818/p10363/fileInstanceId=567725&defectInstanceId=145101&mergedDefectId=45040):
a logical bug in HttpServlet that should probably remain as-is. It's in
the HttpServlet.doOptions method where we build a list of acceptable
HTTP verbs. /Technically/, if ALLOW_GET is set, then ALLOW_HEAD must be
set and therefore checking for "allow" (the string of verbs we're
building) for NULL is illogical. One could argue that ALLOW_HEAD should
be independent of ALLOW_GET -- why can't a servlet implement doHead but
not doGet -- but it probably always makes sense to check for null.
Stated differently: checking for NULL never hurt anybody.

-chris


Mime
View raw message