FindBugs looks only for the most blatant, obvious cases of HTTP response splitting. If FindBugs found any, you almost certainly have more vulnerabilities that FindBugs doesn't report. If you are concerned about HTTP response splitting, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of HTTP response splitting. If FindBugs found any, you almost certainly have more vulnerabilities that FindBugs doesn't report. If you are concerned about HTTP response splitting, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of relative path traversal. If FindBugs found any, you almost certainly have more vulnerabilities that FindBugs doesn't report. If you are concerned about relative path traversal, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of absolute path traversal. If FindBugs found any, you almost certainly have more vulnerabilities that FindBugs doesn't report. If you are concerned about absolute path traversal, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of cross site scripting. If FindBugs found any, you almost certainly have more cross site scripting vulnerabilities that FindBugs doesn't report. If you are concerned about cross site scripting, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of cross site scripting. If FindBugs found any, you almost certainly have more cross site scripting vulnerabilities that FindBugs doesn't report. If you are concerned about cross site scripting, you should seriously consider using a commercial static analysis or pen-testing tool.

FindBugs looks only for the most blatant, obvious cases of cross site scripting. If FindBugs found any, you almost certainly have more cross site scripting vulnerabilities that FindBugs doesn't report. If you are concerned about cross site scripting, you should seriously consider using a commercial static analysis or pen-testing tool.

Number[] arr = new Integer[10]; arr[0] = 1.0;

Consider changing the type of created array or the field type.

Number[] arr = new Integer[10]; arr[0] = 1.0;

Consider changing the type of created array or the local variable type.

Consider changing the type of created array or the method return type.

If all clone() methods call super.clone(), then they are guaranteed to use Object.clone(), which always returns an object of the correct type.

In the past, situations where people have explicitly invoked the garbage collector in routines such as close or finalize methods has led to huge performance black holes. Garbage collection can be expensive. Any situation that forces hundreds or thousands of garbage collections will bring the machine to a crawl.

Values between -128 and 127 are guaranteed to have corresponding cached instances and using valueOf is approximately 3.5 times faster than using constructor. For values outside the constant range the performance of both styles is the same.

Unless the class must be compatible with JVMs predating Java 1.5, use either autoboxing or the valueOf() method when creating instances of Long, Integer, Short, Character, and Byte.

Unless the class must be compatible with JVMs predating Java 1.5, use either autoboxing or the valueOf() method when creating instances of Double and Float.

• String.toUpperCase( Locale l )
• String.toLowerCase( Locale l )

versions instead.

The argument to nextInt must be positive. If, for example, you want to generate a random value from -99 to 0, use -r.nextInt(100).

To fix this problem consider storing the object into the local variable first and save it to the volatile field only after it's fully constructed.

If a connected set of objects beings finalizable, then the VM will invoke the finalize method on all the finalizable object, possibly at the same time in different threads. Thus, it is a particularly bad idea, in the finalize method for a class X, invoke finalize on objects referenced by X, because they may already be getting finalized in a separate thread.

