We’ve posted previously on some of the differences between Findbugs’ open source Java analysis and commercial Java static analysis tools, specifically on the JSR-305 specification and source code versus byte code analysis topics. Due to these differences, many Java shops will use a commercial Java static analysis tool in conjunction with Findbugs to make sure they’re getting as complete issue detection as possible.

One area that’s been discussed previously is the ability to identify situations of possible null pointer dereference. This peaked my interest and led me to do some benchmarking against a few open source projects to assess the Findbugs analysis on intra- and inter-procedural possible null pointer dereference issues.

The normal assumption is that Findbugs is strong with intra-procedural analysis but unable to provide inter-procedural Java analysis. Here is an inter-procedural example from an open source project called hsqldb:

    protected void directRefreshTree() {

         int[]                  rowCounts;

        ...

            try {

                rowCounts = getRowCounts(tables, schemas);

            } catch (Exception e) {

                 //  Added: (weconsultants)@users

               CommonSwing.errorMessage(e);

            }

            ResultSet col;

            // For each table, build a tree node with interesting info

            for (int i = 0; i < tables.size(); i++) {

                col = null;

                String name;

                try {

                    name   = (String) tables.elementAt(i);

                    if (isOracle && name.startsWith("BIN$")) {

                        continue;

                        // Oracle Recyle Bin tables.

                        // Contains metacharacters which screw up metadata

                        // queries below.

                    }

                    schema = (String) schemas.elementAt(i);

                    String schemaname = "";

                    if (schema != null && showSchemas) {

                        schemaname = schema + '.';

                    }

                    String rowcount = displayRowCounts

                                      ? (" " + DECFMT.format(rowCounts[i]))

                                      : "";

    ...

    }

In the first try block getRowCounts() can have a null value.  How you ask?

    protected int[] getRowCounts(Vector inTable,

                                 Vector inSchema) throws Exception {

        if (!displayRowCounts) {

            return (null);

        }

      ...

    }

Okay, so automatically finding these issues is not going to happen with Findbugs.  I understand that, and that is why there are commercial tools to help with that.

But what really surprised me are the intra-procedural examples that are missed.  Again using examples from hsqldb and a specific class called TransferDb, Findbugs finds one issue. But there are clearly more intra-procedural issues that you would think Findbugs would have found.  Let’s take a look:

    TransferResultSet getData(String statement)

    throws DataAccessPointException {

        ResultSet rsData = null;

        try {

            if (srcStatement != null) {

                srcStatement.close();

            }

            srcStatement = conn.createStatement();

            rsData       = srcStatement.executeQuery(statement);

        } catch (SQLException e) {

            try {

                srcStatement.close();

            } catch (Exception e1) {}

            srcStatement = null;

            rsData       = null;

            throw new DataAccessPointException(e.getMessage());

        }

        return new TransferResultSet(rsData);

    }

This is a conditional situation where “srcStatement” could have a null value (the conditional is false), then you throw an exception with “createStatement()” and dereference later with “srcStatement.close()”.  Why does Findbugs miss this issue?  There are several issues of this type.

Or another situation where you throw an exception with “createStatement()” again.  This example spans multiple lines so you will find it in an attachment.  Here “select_rs” has a value of null and is clearly dereferenced much later on.

…

       ResultSet         col            = null;

        int               colnum         = 1;

  Statement         stmt           = null;

Source: null here ->  ResultSet         select_rs      = null;

        ResultSetMetaData select_rsmdata = null;

         try {

Exception throw here -> stmt           = conn.createStatement();

            select_rs      = stmt.executeQuery(TTable.Stmts.sSourceSelect);

            select_rsmdata = select_rs.getMetaData();

            col = meta.getColumns(TTable.Stmts.sDatabaseToConvert,

                                  TTable.Stmts.sSchema,

                                  TTable.Stmts.sSourceTable, null);

        } catch (SQLException eSchema) {

            // fredt - second try with null schema

            if (TTable.Stmts.sSchema.equals("")) {

                try {

                    col = meta.getColumns(TTable.Stmts.sDatabaseToConvert,

                                          null, TTable.Stmts.sSourceTable,

                                          null);

                } catch (SQLException eSchema1) {}

            }

        }

        try {

            while (col.next()) {

                String name = Dest.helper.formatIdentifier(col.getString(4));

                int    type        = col.getShort(5);

                String source      = col.getString(6);

                int    column_size = col.getInt(7);

                String DefaultVal  = col.getString(13);

                boolean rsmdata_NoNulls =

                    (select_rsmdata.isNullable(colnum)

                     == java.sql.DatabaseMetaData.columnNoNulls);

                boolean rsmdata_isAutoIncrement = false;

                try {

                    rsmdata_isAutoIncrement =

                        select_rsmdata.isAutoIncrement(colnum);

                } catch (SQLException e) {

                    rsmdata_isAutoIncrement = false;

                }

                int rsmdata_precision = select_rsmdata.getPrecision(colnum);

                int rsmdata_scale     = select_rsmdata.getScale(colnum);

                type = helper.convertFromType(type);

                type = Dest.helper.convertToType(type);

                Integer inttype  = new Integer(type);

                String  datatype = (String) TTable.hTypes.get(inttype);

                if (datatype == null) {

                    datatype = source;

                    tracer.trace("No mapping for type: " + name + " type: "

                                 + type + " source: " + source);

                }

                if (type == Types.NUMERIC) {

                    datatype += "(" + Integer.toString(rsmdata_precision);

                    if (rsmdata_scale > 0) {

                        datatype += "," + Integer.toString(rsmdata_scale);

                    }

                    datatype += ")";

                } else if (type == Types.CHAR) {

                    datatype += "(" + Integer.toString(column_size) + ")";

                } else if (rsmdata_isAutoIncrement) {

                    datatype = "SERIAL";

                }

                if (DefaultVal != null) {

                    if (type == Types.CHAR || type == Types.VARCHAR

                            || type == Types.LONGVARCHAR

                            || type == Types.BINARY || type == Types.DATE

                            || type == Types.TIME

                            || type == Types.TIMESTAMP) {

                        DefaultVal = "\'" + DefaultVal + "\'";

                    }

                    datatype += " DEFAULT " + DefaultVal;

                }

                if (rsmdata_NoNulls) {

                    datatype += " NOT NULL ";

                }

                v.addElement(inttype);

                datatype = helper.fixupColumnDefRead(TTable, select_rsmdata,

                                                     datatype, col, colnum);

                datatype = Dest.helper.fixupColumnDefWrite(TTable,

                        select_rsmdata, datatype, col, colnum);

                create += name + " " + datatype + ",";

                insert += "?,";

                colnum++;

            }

Sink: dereference here-> select_rs.close();

            stmt.close();

            col.close();

        } catch (SQLException e) {

            throw new DataAccessPointException(e.getMessage());

        }

So I’m a little confused as to why these issues are not highlighted.  It seems that this is the exact type of analysis Findbugs does.  Maybe it has to do with not recognizing the possible exceptions?  Does anyone have any ideas?

JSR-305 is a Java Specification Request intended to improve the effectiveness of static analysis tools operating in Java 5+ environments. The idea here is that one can use special purpose annotations in order to provide static analysis tools with hints regarding the behaviour and side effects of methods.

An example of such annotations can be found in the presentation ‘Annotations for Software Defect Detection’ by William Pugh, who is masterminding the whole spec. Here we go:

 1: void test() {
 2:    if (spec != null) fFragments.add(spec);
 3:    if (isComplete(spec)) fPreferences.add(spec);
 4: }
 6:
 5: boolean isComplete(AnnotationPreferences spec) {
 6:    return spec.getColorPreferenceKey() != null
 7:        && spec.getColorPreferenceValue() != null
 8:        && spec.getTextPreferenceKey() != null
 9:        && spec.getOverviewRulerPreferenceKey() != null;
10: }

What’s wrong with the snippet above? Well, the check for null on line 2 shows that the developer expects that the value of ‘spec’ can potentially be null, but it is still passed to method ‘isComplete’. Later, ‘isComplete’ attempts to dereference the value, which causes a NullPointerException.

According to Dr. Pugh, the best way to detect this issue statically is to force a developer to add the annotation @Nonnull to the method signature like this:

5: boolean isComplete(@Nonnull AnnotationPreferences spec) {

In this way, a basic static analysis tool that can minimally track ‘spec’ as a potential suspect for ‘null’ can issue a warning when the @Nonnull annotation is contradicted (that is, when ‘spec’ is passed to ‘isComplete’ as a parameter).

There are two problems with this approach:

• it forces the developer to do work that rightly should be performed by a static analysis engine
• it takes time to write the annotation for static analysis, but it takes even more effort to maintain the annotations and the actual code base in a consistent state.

In reality, the proposals behind JSR-305 exist to enable a tool intended for single function analysis (so-called intra-procedural analysis) to act as if it were performing whole program analysis by requiring the developer to state expected behaviour up front (whether or not that behaviour is actually expressed correctly in the developer’s code).

In contrast, this same scenario is supported by a whole program static analysis tool (so-called inter-procedural analysis) without developer intervention:

1. First, a complete call-graph of the system is built, and then all the methods are ordered so that called methods are processed prior to callers — such an ordering allows the tool to generate all the necessary information about, in this instance, the method ‘isComplete’ by the time the analysis of the method ‘test’ begins.
2. During the analysis of ‘isComplete’, the tool records the fact that the incoming argument ‘spec’ is dereferenced.
3. Next, the method ‘test’ is analyzed. In this method, the variable ‘spec’ is checked for null, so it is tracked as a potential suspect for an exception. Using the information generated about ‘isComplete’ the tool can reliably issue a warning on line 3, since it already knows that ‘isComplete’ dereferences the incoming argument.

So that example applies to a simple unconditional dereference scenario. In more complicated cases, Dr. Pugh proposes to use the annotation parameter ‘when’, with one of the following values:

• ALWAYS

• NEVER

• MAYBE

• UNKNOWN

For example: ‘@Nonnull(when=When.NEVER)’ means that a value is always null in the given context.

This specification seems to be a compromise between the amount of information provided by a developer to a static analysis tool and the amount of effort a developer has to put into it, a compromise that does not seem to be a particularly good solution here. First of all, the amount of information provided in such a manner is insufficient to provide accurate analysis, and secondly this seems to be too much work for the developer, especially when this work can be avoided.

Let’s examine how conditional value dereferencing is supported by whole program static analysis tools:

 1: void test() {
 2:     entity.qualifiedName = null;
 3:     saveName(entity, false);
 4: }
 5:
 6: boolean    saveName(Entity entity, boolean qualified) {
 7:    String name;
 8:    if (qualified)
 9:        name = entity.qualifiedName.trim();
10:    else
11:        name = entity.name.trim()
12:
13:     save(name);
14: }

In this example, an inter-procedural static analysis tool would first analyze the method ‘saveName’. A good analysis engine should be able to record the fact that this method only dereferences ‘entity.qualifiedName’ if the second parameter, ‘qualified’, is set to ‘true’. This, it would appear, is a deal more detailed than one can practically achieve by adding @Nonnull(when=When.XXX) annotations, even with all the work the annotation implies for the developer.

Next, the method ‘test’ would be analyzed. A good static analysis tool will naturally keep track of ‘entity.qualifiedName’ because of the assignment to ‘null’ on line 2 and its therefore potential use in an exception causing context. However, given that the actual call to ‘saveName’ on line 3 uses ‘false’ as its second argument, such a tool will not issue a warning that would in reality be a false positive, since the knowledge gained from analyzing ‘saveName’ disqualifies any potential warning due to the conditional relationship between arguments.

In summary, JSR-305 proposes a whole roster of interesting ideas for using annotations to enhance static analysis of Java code, and NPE detection seems to be only one aspect of this specification request. In upcoming blog posts, we shall continue the discussion of proposed annotations as well as offering our own ideas about how and when annotations should be used in static analysis.

David posted an interesting discussion on the usage of static analysis tools by developers to find security vulnerabilities.  As always the discussion with static analysis tools lean towards the false positive and false negative discussion.  But also David mentions their results are sometimes difficult to understand.  

This is one of the reasons Klocwork switched from a bytecode analysis tool for Java to a source code analysis tool.  As both have their advantages and disadvantages (and I admit I’m very biased here) we have certainly found that we have been able to reduce our false positive rates, find more issues (not to mention add new issues quickly) and provide more details on the results.

The reason for this is that the bytecode started to get noisy in Java 5 and even more so in Java 6.  Specifically one of the great advantages that static analysis tools have today is being able to show you the details of any issue it finds.  For example

public class Source {
     public static Source getInstance() { return null; }
     public int getValue() { return 32; }
     public Source() { getInstance().getValue(); }
}

This is a Null Pointer Exception issue. Although this is a very small example it is an inter-procedural issue and something that is missed by other defect detection tools. The important thing here is to help the developer understand this issue by tracing through the paths that make this happen.  In other words you need to know that getInstance() has the potential to return null and what paths bring us to the actual error.  This can get quite complex where it starts to not only span the same class but different methods in different classes.

Because of this extra noise this could be part of the reason your seeing new Java annotations being introduced.   Many of you may be familiar with the Java annotation that were introduced in Java 5.  This was part of a specification called JSR-250.  There is a new specification called JSR 305 for the purposes of allowing annotations to help assist tools that detect software defects.  Interestingly enough it is Findbugs leading the charge to add this in for Java 7.  Interesting to see if this specification happens.