As a Java Architect at Sun Microsystems, Carol McDonald has spoken at various conferences including JavaOne, Java University, Sun Tech Days, Sun Network, Sun Code Camps, JDJEdge, and JUGs including Machester, Boston, Maine, Cologne, FAA, Richmond, Memphis, D.C... Carol blogs about the latest technologies that she is speaking about at http://weblogs.java.net/blog/caroljmcdonald/. Before returning to Sun in 2007, Carol worked 2 1/2 yrs as an Architect on massive OLTP Spring/hibernate application to manage > 10 mill loans for the consumer credit division of a leading automoblile manufacturer and a leading bank. Before joining Sun the first time in 1999 Carol worked on Pharmaceutical Intranet applications for Roche in Switzerland, a Telecom Network Management Application for Digital (now HP) in France, a X.400 Email Server for IBM in Germany, and as a student intern for the National Security Agency. Carol holds a M.S. in Computer Science from the University of Tennessee, a B.S. in Geology from Vanderbilt University, and is a Sun Certified Java Architect and Java Language Programmer. Carol is also Fluent in French and German. Carol has posted 11 posts at DZone. View Full User Profile

Finding bugs that matter with Findbugs

12.21.2011
| 1920 views |
  • submit to reddit

What is FindBugs?

•     FindBugs is an open source static analysis tool that analyzes Java class files

•     http://findbugs.sourceforge.net/

•     Developed from Research at University of Maryland, led by Bill Pugh

•     Looks for  for programming defects based on  ~ 300 different bug patterns from real bugs

•     bug patterns are grouped into categories: correctness, bad practice, performance…

•     each pattern assigned a priority: high, medium or low.

•     High-Medium priority have low false positive rates: effort was put into trying to ensure that issues reported as high or medium priority correctness issues were issues that had a low false positive rate, and that developers would be interested in examining all issues reported as high or medium priority correctness issues, even in large code bases.

             

FindBugs looks for  Bugs  based on real bug Patterns:

•      broad and common patterns:

•      a read or write on a null pointer

•      typos

•      Methods whose return value should not be ignored

•      Also specific bug patterns:

•      Every Programming Puzzler

•      Eclipse documented bug fixes

•      Every chapter in Effective Java

•      lots from Worse than failure : http://thedailywtf.com/

 

 

Some Bug Patterns:

 

CN

Cloneable Not Implemented Correctly

DC

Double Checked Locking

DE

Dropped Exception

EC

Suspicious Equals Comparison

Eq

Bad Covariant Definition of Equals

HE

Equal objects must have equal hashcodes

IS2

Inconsistent Synchronization

MS

Static Field Modifiable By Untrusted Code

NP

Null Pointer Dereference

NS

Non-Short-Circuit Boolean Operator

OS

Open Stream

RCN

Redundant Comparison to Null

RR

Read Return Should Be Checked

RV

Return Value Should Be Checked

Se

Non-serializable Serializable Class

UR

Uninitialized Read In Constructor

UW

Unconditional Wait

Wa

Wait Not In Loop

 

 

 

 

Misconceptions about Bugs:

 

•     Programmers are smart

•     Smart people don’t make dumb mistakes

•     WRONG!

•     Smart people make dumb mistakes

•     Common errors:

•     wrong boolean operator, forgetting parentheses, etc. Misunderstood class or method  !

 

Who uses Findbugs?

•     Developed from Research at University of Maryland

•     Google, Ebay, Sun, Wells Fargo…

•     Bill Pugh , Professor from University of Maryland, spent a year sabbatical at Google  working Findbugs  into their development process:

•     Google runs FindBugs over all Java code

•     1800s issues identified, > 600 fixed.

•     Ebay found 2 developers reviewing  Findbugs  was 10 times more effective than 2 testers

 

 

Some Bug Categories

• Correctness - the code is doing something wrong, you should look at it

• Bad practice - the code violates good practice

•      Dodgy Code

•      Concurrency

•      Performance

• Security defect

 

 

Can you find the Bug below ?

 

public String sendMessage (User user, String body, Date time) {

    return sendMessage(user, body, null);

  }

 

public String sendMessage (User user, String body, Date time, List attachments) {

   String xml = buildXML (body, attachments);

   String response = sendMessage(user, xml);

   return response;

  }

 

This line causes an Infinite recursive loop. This bug is  high priority,  in the correctness category.

return sendMessage(user, body, null);

 

 

Can you find the Bug below ?

 

public String foundType() {

   return this.foundType();

}

 

Should be

public String foundType() {

   return this.foundType;

}

 

This is another example of an infinite recursive loop. This bug is found often  in a decorator pattern when you forget to delegate to another method

 Findbugs found 5 infinite recursive loops in  JDK1.6.0-b13.  Including this one written by Joshua Bloch

• Smart people make dumb mistakes

• 27 across all versions of JDK, 31 in Google’s Java code

• Embrace and fix your dumb mistakes!

 

 

Can you find the Bug?

if (name != null || name.length > 0)

 

Should be

if (name != null && name.length > 0)

this null pointer bug was found in com.sun.corba…

 

 

Can you find the Bug?
private final String _lock = "LOCK";
...
synchronized(_lock)
{
...
}

Constant Strings are shared (even private ones) across all other classes loaded by the JVM. This could lead to unexpected deadlocks in conjunction with other code. This bug was found in Jetty.

 

 

This null pointer bug was in Eclipse since  3.2:

if (adapters == null && adapters.length == 0)

     return;

 

•example of  statement or branche that if executed guarantee that a null pointer exception will occur

but in this case adapters is probably never null, that’s why it didn’t get noticed.

•      some mistakes don’t matter much, because  the impact of the mistake is minimal.

• null pointer exceptions usually get noticed. A harder error to find is that it won’t return if the length is 0.

 

Can you find the bug?

try { ... }

catch (IOException e) {

  new SAXException("Server side Exception:" + e);

}

 

The Exception is created and dropped rather than thrown, Should be :

  throw new SAXException("Server side Exception:" + e);

 

 

Can you find the Bug?

public static String getNameById(String userId) {

    String str = userId;

 

    str.replace(' ', '_');

 

    return str;

  }

 

Ignores the return value of the replace() method, Should be:

     str= str.replace(' ', '_');

 

A very common mistake is ignoring the return value on methods whose return value shouldn't be ignored.

• Strings are immutable, so functions like trim() and replace() return a new String.

 

 

What does this Print?

 

Integer one = 1;

Long addressTypeCode = 1L;

if (addressTypeCode.equals(one)) {

     System.out.println("equals");

} else {

     System.out.println("not equals");

}

 

It prints "not equals". According to the contract of equals(), objects of different classes should always compare as unequal

 

Incomparable equality:

 

•      Using .equals to compare incompatible types

•      Using .equals to compare arrays

•      only checks if the same array

•      Checking to see if a Set<Long> contains an Integer

•      never found, even if the same integral value is contained in the map

•      Calling get(String) on a Map<Integer,String>

•      Returns false , not an error

     These bugs may be hard to find on your own

•      Types not always explicit

•      May be introduced by refactoring

            For example a Google refactoring changed a method to return byte[ ] rather than String

 

 

Bugs that Matter…

You can have mistakes which don't actually cause problems. Bugs that matter depend on the context.  Not every bug found matters, but the effective use of Findbugs can remove bugs cheaper than with other methods

 

What is the best way to use Findbugs?

You want to find an effective way to use static analysis to improve software quality. 5-10% of mistakes can be found with Findbugs, for some types of bugs it can find all of them.  Testing and running code in production will find the bugs that matter, the bugs that cause problems .  If you run Findbugs on code in production , then a lot of bugs have been fixed, if you run on new code your are more likely to find bugs. The reason to use findbugs is not to fix all your quality problems, but rather for the issues it does find,  finding and fixing with Findbugs is cheaper than finding with testing or causing a problem in production.

•      Use Findbugs on new code to remove bugs cheaply before bugs are detected using more expensive techniques

•      The best time to have a developer  look at a warning is when it is introduced, while the code is fresh in a developer’s head.

 

Expensive Mistakes are …

•      Mistakes that fail silently

•      Bugs that silently cause the wrong answer to be computed are the scariest bugs, because they will not be noticed easily .

•      Mistakes that cause loss of money when they occur

•      Mistakes that are hard to fix

Mistakes that matter ($$)  can be found by testing or eventually in production,  but it is cost effective to find bugs  before testing and production with findbugs.

 

 

Runtime exceptions can be your friend… runtime exceptions can point you right where the error is

If a mistake causes a runtime exception, then the mistake will be found  and corrected .

Throwing a runtime exception is often a reasonable way to fail safely and report a failure.

Runtime exceptions represent conditions that reflect errors in your program's logic and cannot be reasonably recovered from

 

 

Can you find the Bug?

// calculate DR amount by aggregating CR amounts

BigDecimal drAmount = new BigDecimal(0);

for (JournalEntry je: journalEntries)

     drAmount.add(je.getCrAmount());

     // persist to db

     getTrxnService().saveJournalEntry(id,

        drAmount, // aggregated amount

        true, // Debit

        "USD",

        "Revenue");

 

 

Ignores return value of BigDecimal.add(), should be

 

 drAmount= drAmount.add(je.getCrAmount());

 

This bug would have cost $ in production, but fixed at Google, within 30 minutes of being reported.

 

Can you find the Bug?

int value2;

Public boolean equals(Integer value1){

  return value1== intValue() ;

}

public Integer intValue() {

  return value2;

}

 

Uses reference equality rather than .equals, should be:

Public boolean equals(Integer value1){

  return value1.equals(intValue() );

}

 

For boxed primitives, == and != are computed using pointer equality, but <, <=, >, >= are computed by comparing unboxed primitive values. This can bite you on other classes (e.g., String), but boxed primitives is where people get bit

 

 

Concurrency Bugs

•      Inconsistent synchronization –

•      a lock is held   sometimes when field accessed

•      Problems with wait/notify –

•      e.g., call to wait() not in loop

•      unsafe lazy initialization of static field

 

 

Can you find the bug?

synchronized (object) {

  if (<condition does not hold>) {

    object.wait();

  }

  // Proceed when condition holds

}

 

Should be:

synchronized (object) {

  while (<condition does not hold>) {

    object.wait();

  }

  // Proceed when condition holds

}

 

Use Java 5!  simplified concurrency , less likely to make these mistakes.

 

 

Running FindBugs

•      Eclipse plugin

            http://findbugs.sourceforge.net/manual/eclipse.html

•      Run with Hudson build

 

References:

•      Findbugs home page

            http://findbugs.sourceforge.net/

•      Bill Pugh Findbugs Devoxx talk

            http://www.parleys.com/#id=2106&st=5

•      Bill Pugh Oredev talk:

            http://oredev.org/2010/sessions/defective-java-mistakes-that-matter

             

0
Published at DZone with permission of its author, Carol Mcdonald.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Tags: