DeepCode’s Top Findings#3: Java missing Close or Flush

Hey,

Language: Java
Defect: missing~flush~or~close (Category General 2)
Defect — Resource leak / Data loss
Failure to clean up filehandle might lose data at worse, leak resources at best.

Background:

This type of bug is an interesting case as it not only shows a best-practice and a language concept but also a source of false positives for Static Program Analysis.

Let us start by a prominent example here: Example

...
public class DumpBytecodeVisitor
extends BytecodeVisitor<Void>
{
   private final PrintWriter out;
   private int indentLevel;
   public DumpBytecodeVisitor(Writer out)
   {
      this.out = new PrintWriter(out);
   }
...

The code creates a new PrinterWriter from an existing Writer without auto-flushing (see Java Documentation on PrinterWriter). The Java PrinterWriter class writes formatted data to an underlying Writer.

Searching the code of the above example from prestodb reveals: It never calls flush() or close(). The issue with this that the code might suffer from data loss and bad resource handling. When the application terminates, the operating system in most cases takes care of open streams but buffered, not yet written data gets lost. Secondly, not closing the stream is simply a bad practice as stream handlers are a precious resource and should be taken care of. In JDK < 7, the application has to explicitly call close(). But it seems the issue of not doing so was so immanent that Java added a new way to handle open streams: The try-with-resources statement. With Java 9, the following construct automatically closes the stream:

BufferedWriter writer = new BufferedWriter(new FileWriter(filename);
try (writer) {
   writer.write(someString); //Write someting to file
}
catch(IOException e) {
   // Handle error conditions
}

To be automatically closed, the stream (here BufferedWriter) needs to implement the AutoClosable interface. PrinterWriter implements it.
For a Static Program Analysis, this bug is tricky as it easily can generate false positives. Streams can be generated, being deliberately kept open for the runtime of the application and handed on through the code to different handling classes (which is an antipattern). So, the Static Program Analysis needs to track the stream object through the flow of the program for all possible paths of the application. No path should lead to a situation where the stream is not closed. On the other hand, the Static Program Analysis has to understand the effect of above mentioned try-with-resources statement. This blog article gives a good overview.

In our initial example, the following things should be changed: (1) The class which owns the stream, should be responsible for closing it properly. (2) By using flush() or simply setting autoflush to true in the constructor, the class should make sure to write out data.

If you are unsure about the handling open and close or flush in your code, visit deepcode.ai and give it a test.

CU

0xff