public boolean equals(Object o) {
  if (o instanceof Foo)
    return name.equals(((Foo)o).name);
  else if (o instanceof String)
    return name.equals(o);
  else return false;

This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive. Without those properties, very unexpected behaviors are possible.

The likely intended semantics are object identity: that an object is equal to itself. This is the behavior inherited from class Object. If you need to override an equals inherited from a different superclass, you can use use:

public boolean equals(Object o) { return this == o; }

If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is:

public int hashCode() {
  assert false : "hashCode not designed";
  return 42; // any arbitrary constant will do
  }

From the JavaDoc for the compareTo method in the Comparable interface:

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is:

public int hashCode() {
  assert false : "hashCode not designed";
  return 42; // any arbitrary constant will do
  }

If you don't want to define a hashCode method, and/or don't believe the object will ever be put into a HashMap/Hashtable, define the hashCode() method to throw UnsupportedOperationException.

• The class contains a mix of locked and unlocked accesses,
• The class is not annotated as javax.annotation.concurrent.NotThreadSafe,
• At least one locked access was performed by one of the class's own methods, and
• The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads

A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.

You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.

Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

This bug does not necessarily indicate an error, since the change to mutable object state may have taken place in a method which then called the method containing the notification.

Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.

See the Java Language Specification for details.

See the Java Language Specification for details.

To make this more concrete, consider the following classes:

abstract class A {
  int hashCode;
  abstract Object getValue();
  A() {
    hashCode = getValue().hashCode();
    }
  }
class B extends A {
  Object value;
  B(Object v) {
    this.value = v;
    }
  Object getValue() {
    return value;
  }
  }

When a B is constructed, the constructor for the A class is invoked before the constructor for B sets value. Thus, when the constructor for A invokes getValue, an uninitialized value is read for value.

public class CircularClassInitialization {
    static class InnerClassSingleton extends CircularClassInitialization {
        static InnerClassSingleton singleton = new InnerClassSingleton();
    }

    static CircularClassInitialization foo = InnerClassSingleton.singleton;
}

private static String LOCK = "LOCK";
...
  synchronized(LOCK) { ...}
...

Constant Strings are interned and shared across all other classes loaded by the JVM. Thus, this code is locking on something that other code might also be locking. This could result in very strange and hard to diagnose blocking and deadlock behavior. See http://www.javalobby.org/java/forums/t96352.html and http://jira.codehaus.org/browse/JETTY-352.

See CERT CON08-J. Do not synchronize on objects that may be reused for more information.

private static Boolean inited = Boolean.FALSE;
...
  synchronized(inited) {
    if (!inited) {
       init();
       inited = Boolean.TRUE;
       }
     }
...

Since there normally exist only two Boolean objects, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock.

See CERT CON08-J. Do not synchronize on objects that may be reused for more information.

private static final Integer fileLock = new Integer(1);
...
  synchronized(fileLock) {
     .. do something ..
     }
...

It would be much better, in this code, to redeclare fileLock as

private static final Object fileLock = new Object();

The existing code might be OK, but it is confusing and a future refactoring, such as the "Remove Boxing" refactoring in IntelliJ, might replace this with the use of an interned Integer object shared throughout the JVM, leading to very confusing behavior and potential deadlock.

private static Integer count = 0;
...
  synchronized(count) {
     count++;
     }
...

Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock.

See CERT CON08-J. Do not synchronize on objects that may be reused for more information.

synchronized() {}

Empty synchronized blocks are far more subtle and hard to use correctly than most people recognize, and empty synchronized blocks are almost never a better solution than less contrived solutions.

• The class contains a mix of locked and unlocked accesses,
• At least one locked access was performed by one of the class's own methods, and
• The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads

A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.

Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

private Long myNtfSeqNbrCounter = new Long(0);
private Long getNotificationSequenceNumber() {
     Long result = null;
     synchronized(myNtfSeqNbrCounter) {
         result = new Long(myNtfSeqNbrCounter.longValue() + 1);
         myNtfSeqNbrCounter = new Long(result.longValue());
     }
     return result;
 }

If you really intend to invoke the inherited method, invoke it by invoking the method on super (e.g., invoke super.foo(17)), and thus it will be clear to other readers of your code and to FindBugs that you want to invoke the inherited method, not the method in the outer class.

If you call this.foo(17), then the inherited method will be invoked. However, since FindBugs only looks at classfiles, it can't tell the difference between an invocation of this.foo(17) and foo(17), it will still complain about a potential ambiguous invocation.

import alpha.Foo;
public class A {
  public int f(Foo x) { return 17; }
}
----
import beta.Foo;
public class B extends A {
  public int f(Foo x) { return 42; }
}

The f(Foo) method defined in class B doesn't override the f(Foo) method defined in class A, because the argument types are Foo's from different packages.

import alpha.Foo;
public class A {
  public int f(Foo x) { return 17; }
}
----
import beta.Foo;
public class B extends A {
  public int f(Foo x) { return 42; }
  public int f(alpha.Foo x) { return 27; }
}

The f(Foo) method defined in class B doesn't override the f(Foo) method defined in class A, because the argument types are Foo's from different packages.

In this case, the subclass does define a method with a signature identical to the method in the superclass, so this is presumably understood. However, such methods are exceptionally confusing. You should strongly consider removing or deprecating the method with the similar but not identical signature.

Because the analysis only looks at the generated bytecode, this warning can be incorrect triggered if the default case is at the end of the switch statement and the switch statement doesn't contain break statements for other cases. ]]>

If possible, making the inner class a static inner class should solve the problem. Making the outer class serializable might also work, but that would mean serializing an instance of the inner class would always also serialize the instance of the outer class, which it often not what you really want. ]]>

We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:

- The method is intended to trigger loading of some class which may have a side effect.

- The method is intended to implicitly throw some obscure exception.

This analysis rarely produces false-positives. Common false-positive cases include:

- This object used to implicitly throw some obscure exception.

- This object used as a stub to generalize the code.

- This object used to hold strong references to weak/soft-referenced objects.

We are guessing that ignoring the return value might be a bad idea just from a simple analysis of the body of the method. You can use a @CheckReturnValue annotation to instruct FindBugs as to whether ignoring the return value of this method is important or acceptable.

Please investigate this closely to decide whether it is OK to ignore the return value.

We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:

- The method is designed to be overridden and produce a side effect in other projects which are out of the scope of the analysis.

- The method is called to trigger the class loading which may have a side effect.

- The method is called just to get some exception.

If you feel that our assumption is incorrect, you can use a @CheckReturnValue annotation to instruct FindBugs that ignoring the return value of this method is acceptable.

String dateString = getHeaderField(name);
dateString.trim();

the programmer seems to be thinking that the trim() method will update the String referenced by dateString. But since Strings are immutable, the trim() function returns a new String value, which is being ignored here. The code should be corrected to:

String dateString = getHeaderField(name);
dateString = dateString.trim();

if (x < 0)
  new IllegalArgumentException("x must be nonnegative");

It was probably the intent of the programmer to throw the created exception:

if (x < 0)
  throw new IllegalArgumentException("x must be nonnegative");

Also note that FindBugs considers the default case of a switch statement to be an exception path, since the default case is often infeasible.

Also note that FindBugs considers the default case of a switch statement to be an exception path, since the default case is often infeasible.

Note that a check such as if (x == null) throw new NullPointerException(); is treated as a dereference of x.

On the other hand, using null to indicate "there is no answer to this question" is probably appropriate. For example, File.listFiles() returns an empty list if given a directory containing no files, and returns null if the file is not a directory.

    if (argv.length == 0) {
    // TODO: handle this case
    }

    if (argv.length == 1);
        System.out.println("Hello, " + argv[0]);

• Both values compared are definitely null
• One value is definitely null and the other is definitely not null

This particular warning generally indicates that a value known not to be null was checked against null. While the check is not necessary, it may simply be a case of defensive programming.

    Lock l = ...;
    l.lock();
    try {
        // do something
    } finally {
        l.unlock();
    }

    Lock l = ...;
    l.lock();
    try {
        // do something
    } finally {
        l.unlock();
    }

  int foo;
  public void setFoo(int foo) {
    foo = foo;
  }

The assignment is useless. Did you mean to assign to the field instead?

  public void foo() {
    int x = 3;
    x = x;
  }

Such assignments are useless, and may indicate a logic error or typo.

  int x;
  public void foo() {
    x = x;
  }

Such assignments are useless, and may indicate a logic error or typo.

  int x,y;
  public void foo() {
    x = x = 17;
  }

Assigning to a field twice is useless, and may indicate a logic error or typo.

  public void foo() {
    int x,y;
    x = x = 17;
  }

Assigning the same value to a variable twice is useless, and may indicate a logic error or typo.

If it is important that the generated Random numbers not be guessable, you must not create a new Random for each random number; the values are too easily guessable. You should strongly consider using a java.security.SecureRandom instead (and avoid allocating a new SecureRandom for each random number needed).

One out of 2^32 strings have a hashCode of Integer.MIN_VALUE, including "polygenelubricants" "GydZG_" and ""DESIGNING WORKHOUSES".

Assuming you want to ensure that the result of your computation is nonnegative, you may need to change your code. If you know the divisor is a power of 2, you can use a bitwise and operator instead (i.e., instead of using x.hashCode()%n, use x.hashCode()&(n-1)). This is probably faster than computing the remainder as well. If you don't know that the divisor is a power of 2, take the absolute value of the result of the remainder operation (i.e., use Math.abs(x.hashCode()%n)).

In particular, the following code for packing a byte array into an int is badly wrong:

int result = 0;
for(int i = 0; i < 4; i++)
  result = ((result << 8) | b[i]);

The following idiom will work instead:

int result = 0;
for(int i = 0; i < 4; i++)
  result = ((result << 8) | (b[i] & 0xff));

In particular, the following code for packing a byte array into an int is badly wrong:

int result = 0;
for(int i = 0; i < 4; i++)
  result = ((result << 8) + b[i]);

The following idiom will work instead:

int result = 0;
for(int i = 0; i < 4; i++)
  result = ((result << 8) + (b[i] & 0xff));

Typically, this bug occurs because the code wants to perform a membership test in a bit set, but uses the bitwise OR operator ("|") instead of bitwise AND ("&").

Also such bug may appear in expressions like (e & A | B) == C which is parsed like ((e & A) | B) == C while (e & (A | B)) == C was intended.

Even if you feel confident that the method is never called by multiple threads, it might be better to not set the static field until the value you are setting it to is fully populated/initialized. ]]>

Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.

Better performance can be obtained by using a StringBuffer (or StringBuilder in Java 1.5) explicitly.

For example:

  // This is bad
  String s = "";
  for (int i = 0; i < field.length; ++i) {
    s = s + field[i];
  }

  // This is better
  StringBuffer buf = new StringBuffer();
  for (int i = 0; i < field.length; ++i) {
    buf.append(field[i]);
  }
  String s = buf.toString();

public static junit.framework.Test suite()

public static junit.framework.TestSuite suite()

Note that Sun's javac compiler often generates dead stores for final local variables. Because FindBugs is a bytecode-based tool, there is no easy way to eliminate these false positives.

See Sun's article on Java SE compatibility for more details and examples, and suggestions on how to force class initialization in Java 5.

A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:

  try {
    ...
  } catch (RuntimeException e) {
    throw e;
  } catch (Exception e) {
    ... deal with all non-runtime exceptions ...
  }

    long convertDaysToMilliseconds(int days) { return 1000*3600*24*days; }

If the multiplication is done using long arithmetic, you can avoid the possibility that the result will overflow. For example, you could fix the above code to:

    long convertDaysToMilliseconds(int days) { return 1000L*3600*24*days; }

    static final long MILLISECONDS_PER_DAY = 24L*3600*1000;
    long convertDaysToMilliseconds(int days) { return days * MILLISECONDS_PER_DAY; }

Date getDate(int seconds) { return new Date(seconds * 1000); }

The multiplication is done using 32-bit arithmetic, and then converted to a 64-bit value. When a 32-bit value is converted to 64-bits and used to express an absolute time value, only dates in December 1969 and January 1970 can be represented.

Correct implementations for the above method are:

// Fails for dates after 2037
Date getDate(int seconds) { return new Date(seconds * 1000L); }

// better, works for all dates
Date getDate(long seconds) { return new Date(seconds * 1000); }

int x = 2;
int y = 5;
// Wrong: yields result 0.0
double value1 =  x / y;

// Right: yields result 0.4
double value2 =  x / (double) y;

formatter.format("%<s %s", "a", "b")

would throw a MissingFormatArgumentException when executed.

The %d placeholder requires a numeric argument, but a string value is passed instead. A runtime exception will occur when this statement is executed.

String[] getAsArray(Collection<String> c) {
  return (String[]) c.toArray();
  }

This will usually fail by throwing a ClassCastException. The toArray() of almost all collections return an Object[]. They can't really do anything else, since the Collection object has no reference to the declared generic type of the collection.

The correct way to do get an array of a specific type from a collection is to use c.toArray(new String[]); or c.toArray(new String[c.size()]); (the latter is slightly more efficient).

There is one common/known exception to this. The toArray() method of lists returned by Arrays.asList(...) will return a covariantly typed array. For example, Arrays.asArray(new String[] { "a" }).toArray() will return a String []. FindBugs attempts to detect and suppress such cases, but may miss some.

This bug exists in many earlier implementations of binary search and merge sort. Martin Buchholz found and fixed it in the JDK libraries, and Joshua Bloch widely publicized the bug pattern.

• javax.xml.parsers.DocumentBuilderFactory
• javax.xml.parsers.SAXParserFactory
• javax.xml.transform.TransformerFactory
• org.w3c.dom.Document.createXXXX

for details.

In general, instances of two unrelated classes are not equal. For example, if the Foo and Bar classes are not related by subtyping, then an instance of Foo should not be equal to an instance of Bar. Among other issues, doing so will likely result in an equals method that is not symmetrical. For example, if you define the Foo class so that a Foo can be equal to a String, your equals method isn't symmetrical since a String can only be equal to a String.

In rare cases, people do define nonsymmetrical equals methods and still manage to make their code work. Although none of the APIs document or guarantee it, it is typically the case that if you check if a Collection<String> contains a Foo, the equals method of argument (e.g., the equals method of the Foo class) used to perform the equality checks.

You may also experience serialization problems.

Using an instance field is recommended.

For more information on this see JDK Bug #6231579 and JDK Bug #6178997.

For more information on this see JDK Bug #6231579 and JDK Bug #6178997.

You may also experience serialization problems.

Using an instance field is recommended.

For more information on this see JDK Bug #6231579 and JDK Bug #6178997.

For more information on this see JDK Bug #6231579 and JDK Bug #6178997.

More precisely, a value annotated with a type qualifier specifying when=ALWAYS is compared with a value that where the same type qualifier specifies when=NEVER.

For example, say that @NonNegative is a nickname for the type qualifier annotation @Negative(when=When.NEVER). The following code will generate this warning because the return statement requires a @NonNegative value, but receives one that is marked as @Negative.

public boolean example(@Negative Integer value1, @NonNegative Integer value2) {
    return value1.equals(value2);
}

More precisely, a value annotated with a type qualifier specifying when=ALWAYS is guaranteed to reach a use or uses where the same type qualifier specifies when=NEVER.

For example, say that @NonNegative is a nickname for the type qualifier annotation @Negative(when=When.NEVER). The following code will generate this warning because the return statement requires a @NonNegative value, but receives one that is marked as @Negative.

public @NonNegative Integer example(@Negative Integer value) {
    return value;
}

To coerce a value to have a strict annotation, define an identity function where the return value is annotated with the strict annotation. This is the only way to turn a non-annotated value into a value with a strict type qualifier annotation.

More precisely, a value annotated with a type qualifier specifying when=NEVER is guaranteed to reach a use or uses where the same type qualifier specifies when=ALWAYS.

TODO: example

The only situation in which opening a file in append mode and the writing an object output stream could work is if on reading the file you plan to open it in random access mode and seek to the byte offset where the append started.

TODO: example.

     private static final String base = "label";
     private static int nameCounter = 0;
     String constructComponentName() {
        synchronized (getClass()) {
            return base + nameCounter++;
        }
     }
     

Subclasses of Label won't synchronize on the same subclass, giving rise to a datarace. Instead, this code should be synchronizing on Label.class

     private static final String base = "label";
     private static int nameCounter = 0;
     String constructComponentName() {
        synchronized (Label.class) {
            return base + nameCounter++;
        }
     }
     

Bug pattern contributed by Jason Mehrens

In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.

This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. To send feedback, either:

• send email to findbugs@cs.umd.edu
• file a bug report: http://findbugs.sourceforge.net/reportingBugs.html

In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.

See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes, for a description of the analysis technique.

In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.

This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. To send feedback, either:

• send email to findbugs@cs.umd.edu
• file a bug report: http://findbugs.sourceforge.net/reportingBugs.html

In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.

See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes, for a description of the analysis technique.

public static void initLogging() throws Exception {
 Logger logger = Logger.getLogger("edu.umd.cs");
 logger.addHandler(new FileHandler()); // call to change logger configuration
 logger.setUseParentHandlers(false); // another call to change logger configuration
}

The logger reference is lost at the end of the method (it doesn't escape the method), so if you have a garbage collection cycle just after the call to initLogging, the logger configuration is lost (because Logger only keeps weak references).

public static void main(String[] args) throws Exception {
 initLogging(); // adds a file handler to the logger
 System.gc(); // logger configuration lost
 Logger.getLogger("edu.umd.cs").info("Some message"); // this isn't logged to the file as expected
}

Ulf Ochsenfahrt and Eric Fellheimer