Friday, July 25, 2008

Obligation analysis: success!

The implementation of obligation analysis in FindBugs seems to be in a useful state.

The analysis found about 8 bugs related to unclosed streams in FindBugs itself. If you write tools to find bugs, people always ask you if the tool finds bugs in itself. Well, FindBugs certainly does on a regular basis.

I analyzed Vuze (formerly Azureus), and the detector reported 35 warnings. Of those warnings, 17 appear to be legitimate issues, and another 17 are probably benign warnings that could be eliminated through the use of the JSR-305 @WillClose or @WillCloseWhenClosed annotations. (These annotations are used to specify methods and objects that assume responsibility for closing a resource.) 1 warning was essentially a duplicate of another (apparently correct) warning.

Analysis of jEdit was not quite as impressive, but still interesting: 4 apparent bugs, 9 warnings about probably-correct code that could be eliminated by annotations, and 3 cases where the analysis was wrong. (I need to investigate the last category.)

One type of false positive the paper didn't mention (that I can recall) was when one resource object "wraps" another. This, of course, is a common design pattern (Adapter) used in the package.

InputStream in = new FileInputStream(filename);
Reader r = new InputStreamReader(in);
try {
} finally {
The analysis assumes that the InputStream is the obligation needing to be cleaned up, but the finally block closes the Reader instead.

The @WillCloseWhenClosed annotation would fix this problem (explicitly specifying the "transfer" of one obligation type to another), but since JSR-305 is not official yet, the standard Java classes don't use this annotation.

I worked around this issue by having the detector find likely places where an obligation transfer occurs, and then checking to see if the unmet obligation can be explained by an obligation transfer. This heuristic seems to work fairly well in practice.

Interestingly, a similar issue occurs when the "wrapped" resource is closed instead of the "wrapper". Technically, this could be considered a bug (the "wrapper" resource's close() method might have extra work it wants to do), but in many cases this is also a correct approach. The same heuristic (looking for probable obligation transfers) seems to be effective.

No comments: