Custom FindBugs detectors in Android

Fri, Mar 10, 2017 Companion code for this post available on Github

Modern compilers can detect all sorts of things, from the humble type error to mismatched format strings, but in some cases it’s just not feasible or the the use case is not widespread enough for an error pattern to be detected at compile time. Luckily, in the Java/Android ecosystem, there are two tools that exist to take compile(ish)-time checking to the next level - Android Lint, a tool supplied as part of the Android SDK for catching Android-specific errors (resource size inconsistencies, missing translatins, etc) and the FindBugs Project, a well established project from the university of Maryland, and what we will be digging into here. We’ll take a quick look at what it is, go over a small refresher on Java’s try-with-resources pattern and then dive into writing our own detector that will ensure all Cursor operations are wrapped in one of these try-with-resources blocks.

Some background

Findbugs is a static analysis tool that operates on compiled java bytecode to detect code that is deemed acceptable by the compiler, but not necessarily what the programmer mind. Examples would be detecting a null return from a method that should return Boolean, inconsistent synchronization of variables and unnecessary math operations.

At it’s core, FindBugs is powered by the Apache BCEL, a library for the inspection and manipulation of compiled Java bytecode. On top of this, FindBugs adds some extra parsing for easy access of operands, a visitor pattern for iterating over bytecode, and a mechanism for accumulating and displaying bugs. If you don’t already use FindBugs as a part of your android testing and deployment pipeline, even just the core detectors are well worth adding to a project. An example of how to add FindBugs as a task to an existing Android project (with gradle) can be seen here.

Customization

But what happens if you have a code case that is too specific to your application (e.g. invoves Android classes) or to your internal practises (style guides, design patterns, etc)? Luckily, findbugs makes it fairly simple to add additional detectors, and it’s even possible to include a set of custom detectors as part of an existing android project to be run alongside the built in detectors whenever the project is tested by hand or CI server.

A complete, buildable Android project with the detectors built in this post is online here if you wish to use it as an implementation reference or just follow along.

The use case

In Java 7, the try-with-resources pattern was added. This allowed for the declaration of resources as part of the try() header that would be automatically closed when the block exited, either normally or with an exception. This is very convenient when dealing with files, sockets, database cursors or other objects that must be closed when you are finished with them. Naturally, you’ll want to implement this everywhere you use Cursors in Android, because it’s a nice simple way to avoid leaking them:

try (Cursor c = db.query(...)) {
    c.moveToFirst();
    while (!c.isAfterLast()) {
        return new Foo(
                c.getString(c.getColumnIndex(...))
                ...
        );
    }
}

Alas, if you really want to target as many Android users as possible, you inevitably have to make sacrifices for compatibility. One of them is that try-with-resources requires API 19, which cuts off the small but not wholly insignificant ~10% (at time of writing) of users on API 18 and older. There is a workaround, however, which is to use an explicit finally block to close your cursors:

Cursor c = db.query(...);
try {
    c.moveToFirst();
    while (!c.isAfterLast()) {
        return new Foo(
                c.getString(c.getColumnIndex(...))
                ...
        );
    }
} finally {
    c.close();
}

This achieves the same result, albeit with slightly more lines. But good enough. Now the issue we want to address is that every now and then, someone is going to forget to wrap their cursor operations, and leak one. javac won’t catch it, android lint will only catch it sometimes, and neither really care about using try-with-resources. So let’s implement our own detector for findbugs that does! As acceptance criteria, let’s say that our detector needs to be able to

Detect cursor operations (other than Cursor#close) that are called outside of a try {} block
Detect try/catch blocks in methods that open cursors that do not close the cursor as part of their cleanup routine.

Detective work

First things first, we need to create a new detector. In order to make it easier to bundle my detectors with my app, I’ve added them to a module in the same project tree as can be seen here. The key ingredients are three files:

findbugs.xml: Your main plugin definition. This declares your plugin’s package, which classes within it are detectors, and which bugs they can be expected to surface.
messages.xml: A collection of strings that describe your plugin, detectors and each bug instance that you can raise.
A detector! This should be a class that extends either the OpcodeStackDetector or BytecodeScanningDetector.

For this detector, we’ll extend the more basic BytecodeScanningDetector since we don’t really need to fuss with the stack at all. Since at this point we don’t really know that much about how we’ll actually write this detector, the best first thing to do is take a look at how the bytecode for the cases we care about is structured. So let’s code up a quick ‘detector’ that just prints out the code for methods that involve cursors:

public class TestDetector BytecodeScanningDetector {

    private static final String ANDROID_CURSOR = "Landroid/database/Cursor;";

    @Override
    public void visitMethod(Method method) {
        super.visitMethod(method);

        // Fetch the table of local variables for this new method
        LocalVariableTable localVariableTable = method.getLocalVariableTable();

        // If any of the local variables in this method are of the type Cursor,
        // then let's print a dump of the method's bytecode.
        if (variableTableContainsType(localVariableTable, ANDROID_CURSOR)) {
            System.out.println(method.getCode());
        }
    }

    /**
     * Simple method that iterates over a variable table and returns whether or
     * not any of the entries have the specified type signature.
     * @param table Local variable table
     * @param type Java class type we're searching for
     * @return True if any of the local variables are of class type
     */
    private static boolean variableTableContainsType(LocalVariableTable table, String type) {
        for (LocalVariable variable : table.getLocalVariableTable()) {
            if (type.equals(variable.getSignature())) {
                return true;
            }
        }
        return false;
    }

}

And in order to get a more readable output, let’s create a dummy test method that assumes it has and then closes a cursor, with some string literals to help us pinpoint operations:

public void tryFinallyExample() {
    Cursor c = null;
    System.out.println("Before");
    try {
        System.out.println("Try");
    } finally {
        System.out.println("Finally");
        c.close();
    }
    System.out.println("After");
}

Now let’s assemble both our project and our ‘detector’ and then run the detector (outside of gradle, since gradle will swallow our System.out debugging lines)

ross@mjolnir:/h/r/P/A/ExampleFindbugs$ ./gradlew :app:assembleDebug :findbugs:assemble
[...]
ross@mjolnir:/h/r/P/A/ExampleFindbugs$ findbugs \
    -pluginList ./findbugs/build/libs/findbugs.jar \  # Our compiled 'detector'
    -effort:max \
    ./app/build/intermediates/classes/debug/com/schlaikjer/findbugs/database/LeakyDatabase.class

Then as output we find the following:

Code(max_stack = 2, max_locals = 3, code_length = 61)
0:    aconst_null
1:    astore_1
2:    getstatic         java.lang.System.out:Ljava/io/PrintStream; (36)
5:    ldc               "Before" (37)
7:    invokevirtual     java.io.PrintStream.println:(Ljava/lang/String;)V (38)
10:   getstatic         java.lang.System.out:Ljava/io/PrintStream; (36)
13:   ldc               "Try" (39)
15:   invokevirtual     java.io.PrintStream.println:(Ljava/lang/String;)V (38)
18:   getstatic         java.lang.System.out:Ljava/io/PrintStream; (36)
21:   ldc               "Finally" (40)
23:   invokevirtual     java.io.PrintStream.println:(Ljava/lang/String;)V (38)
26:   aload_1
27:   invokeinterface   android.database.Cursor.close:()V (35)  1   0
32:   goto              #52
35:   astore_2
36:   getstatic         java.lang.System.out:Ljava/io/PrintStream; (36)
39:   ldc               "Finally" (40)
41:   invokevirtual     java.io.PrintStream.println:(Ljava/lang/String;)V (38)
44:   aload_1
45:   invokeinterface   android.database.Cursor.close:()V (35)  1   0
50:   aload_2
51:   athrow
52:   getstatic         java.lang.System.out:Ljava/io/PrintStream; (36)
55:   ldc               "After" (41)
57:   invokevirtual     java.io.PrintStream.println:(Ljava/lang/String;)V (38)
60:   return

Exception handler(s) =
From    To  Handler Type
10  18  35  <Any exception>(0)

Attribute(s) =
LineNumber(0, 74), LineNumber(2, 75), LineNumber(10, 77), LineNumber(18, 79),
LineNumber(26, 80), LineNumber(32, 81), LineNumber(35, 79), LineNumber(44, 80),
LineNumber(52, 82), LineNumber(60, 83)
LocalVariable(start_pc = 0, length = 61, index = 0:com.schlaikjer.findbugs.database.LeakyDatabase this)
LocalVariable(start_pc = 2, length = 59, index = 1:android.database.Cursor c)
StackMap(
    (
        FULL, offset delta=35,
         locals={
            (type=Object, class=com.schlaikjer.findbugs.database.LeakyDatabase),
            (type=Object, class=android.database.Cursor)
        },
        stack items={
            (type=Object, class=java.lang.Throwable)
        }
    ),
    (
        SAME,
        offset delta=16
    )
)

Looking at the code dump, we learn something interesting about how the try ... finally block has been implemented at the bytecode level. The body of our finally appears in two places - once at the end of the contents of the try block at opcode 18, and once at opcode 36. So rather than having a single finally label and jumping to it both when an exception is thrown and when one isn’t, the two paths exist separately. If the try block exists normally, then control flows through the finally copy in codes 18-32, after which it jumps to the ‘After’ code we added at code 52 and exits.

If, however, an exception is thrown, then the source is checked againts the exception table for the method. We have one entry, for any exception type, that covers codes 10-18 and has a handler located at code 58. Codes 10-18 (not inclusive) are our try block, so this adds up. Code 35 is just after the jump to ‘After’ that would have ended the method in the no-exception case, and is the start of our exception handling routine. In this routine, we can see that the first thing we do is astore_2: take the topmost value from the operand stack and store in local variable 2. If we look at the StackMap dump at the end, we can see that there’s an entry for this section - one stack item, of type Throwable. So far, so good. We then call the same finally block code that was called in the other branch, but afterwards we then perform the re-throwing of the exception by loading it back onto the stack (aload_2, code 50) and throwing it (athrow, 51)

Bytecode wrangling

OK, now that we have an idea of what our try structure looks like as bytecode and the data we have available to us at detector runtime, let’s take a look at how we can meet the criteria we set out earlier. First, let’s tackle the easier case where a cursor method is called while we’re outside of a try block.

So firstly, we want to be able to know if a given instruction is a method call. Conveniently, our BytecodeScanningDetector extends the DismantleBytecode class, which at each opcode decodes the instruction as well as any arguments and makes them readily accessible. This means that in order to check if we’re at a method call on a cursor, we need only do the following for each opcode we see:

private static final String ANDROID_CURSOR_CLASS_CONST_OPERAND = "android/database/Cursor";

private void checkIfCursorMethodsCalledOutsideTry(int seen) {
    // Not a method call, return
    if (!isMethodCall()) {
        return;
    }

    // If the method is not being called on a cursor, return
    if (!ANDROID_CURSOR_CLASS_CONST_OPERAND.equals(getClassConstantOperand())) {
        return;
    }

    // Figure out try block later
}

Now that we can know if we’re at a call to an instance method of a cursor, we need to be able to check if the call is happening inside of a try. Luckily, we can use the info encoded in the ‘Exception handlers’ section of the code dump above to help us out. We can see that we have one handler registered, which covers codes 10-18 and has a handler method at code 35. Since it looks like bytecode indexes 10-18 are the body of the try block, we can easily use the offsets from the exception table to find out if a given program counter index is inside a try block or not! So let’s translate that logic to code:

private static boolean isInTryBlock(Method method, int pc) {
    CodeException[] exceptionTable = method.getCode().getExceptionTable();
    for (CodeException exception : exceptionTable) {
        if (exception.getStartPC() <= pc && pc < exception.getEndPC()) {
            return true;
        }
    }

    return false;
}

and update our detector method from before:

private static final String ANDROID_CURSOR_CLASS_CONST_OPERAND = "android/database/Cursor";
private static final String CLOSE = "close";

private void checkIfCursorMethodsCalledOutsideTry(int seen) {
    // Not a method call, return
    if (!isMethodCall()) {
        return;
    }

    // If the method is not being called on a cursor, return
    if (!ANDROID_CURSOR_CLASS_CONST_OPERAND.equals(getClassConstantOperand())) {
        return;
    }

    // If a method is called on a cursor outside a try block, and that method is not
    // close, that's an error!
    if (!CLOSE.equals(getNameConstantOperand()) && !isInTryBlock(getMethod(), getPC())) {
        System.out.println("Cursor." + getNameConstantOperand() + " called outside of try block!");
    }
}

Excellent! Now all we need to do is pass the bug info up into findbugs so it can be processed and displayed with other detector output. This can be done with a quick snippet:

bugAccumulator.accumulateBug(
        new BugInstance(
                this,
                "DB_CURSOR_METHODS_CALLED_OUTSIDE_TRY",
                HIGH_PRIORITY
        ).addClassAndMethod(this),
        this
);

Word of warning: the detector is highly stateful! When sawOpcode is called, all of the isMethodCall() / etc. checks, and the line numbers recorded by the above accumulateBug call refer to the current opcode. If you, e.g., have a bug case where you mark the start of a possible bug at one opcode and confirm it at a later point, accumulating the bug at the second location will report it as though that is where it occurred!

That detector wasn’t too bad at all. Let’s move on to our second goal - detecting try {} blocks in methods with cursors that don’t close those cursors. In order to check whether we are operating inside a finally block, we’re going to use a similar trick for checking if we’re in a try block. This time we can’t just use the provided numbers though, since the table only has the entry point for the handler. That’s OK though, since we know that the handler will have to end in either a goto, areturn or athrow.

/**
 * Finally blocks are defined as the PCs between the handler PC and the next call to athrow,
 * goto or return.
 *
 * @param method
 * @param pc
 * @return
 */
private int getFinallyBlockIndex(Method method, int pc) {
    CodeException[] exceptionTable = method.getCode().getExceptionTable();
    int blockIndex = 0;
    for (CodeException exception : exceptionTable) {
        if (exception.getHandlerPC() <= pc) {
            int pc2 = pc;
            int codeByte;
            while ((codeByte = getCodeByte(pc2)) != ATHROW && codeByte != ARETURN && codeByte != GOTO && pc2 < getMaxPC()) {
                pc2++;
            }
            if ((codeByte == ATHROW || codeByte == ARETURN || codeByte == GOTO) && pc < pc2) {
                return blockIndex;
            }
        }
        blockIndex++;
    }

    return -1;
}

Not the best runtime complexity to be calling this all the time, but good enough for now. This detection method is going to be a bit more stateful - for each opcode, we’re going to check if we’re in a finally block, and if so:

Add an entry to our local log of the start of the block
Check if the current opcode in the block is a method call
- If it is, check if it’s on a cursor
  - If so, check if it’s a close
    - If it is, mark this finally block as good
    - If it’s not, then this block might be leaky!

So let’s port that logic over:

private final Map<String, FinallyInfo> suspectFinallys = new HashMap<>();

private static class FinallyInfo {
    boolean callsCursorClose;
    BugInstance bugInstance;

    public FinallyInfo(BugInstance instance) {
        this.callsCursorClose = false;
        this.bugInstance = instance;
    }
}

private void checkExceptionHandlersCloseCursors(int seen) {
    // Check if we're in a finally block
    int blockIndex = getFinallyBlockIndex(getMethod(), getPC());
    if (blockIndex < 0) {
        return;
    }

    // We create a bug instance immediately on entering *all* finally blocks;
    // this is just so that we get the line numbers in the right place.
    // If the finally block does close the cursor, we just toss the buginstance
    String finallyReference = getMethodName() + blockIndex;
    if (!suspectFinallys.containsKey(finallyReference)) {
        suspectFinallys.put(finallyReference, new FinallyInfo(new BugInstance(
                this,
                "DB_CURSOR_NOT_FINALLY_CLOSED",
                HIGH_PRIORITY
        ).addClassAndMethod(this)));
    }

    // Not a method call, return
    if (!isMethodCall()) {
        return;
    }

    // If the method is not being called on a cursor, return
    if (!ANDROID_CURSOR_CLASS_CONST_OPERAND.equals(getClassConstantOperand())) {
        return;
    }

    // If the method isn't close, return
    if (!CLOSE.equals(getNameConstantOperand())) {
        return;
    }

    // Mark this finally block as OK
    suspectFinallys.get(finallyReference).callsCursorClose = true;
}

And that’s pretty much it! There’s a little bit of extra accounting that is necessary to actually finish up and report all the bugs accumulated in that fashion, which can be found here for those curious. Now that we have these detectors set up, the next time we run this project through our CI system (in this case Jenkins), we should see it error out with our expected bug instances:

Perfect! No more unclosed cursors. From the basic ideas here, it should be possible to add checks on more or less any code pattern that you want to make sure to implement or avoid in your production code.