mebigfatguy / fb-contrib

a FindBugs/SpotBugs plugin for doing static code analysis for java code bases

fb-contrib

a FindBugs and SpotBugs plugin for doing static code analysis on java byte code. For information see http://fb-contrib.sf.net

Available on maven.org

for FindBugs:

   GroupId: com.mebigfatguy.fb-contrib
ArtifactId: fb-contrib
   Version: 7.6.4

For SpotBugs

   GroupId: com.mebigfatguy.sb-contrib
ArtifactId: sb-contrib
   Version: 7.6.4

Developer

• Dave Brosius

Contributors

• Bhaskar Maddala
• Chris Peterson
• Grzegorz Slowikowski
• Trevor Pounds
• Ronald Blaschke
• Zenichi Amano
• Philipp Wiesemann
• Kevin Lubick
• Philippe Arteau
• Thrawn
• Juan Martin Sotuyo Dodero
• Richard Fearn
• Mikkel Kjeldsen
• Jeremy Landis
• Peter Hermsdorf
• David Burström
• Venkata Gajavalli
• Rubén López
• Pavel Roskin
• Kevin Seymour
• Piotrek Żygieło

fb-contrib has two main branches, 'findbugs' and 'spotbugs'. Code is committed to findbugs, and then merged to spotbugs. It is preferable therefore to create merge requests against the findbugs branch. Thanks!

Setting up for Development - Ant

1. Download/install Eclipse, ideally 4.3 (Kepler) or newer. The standard release (for Java) will work fine.
2. Ant Dependencies Download yank, the dependency manager and bug-rank-check-style. Both jars (v1.2.0+ and v1.0.0+) should go in your ~/.ant/lib folder, which you will have to make if it doesn't exist. Windows people, this goes under [Username]/.ant/lib. Don't have more than one version of either jar in this folder, as it's not clear which one Ant will load, leading to annoying compatibility issues. This can be done using the ant target ant infra_jars
3. Fork this git repo and clone it. GitHub for Windows or GitHub for Mac are good clients if you don't already have one.
4. Open Eclipse. File>Import and then choose "Existing projects into workspace", and find the fb-contrib folder you created in step 3. Ignore any compile errors (for now).
5. Using git, clone the FindBugs repository using git clone https://code.google.com/p/findbugs/ You will only need the findbugs subfolder (the one that has README.txt in it). You can delete the rest, if you wish.
6. Import this project into Eclipse as well. You may wish to mark these files as read-only, so you modify the "correct" files.
7. In the fb-contrib project, find the user.properties.example file. Make a copy of it named user.properties (this will not be tracked by version control). Modify the findbugs.dir property to where ever you have the FindBugs distribution installed. This is the executable FindBugs folder, not the source folder. The jar will be "installed" to (findbugs.dir)\plugin. For example, If you are using FindBugs with Eclipse (and you extracted Eclipse to C:\), you'll set this to something like findbugs.dir=/eclipse/plugins/edu.umd.cs.findbugs.plugin.eclipse_3.0.0.20140706-2cfb468
8. Finally, build fb-contrib by finding the build.xml file in Eclipse, right-click it, and select Run As > Ant Build. The dependencies needed should be downloaded to fb-contrib/lib and the fb-contrib-VERSION.jar should be built.

Setting up for Development - Maven

1. Download/install Maven, version 2.2.1 or newer.
2. Clone the Git repository, as per step 3 above.
3. Run mvn clean install in the fb-contrib directory.

Usage - Maven

To include the fb-contrib detectors when checking your project with FindBugs, you can use the FindBugs Maven plugin. The group ID for fb-contrib is com.mebigfatguy.fb-contrib, and the artifact ID is fb-contrib. Eg:

<plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>findbugs-maven-plugin</artifactId>
    <version>3.0.4</version>
    <configuration>
        <plugins>
            <plugin>
                <groupId>com.mebigfatguy.fb-contrib</groupId>
                <artifactId>fb-contrib</artifactId>
                <version>7.6.4</version>
            </plugin>
        </plugins>
    </configuration>
    <executions>
        <execution>
            <goals>
                <goal>check</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Or to include the fb-contrib detectors when checking your project with Spotbugs, you can use the SpotBugs Maven plugin which is a fork of findbugs maven plugin to provide spotbugs integration. The group ID for sb-contrib is com.mebigfatguy.sb-contrib, and the artifact ID is sb-contrib. Eg:

<plugin>
    <groupId>com.github.spotbugs</groupId>
    <artifactId>spotbugs-maven-plugin</artifactId>
    <version>3.1.12</version>
    <configuration>
        <plugins>
            <plugin>
                <groupId>com.mebigfatguy.sb-contrib</groupId>
                <artifactId>sb-contrib</artifactId>
                <version>7.6.4</version>
            </plugin>
        </plugins>
    </configuration>
    <executions>
        <execution>
            <goals>
                <goal>check</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Usage - Gradle

apply plugin: 'findbugs'

dependencies {
    // We need to manually set this first, or the plugin is not loaded
    findbugs 'com.google.code.findbugs:findbugs:3.0.0'
    findbugs configurations.findbugsPlugins.dependencies

    // To keep everything tidy, we set these apart
    findbugsPlugins 'com.mebigfatguy.fb-contrib:fb-contrib:7.6.4'
}

task findbugs(type: FindBugs) {
   // Add all your config here ...

   pluginClasspath = project.configurations.findbugsPlugins
}

Contributing

Once you have the dev environment set up, feel free to make changes and pull requests. Any edits are much appreciated, from finding typos, to adding examples in the messages, to creating new detectors, all help is welcome.

External guides for making detectors:

• https://www.ibm.com/developerworks/library/j-findbug2/
• http://kjlubick.github.io/blog/post/1?building-my-first-findbugs-detector

Misc references about bytecode:

• http://www.jroller.com/dbrosius/entry/the_jvm_class_file_format

For making detectors, it best to make several test cases, like those in the sample directory. Even better is if you can comment where you expect bug markers to appear and why, like this.

In your pull request, give an overview of your changes along with the related commits.

If you are not up for contributing code but notice a common problem with some third party library, or general purpose pattern, please add an issue too. We always like new ideas.

Often available on #fb-contrib on freenode.net for conversation.

fb-contrib's Issues

SecureRandom and Random (MDM_SECURERANDOM & MDM_SECURERANDOM)

I think two detectors related to Randomness are incorrect.

MDM_SECURERANDOM

The SecureRandom() constructors and SecureRandom.getSeed() method are deprecated. Call SecureRandom.getInstance() and SecureRandom.getInstance().generateSeed() instead.

The constructor of SecureRandom doesn't appears to be deprecated according to the javadoc :
http://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html

MDM_RANDOM_SEED

Random() constructor without a seed is insecure because it defaults to easily guessable seed: System.currentTimeMillis(). Initialize seed with Random(SecureRandom.getInstance().generateSeed()) or use SecureRandom instead.

The first recommendation to give a good seed is a bad idea. The initial state will be unpredictable but given a single long value it is possible to recover all the following values. This is not an obvious behavior. It can introduce vulnerability like CVE-2014-7809 or CVE-2015-0201.

What is the point of STT_STRING_PARSING_A_FIELD?

There are a few instances of STT_STRING_PARSING_A_FIELD in the fb-contrib codebase, which are proving difficult to eliminate to the detector's satisfaction (eg PoorlyDefinedParameter line 120), but I can't see the actual problem with this pattern. Why is there a detector for it?

Release schedule?

Is there any release schedule ahead? Over the last couple months there have been several fp / fn fixes to existing detectors that would be quiet interesting to have available on Maven. Are there any plans for a 6.2.2 release?

Or are we looking straight ahead to 6.3.0 with the changes to JAO? (and maybe MRC, not sure if it's ready for prime time yet)

UTAO_TESTNG_ASSERTION_ODDITIES_USE_ASSERT_NOT_NULL can't tell when annotated fields are externally populated

We use Mockito to automagically populate annotated fields with mock objects for testing, but fb-contrib can't tell that these fields are ever assigned, and if we use them in equality assertions, it fires UTAO_TESTNG_ASSERTION_ODDITIES_USE_ASSERT_NOT_NULL

import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import static org.mockito.Mockito.*;
import static org.testng.Assert.*;

public class EqualMockTest {

    private Object realObject;

    @Mock private Object mockObject;

    @BeforeMethod
    public void setUp() throws Exception {
        MockitoAnnotations.initMocks(this);

        realObject = new Object();
    }

    @Test
    public void shouldNotEqualMockObject() {
        assertNotEquals(realObject, mockObject);
    }

}

Alternative to temporary trim (SPP_TEMPORARY_TRIM) for finding Selenium WebElement?

Our code searches through Selenium web elements to find and select a drop-down option matching a given text, like so:

public static void select(WebElement element, String text) {
    for (WebElement subElement : element.findElements(By.xpath("option"))) {
        if (subElement.getText().trim().equals(text)) {
            subElement.click();
            return;
        }
    }

    throw new NoSuchElementException("No option with text: " + text + " under " + element);
}

This triggers SPP_TEMPORARY_TRIM, because it is trimming the element text for the comparison. However, contrary to what the detector suggests, we have no interest in storing the trimmed value for later; we only want to identify the element so we can click on it. Is there a better way to perform this search?

Add TestNGAssertionOddities?

TestNG tests work similarly in most respects to JUnit tests, and have most of the same considerations, they just have slightly different annotations and assertion styles.

• No base class or naming convention for tests
• Test annotation is org.testng.annotation.Test
• Assertions come from org.testng.Assert
• Assertions have different parameter order: "actual, expected, failure message" instead of "failure message, expected, actual"

Is it worth copying (or expanding) JUnitAssertionOddities to include it?

IICU_INCORRECT_INTERNAL_CLASS_USE false positive with JavaMail

The current detector reports on com.sun.xxx, however, there are libraries fully documented and intended for developer's use under such package.

For instance JavaMail offers a number of APIs under com.sun.mail.

I believe these packages should be whitelisted.

PMB_POSSIBLE_MEMORY_BLOAT shouldn't be reported for instances of WeakHashMap

Even if the collection is never explicitly being pruned of elements, they are getting removed when they can; and therefore won't cause a memory bloat.

It would be nice to filter this false positive.

Dogfooding - pass core FindBugs checks

fb-contrib has a pretty good FindBugs result, but still a handful of issues, and some false positives that should be specifically excluded.

I'll send through a pull request for these.

LO_APPENDED_STRING_IN_FORMAT_STRING does not check whether message is reused

The LO_APPENDED_STRING_IN_FORMAT_STRING detector is flagging a situation where a message is generated through string appending, used as an email body, and logged via SLF4J.

Since the concatenated string is in fact always needed, the detector should ignore this case.

Code looks like:

public void sendRecipientNotification(Recipient recipient, int changeCount) {
    SimpleMailMessage msg = new SimpleMailMessage();
    msg.setTo(recipient.getEmail());
    String subject;
    if (changeCount == 0) {
        subject = "There are no requests for " + recipient;
    } else {
        subject = recipient + " has new requests";
    }
    msg.setSubject(subject);

    try {
        LOG.trace(subject);
        sender.send(msg);
    } catch (MailException e) {
        LOG.error("Could not send notification email to recipient", e);
    }
}

FalsePositive on AssertionError

There seems to be a false positive when creating an AssertionError using fb-contrib (4.6.1?):

try {
 //do someting which might throw an exception
} 
catch (SomeException se) {
 throw new AssertionError(se);
}

Findbugs throws: Correctness - Method throws alternative exception from catch block without history [fb-contrib] which is not true since the AssertionError sets the cause:

public AssertionError(Object detailMessage) {
    this("" +  detailMessage);
    if (detailMessage instanceof Throwable)
        initCause((Throwable) detailMessage);
}

UPDATE: Fixed in a newer version...

False positive (OCP_OVERLY_CONCRETE_PARAMETER)

Hi, I've come around a false positive with this check. It reports a parameter as overly concrete, yet it is clearly not the case. Possibly referred to issue 38, but not sure.

public class App 
{
    @Nonnull
    public Factory bazFactory(@Nonnull final Foo foo) {
        return (bar) -> baz(foo, bar);
    }

    @Nonnull
    public Baz baz(@Nonnull final Foo foo, @Nonnull final Bar bar) {
        return new Baz(foo, bar);
    }
}

public class Baz {

    private final Foo foo;
    private final Bar bar;

    public Baz(@Nonnull final Foo foo, @Nonnull final Bar bar) {
        this.foo = foo;
        this.bar = bar;
    }

    public void method() {
        foo.methodB(bar);
    }
}

public interface Foonterface<T> {
    void methodA(@Nonnull final T elem);
}

public class Foo implements Foonterface<Bar> {
    public void methodA(final Biar bar) {
    }

    public void methodB(@Nonnull final Bar bar) {
    }   
}

public interface Factory {
    @Nonnull Baz create(@Nonnull final Bar bar);
}

It reports:
"1st parameter 'foo' could be declared as Foonterface instead" in line: return (bar) -> baz(foo, bar);

Yet it clearly should be class Foo, as it is needed to create a Baz, and Baz can't use a Foonterface since it has to call methodB, that is not part of the interface

FCBL false positives on field being modified from multiple methods

FCBL_FIELD_COULD_BE_LOCAL is firing on several fb-contrib detectors that maintain state in instance fields. I'm not entirely clear on why; perhaps it's unable to trace the method calls and recognise that calling super.visitCode will result in a call to seenOpcode?

Sample instances:
SuspiciousComparatorReturnValues lines 109-112 (seenNegative, seenPositive, seenZero, seenUnconditionalNonZero)
MethodReturnsConstant line 92 (returnPC)
UnitTestAssertionOddities line 159 (sawAssert)

Actually, the MethodReturnsConstant instance does look like a genuine hit, because there doesn't seem to be any other method that alters returnPC.

Circular Depenency detector Falsely reporting builder pattern

Findbugs is falsely reporting a CD_CIRCULAR_DEPENDENCY when a class implements the Builder pattern.

Here is a sample class that gets flagged as having a circular dependency between the User class and the Builder class.

import java.io.Serializable;
import java.util.UUID;
import jdk.nashorn.internal.ir.annotations.Immutable;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;

/*
* A user represents any user of the system. This object is for tracking which user is tied to a specific object and is not
* intended to provide login validation.
*
* @author Chris Picard, [email protected]
* @version 1.0.0
/
@Immutable
public final class User implements Serializable {
private static final long serialVersionUID = 1L;
private final UUID userId;
private final String firstName;
private final String lastName;

@SuppressWarnings("synthetic-access")
private User(final Builder builder) {
this.userId = builder.userId;
this.firstName = builder.firstName;
this.lastName = builder.lastName;
}

public UUID getUserId() {
return userId;
}

public String getFirstName() {
return firstName;
}

public String getLastName() {
return lastName;
}

@Override
public int hashCode() {
final HashCodeBuilder builder = new HashCodeBuilder();
builder.append(userId);
builder.append(firstName);
builder.append(lastName);
return builder.toHashCode();
}

@Override
public boolean equals(final Object obj) {
if (this == obj) {
return true;
}
if (obj == null) {
return false;
}
if (getClass() != obj.getClass()) {
return false;
}
final User other = (User) obj;
final EqualsBuilder builder = new EqualsBuilder();
builder.append(this.userId, other.userId);
builder.append(this.firstName, other.firstName);
builder.append(this.lastName, other.lastName);
return builder.isEquals();
}

@Override
public String toString() {
return ToStringBuilder.reflectionToString(this, ToStringStyle.MULTI_LINE_STYLE);
}

/*
* Implementation of the builder pattern.
*
* @author Chris Picard, [email protected]
* @version 1.0.0

/
public static final class Builder {
private UUID userId;
private String firstName;
private String lastName;

  public Builder setValues(final User user) {
     this.userId = user.getUserId();
     this.firstName = user.getFirstName();
     this.lastName = user.getLastName();
     return this;
  }

  public Builder setUserId(final UUID userId) {
     this.userId = userId;
     return this;
  }

  public Builder setFirstName(final String firstName) {
     this.firstName = firstName;
     return this;
  }

  public Builder setLastName(final String lastName) {
     this.lastName = lastName;
     return this;
  }

  @SuppressWarnings("synthetic-access")
  public User build() {
     if (userId == null) {
        userId = UUID.randomUUID();
     }
     if (StringUtils.isEmpty(firstName)) {
        throw new IllegalStateException("First name is not set");
     }
     return new User(this);
  }
}
}

False Positive on "Dls dead local store"

Hi Dave! It appears i have a false positive on this warning. In my Android project, I try to make a file where i'm going to store some profile pictures. I'm doing this on a singleton service, inside a static method:

private static File createCameraImageFile() throws IOException {
        final String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.getDefault()).format(new Date());
        final String cameraPhotoFileName = String.format("UMW_PROFILE_%1$s.jpeg", timeStamp);
        final File storageDir = Environment.getExternalStoragePublicDirectory(
                Environment.DIRECTORY_PICTURES);
        return new File(storageDir, cameraPhotoFileName);
    }

The warning says I have a dead local store on cameraPhotoName. I tried deleting the variable and just put it directly in the return, like this:

private static File createCameraImageFile() throws IOException {
        final String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.getDefault()).format(new Date());
        final File storageDir = Environment.getExternalStoragePublicDirectory(
                Environment.DIRECTORY_PICTURES);
        return new File(storageDir, String.format("UMW_PROFILE_%1$s.jpeg", timeStamp));

But it would point out that i have a dead local store in timeStamp. Its seems that it doesnt like local variables! Thank you very much! Cheers

SPP_TEMPORARY_TRIM does not like my pattern

So, Some Other Developer wrote some code that tripped SPP_TEMPORARY_TRIM, and rightfully so:

    if (searchCrit.getName() != null && !searchCrit.getName().trim().equals("")) {
        sqlQuery.setString("account_name", searchCrit.getName() + "%");

So I changed it to:

    String name = searchCrit.getName();
    if (name != null && !(name = name.trim()).equals("")) {
        sqlQuery.setString("account_name", name + "%");

(mostly because I don't like small changes like this ballooning the diff because I changed indentation on a huge block after the && condition was split in order to make room for a temporary variable.) And it worked fine for some time, until I upgraded fb-contrib to 6.4.0, after which this pattern trips SPP_TEMPORARY_TRIM again. BTW: it also trips if I introduce another variable instead of reassigning name.

Thanks :)

False negative on Circular Dependency

(FB.W.CORRECTNESS) Cd circular dependency
Class com.monits.agilefant.A has a circular dependency with other classes

We are not getting an error report when a class A uses B.class and at the same time said class B extends from the first one. For example when we create an intent using B.class as a parameter when said class B extends from A, we do have a circular dependency but we are not getting any report of it:

public class A extends Activity {
    @Override
    protected void onCreate(final Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        final Intent intent = new Intent(this, B.class);
        startActivity(intent);
    }
}

But, in the case where B uses A and A uses a method/instance of class B we do get the report. For example when we create an intent using a method from B called getIntent when that class B extends from A, we also have a circular dependency and we get the error report:

public class A extends Activity {
    @Override
    protected void onCreate(final Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        startActivity(B.getIntent(this));
    }
}
public class B extends A {
    /**
     * This factory method returns an intent of this class
     *
     * @param context A Context of class A
     * @return An intent
     */
    public static Intent getIntent(final Context context) {
        return new Intent(context, B.class);
    }
}

In resume, we are not getting an error over a circular dependency issue when we use .class and we should.

Sourceforge site is outdated

http://fb-contrib.sf.net/ doesn't mention release 6.0.1, nor that it uses findbugs 3.0.1.

Also, I'm not sure if all detectors are listed there.

LSYC_LOCAL_SYNCHRONIZED_COLLECTION not detected on StringBuffer

I have found an inconsistent behaviour on LSYC_LOCAL_SYNCHRONIZED_COLLECTION, which is not always reproting for StringBuffer usages.

In the following code, there are 2 equally wrong uses of StringBuffer, yet only one is ever reported.

public class StringBufferTest {

    private static String TAG = "TAG";

    public void test() {
        // This one is not reported as LSYC_LOCAL_SYNCHRONIZED_COLLECTION
        final StringBuffer stringBuffer = new StringBuffer().append("agrego ").append("un ");
        stringBuffer.append("string ");
        Log.i(TAG, stringBuffer.toString());

        // This one gets reported as LSYC_LOCAL_SYNCHRONIZED_COLLECTION
        final StringBuffer stringBuffer2 = new StringBuffer();
        stringBuffer2.append("agrego ");
        stringBuffer2.append("un ");
        stringBuffer2.append("string ");
        Log.i(TAG, stringBuffer2.toString());
    }
}

I checked the dissasembled code, thinking maybe the compiler was changing the other StringBuffer (after all, I'm using it with constants), yet it's not.

$ javap -c test/StringBufferTest.class
Compiled from "StringBufferTest.java"
public class test.StringBufferTest {
public test.StringBufferTest();
Code:
0: aload_0
1: invokespecial #1 // Method java/lang/Object."":()V
4: return

public void test();
Code:
0: new #2 // class java/lang/StringBuffer
3: dup
4: invokespecial #3 // Method java/lang/StringBuffer."":()V
7: ldc #4 // String agrego
9: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
12: ldc #6 // String un
14: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
17: astore_1
18: aload_1
19: ldc #7 // String string
21: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
24: pop
25: getstatic #8 // Field TAG:Ljava/lang/String;
28: aload_1
29: invokevirtual #9 // Method java/lang/StringBuffer.toString:()Ljava/lang/String;
32: invokestatic #10 // Method android/util/Log.i:(Ljava/lang/String;Ljava/lang/String;)I
35: pop
36: new #2 // class java/lang/StringBuffer
39: dup
40: invokespecial #3 // Method java/lang/StringBuffer."":()V
43: astore_2
44: aload_2
45: ldc #4 // String agrego
47: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
50: pop
51: aload_2
52: ldc #6 // String un
54: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
57: pop
58: aload_2
59: ldc #7 // String string
61: invokevirtual #5 // Method java/lang/StringBuffer.append:(Ljava/lang/String;)Ljava/lang/StringBuffer;
64: pop
65: getstatic #8 // Field TAG:Ljava/lang/String;
68: aload_2
69: invokevirtual #9 // Method java/lang/StringBuffer.toString:()Ljava/lang/String;
72: invokestatic #10 // Method android/util/Log.i:(Ljava/lang/String;Ljava/lang/String;)I
75: pop
76: return

static {};
Code:
0: ldc #11 // String TAG
2: putstatic #8 // Field TAG:Ljava/lang/String;
5: return
}

PRMC Detector fails with IllegalArgumentException

Hi,

I'm using fb-contrib 6.2.3 (which is bundled in SonarQube 5.2+Findbugs plugin), and launching the analysis on a big project (2,5M LoC), I get the following stacktrace very often:

  Can't get stack offset 0 from [] @ 528 in x.y.Z.method : (Ljava.lang.String;)Lx.y.Z;
    java.lang.IllegalArgumentException: 0 is not a value stack offset
      At edu.umd.cs.findbugs.OpcodeStack.getStackItem(OpcodeStack.java:3100)
      At com.mebigfatguy.fbcontrib.detect.PossiblyRedundantMethodCalls.sawOpcode(PossiblyRedundantMethodCalls.java:289)
      At edu.umd.cs.findbugs.visitclass.DismantleBytecode.visit(DismantleBytecode.java:883)
      At edu.umd.cs.findbugs.visitclass.BetterVisitor.visitCode(BetterVisitor.java:218)
      At edu.umd.cs.findbugs.visitclass.PreorderVisitor.visitCode(PreorderVisitor.java:235)
      At com.mebigfatguy.fbcontrib.detect.PossiblyRedundantMethodCalls.visitCode(PossiblyRedundantMethodCalls.java:202)
      At org.apache.bcel.classfile.Code.accept(Code.java:135)
      At edu.umd.cs.findbugs.visitclass.PreorderVisitor.doVisitMethod(PreorderVisitor.java:307)
      At edu.umd.cs.findbugs.visitclass.PreorderVisitor.visitJavaClass(PreorderVisitor.java:395)
      At org.apache.bcel.classfile.JavaClass.accept(JavaClass.java:215)
      At edu.umd.cs.findbugs.BytecodeScanningDetector.visitClassContext(BytecodeScanningDetector.java:38)
      At com.mebigfatguy.fbcontrib.detect.PossiblyRedundantMethodCalls.visitClassContext(PossiblyRedundantMethodCalls.java:174)
      At edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
      At edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1089)
      At edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:283)
      At org.sonar.plugins.findbugs.FindbugsExecutor$FindbugsTask.call(FindbugsExecutor.java:211)
      At java.util.concurrent.FutureTask.run(FutureTask.java:266)
      At java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
      At java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
      At java.lang.Thread.run(Thread.java:745)

... which break the analysis and is very annoying. Is it already known?

Thanks

OCP_OVERLY_CONCRETE_PARAMETER complains when implementing parameterised interface

I've written a JPA AttributeConverter for converting Joda DateTime instances to/from java.sql.Timestamp instances, but since I only need the millisecond value to convert them, the OCP_OVERLY_CONCRETE_PARAMETER detector claims that I could change a method parameter to type ReadableInstant instead of DateTime.

I don't want to do this, because I want the return type from converting a Timestamp to be of type DateTime, so to implement the interface properly, I need to use the concrete type.

Is there a better way to do this?

package findbugs.sample;

import java.sql.Timestamp;
import javax.persistence.AttributeConverter;
import javax.persistence.Converter;

import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;

@Converter(autoApply = true)
public class DateTimeConverter implements AttributeConverter<DateTime, Timestamp> {

    @Override
    public Timestamp convertToDatabaseColumn(DateTime attribute) {
        if (attribute == null) {
            return null;
        }
        return new Timestamp(attribute.getMillis());
    }

    @Override
    public DateTime convertToEntityAttribute(Timestamp dbData) {
        if (dbData == null) {
            return null;
        }
        return new DateTime(dbData.getTime(), DateTimeZone.UTC);
    }

}

False positive on IMC_IMMATURE_CLASS_NO_EQUALS

The following class incorrectly triggers IMC_IMMATURE_CLASS_NO_EQUALS with the latest trunk version of fb-contrib (dc8d086), and I can't work out why:

package findbugs.sample;

import org.apache.commons.lang3.builder.*;

public class DomainObject {

    private String field;

    public DomainObject(String field) {
        this.field = field;
    }

    public String getField() {
        return field;
    }

    public void setField(String field) {
        this.field = field;
    }

    @Override
    public boolean equals(Object o) {
        return EqualsBuilder.reflectionEquals(this, o);
    }

    @Override
    public int hashCode() {
        return HashCodeBuilder.reflectionHashCode(this);
    }

    @Override
    public String toString() {
        return String.format("DomainObject{ field=%s }", field);
    }
}

Compiling samples?

The Ant build compiles the sample code, while the Maven build does not. However, as far as I can tell, there are no automated tests that actually use the sample code.

Are the sample classes used just for manual testing?

I would like to enhance the POM to compile the sample classes, but first need to clarify their use case. Should the build compile them after creating the JAR file, as a sanity check? Compile them only on demand, in a separate build profile? Should there be unit tests for them?

Is ISB_EMPTY_STRING_APPENDING really slower?

This method concatenates an empty string with a literal value,
in order to convert the literal value into a string. It is more efficient
to use String.valueOf() to do the same thing as you do not incur
the cost of creating a StringBuffer/Builder and calling methods
on it to accomplish this.

I profiled the String.valueOf(x), new Integer(x).toString(), Integer.toString(x) and ""+x on Java 1.8.0.31. First three methods produce the same speed which was expected as Hotspot C2 JIT compiler is smart enough to get rid of extra Integer object and inline these calls. However it was complete surprise for me to discover that (""+x) is about 1.5x faster! You may probably want to test by yourself.

Add bug detector for non-static constants?

It might be valuable to add a performance-related bug detector for known-immutable classes being initialized from literal/constant parameters in a non-static context. A classic example is compiling a Pattern from a literal string; it is costly and will always produce the same output, so it should be done statically and saved in a constant.

Numbers probably should not be included, since they already have an efficient valueOf mechanism.

Guava's immutable collection/map builders might also be a candidate, when their parameters are all literals, along with most of the Joda Time classes (except the constructors that use the current time).

This might, of course, have false positives on unit tests, which could initialize an object that is only relevant to one test and therefore needed only once. (On the other hand, I tend to disable all performance-related bug detectors on tests anyway; there's not much point in micro-optimizing them.)

Make it easier for someone to recognise which findbugs rule they must suppress

Hello,

It seems that the comments in the error itself don't match (down to the word) the descriptions in your web page, making it difficult to track down the error when one needs to suppress it. While you are at it, can't you just add the specific BugType enum (https://github.com/mebigfatguy/fb-contrib/blob/master/src/com/mebigfatguy/fbcontrib/utils/BugType.java) in the error so that one does not have to spend time to find out which one it is in order to suppress it ?
PMD for example already does this.

Cheers,
Sakis

PRMC false positive on Arrays.asList

The Possibly-Redundant Method Calls detector is giving warnings when Arrays.asList is called multiple times with different parameters. Perhaps it is too aggressive about matching array contents?

The following class trips the detector:

package example;

import java.util.Arrays;

public class FindBugsSample {
    public void run() {
        System.out.println(Arrays.asList("foo"));
        System.out.println(Arrays.asList("bar"));
    }
}

Working right up to Christmas

You should take a break!

Thanks for your work on fb-contrib :).

Proposal: Unnecessary Collection Nesting Detector

I noticed the following anti-pattern in some service code the other day. Arrays.asList(...) already creates a copy of the original array so nesting it inside an additional ArrayList is unnecessary.

final List<...> valueList = new ArrayList<...>(Arrays.<...>asList(valueArray));

I'll take a crack at adding new detector for this bug but wanted to report it in case anyone wanted to get to it beforehand.

FP on FCBL field could be local

Given the following Android code:

public class MultiStateButton extends LinearLayout {
    private final int normalStateBackgroundResource;

    public MultiStateButton(final Context context, final AttributeSet attrs, final int defStyleAttr) {
        super(context, attrs, defStyleAttr);

        final TypedArray attr = context.obtainStyledAttributes(attrs, R.styleable.MultiStateButton, defStyleAttr, 0);
        normalStateBackgroundResource = attr.getResourceId(R.styleable.MultiStateButton_normalStateBackground, 0);
        attr.recycle();
    }

    public void showAsNormal() {
        setBackgroundResource(normalStateBackgroundResource);
    }
}

normalStateBackgroundResource gets flagged as could be local. But in truth it can't. It's a value obtained at one point in the lifecycle, and used at another, therefore needing to be stored as internal state (and since it won't change, properly flagged as final).

The only alternative would be to keep the TypedArray or AttributeSet instead of the single int. This is not viable. Those objects wrap around a STAX parser you don't want to keep around in memory at all, and even without the memory concerns, the performance of parsing the XML each time should be enough to prefer keeping the single cached int value.

I believe the detector currently just checks on how many different places the variable is read, but I believe it should also check if it's being written from somewhere else and the value being written depends somehow on that method's parameters (I understand his may be hard, since such dependency may be direct or indirect).

FCBL_FIELD_COULD_BE_LOCAL false positive

Given the following sample code, where a single field is clearly being used in two separate methods of the same class, I get a FCBL_FIELD_COULD_BE_LOCAL on method1.

public class LocalFPDemo {
    private boolean notLocal;

    public void method1() {
        notLocal = false; // FCBL_FIELD_COULD_BE_LOCAL reported here
        method2();
        if (notLocal) {
            System.out.println("Not local");
        }
    }

    private void method2() {
        if (Math.random() > 0.5) {
            notLocal = true;
        }
    }
}

Proposed new detector for vulnerable Jetty Servers

I recently ran into an exploit that depended on the following snippet:
https://gist.github.com/kjlubick/560dda55c47dd3a53643

The problem was that I meant to make a local server that was only accessible to the local machine, but instead, because I didn't specify otherwise, the port was open to everyone on the local network.

I'm proposing a relatively simple detector that looks for a Jetty server (and possibly other flavors) that is created by only specifying the port number, thereby making them vulnerable to snooping via network.

One topic for discussion is avoiding false positives when the intent is to make a local network server. This could possibly be done by having a whitelist of ports (e.g. port 80) that are not flagged as a bug.

Thoughts?

PRMC false positive on try-finally blocks

The PRMC detector fires when there are duplicate method calls inside a try block and its corresponding finally block.

Since the nature of try blocks is that we never know which statements will or won't be called, they probably should only be analysed in isolation (ie redundant method calls within the try block), not in conjunction with code outside the block.

This applies to many parts of the fb-contrib codebase itself! However, here is a minimalist sample:

package findbugs.sample;

class DuplicateMethodCallInsideFinally {

    public void run() {
        Object foo = new Object();
        try {
            willThrow();
            System.err.println(foo.toString());
        } finally {
            System.out.println(foo.toString());
        }
    }

    void willThrow() {
        throw new RuntimeException("kaboom!");
    }

}

OPM_OVERLY_PERMISSIVE_METHOD does not take account of libraries

The new OPM_OVERLY_PERMISSIVE_METHOD detector does not consider the possibility that a method may validly be used outside the visible codebase (eg if it is a library). This makes it so noisy that it usually needs to be turned off. Perhaps it should be lowered in priority to reflect this.

However, there may still be more reliable uses of this detector, eg there is no need for the constructor of an abstract class to be public.

Need to specify encoding when invoking javac tasks in Ant build file

In 6.4.0, one of the fb-contrib source files is now UTF-8, due to commit 302fb90.

The Fedora package for 6.4.0 failed to build due to this:

compile:
    [javac] Compiling 153 source files to /builddir/build/BUILD/findbugs-contrib-6.4.0/target/classes/main
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.7
    [javac] /builddir/build/BUILD/findbugs-contrib-6.4.0/src/com/mebigfatguy/fbcontrib/detect/UnitTestAssertionOddities.java:4: error: unmappable character for encoding ASCII
    [javac]  * Copyright (C) 2015 - Juan Mart??n Sotuyo Dodero
    [javac]                                  ^
    [javac] /builddir/build/BUILD/findbugs-contrib-6.4.0/src/com/mebigfatguy/fbcontrib/detect/UnitTestAssertionOddities.java:4: error: unmappable character for encoding ASCII
    [javac]  * Copyright (C) 2015 - Juan Mart??n Sotuyo Dodero
    [javac]                                   ^
    [javac] 2 errors
    [javac] 1 warning
BUILD FAILED

I added an ant -diagnostics call that shows:

file.encoding : ANSI_X3.4-1968

I don't know why the encoding is ANSI_X3.4-1968 (otherwise known as ASCII) on that host, but the fb-contrib build depends on the system encoding, and is therefore a bit fragile.

I recommend adding an explicit encoding to the javac and javadoc task invocations.

Dogfooding - pass fb-contrib checks

Not sure whether it's worth constantly aiming fb-contrib at itself (and which version would you use to check the checker?), but using 6.4.3 on the current codebase shows up some issues worth resolving.

Will submit pull request(s) soon.

false positive with Java 8 lambdas (OCP_OVERLY_CONCRETE_PARAMETER)

When running findbugs on code that uses lambdas, we are seeing several false positives reported as parameters overly concrete.

I've veen able to reproduce the problem with this simple code snippet:

import java.util.Arrays;
import java.util.Properties;


public class Test {
    public void test() {
        final Properties properties = new Properties();

        Arrays.stream(new String[] { "a", "b" })
            .forEach(prop -> properties.put(prop, "ok"));
    }
}

This is reporting:

 Method Test.lambda$test$3(Properties, String) needlessly defines parameter with concrete classes

Which is probably due to the way the compiler infers the types when creating the class for the lambda expression.

DRE_DECLARED_RUNTIME_EXCEPTION false positive on method generated in implementing class

When an interface from a third-party library declares a RuntimeException in the throws clause of a method that returns a type, and then one of our classes implements that interface with a method that returns a subtype and does not declare the RuntimeException, we get a DRE_DECLARED_RUNTIME_EXCEPTION false positive.

This appears to happen because when the implementing method returns a subtype, Java generates a second method with the original signature, delegating to the first method, and that generated method still declares the RuntimeException.

It would be correct for the rule to fire on the third-party interface, but we cannot fix that (in this case, it's from Spring).

Migration to FindBugs 3.0

Will this project be migrating for compatibility with 3.0 or staying with the 2.0 branch?

Best way to do concatenation of single character with integer?

The PoorlyDefinedParameter detector does String concatenation to surround an int with brackets. Since brackets are single characters, UCPM_USE_CHARACTER_PARAMETERIZED_METHOD doesn't like this, saying that it should use char instead of String; however, if we do that, then we will get addition instead of concatenation.

String.format would resolve this, but with slower performance. Manually creating the implicit StringBuilder would also work, but seems verbose for such a simple operation.

Is it feasible for UCPM_USE_CHARACTER_PARAMETERIZED_METHOD to recognise this situation (1-character String + numeric type) where we must use the String type in order to get String concatenation?

HCP_HTTP_REQUEST_RESOURCES_NOT_FREED_* does not apply to HttpClient 4.4

FindBugs noticed our usage of HttpClient as per http://fb-contrib.sourceforge.net/bugdescriptions.html#HCP_HTTP_REQUEST_RESOURCES_NOT_FREED_LOCAL . The fix is to call .reset:

public String requestInfo(URI u) {
    HttpGet httpGet = new HttpGet(u);
    try(CloseableHttpResponse response = client.execute(httpGet);) {
        return getResponseAsString(response);
    }
    catch (IOException e) {
        e.printStackTrace();
    }
    finally {
        httpGet.reset();
    }
    return null;
}

I was very surprised to see that this is an issue as it is not documented anywhere on Apache's site. I dug into code for HttpClient 4.4 to investigate. It appears that calling HttpGet.reset() (or any of the Http request objects that extend from AbstractExecutionAwareRequest) calls .cancel on an arbitrary Cancellable object held on the request. Throughout the operation of client.execute the cancellable is swapped depending on the stage the request might be in (e.g. requesting a connection from the connection pool, consuming the http entity).

Calling .reset() prior to .execute(...) does nothing because the cancellable would not be set. The call to .execute(...) is well encapsulated in a try-catch block to abort the connection if needed:

try {
...
} catch (final ConnectionShutdownException ex) {
            final InterruptedIOException ioex = new InterruptedIOException(
                    "Connection has been shut down");
            ioex.initCause(ex);
            throw ioex;
        } catch (final HttpException ex) {
            releaseTrigger.abortConnection();
            throw ex;
        } catch (final IOException ex) {
            releaseTrigger.abortConnection();
            throw ex;
        } catch (final RuntimeException ex) {
            releaseTrigger.abortConnection();
            throw ex;
        }

The only time calling .reset() would be handy is if the http request object is being reused or to cancel the request asynchronously.

Can the documentation for HCP_HTTP_REQUEST_RESOURCES_NOT_FREED_* be updated to clarify which version(s) of HttpClient this applies to?

I realize a more thorough investigation may be needed for older versions of HttpClient.

STT_TOSTRING_STORED_IN_FIELD trips on result of String concatenation

The STT_TOSTRING_STORED_IN_FIELD detector fires when a String is assembled via String concatenation (which creates StringBuilders behind the scenes).

Sample class:

public class StoreConcatenatedString {

    private String message;

    public StoreConcatenatedString(int value) {
        message = "Value was: " + value;
    }

    public String getMessage() {
        return message;
    }

}

False Positive on FCBL field could be local

Hi there! :)
Here is a simplified version of an adapter I'm implementing in my Android project:
MyAdapter.class

public class MyAdapter {

    private final CallbackManager callbackManager;
    private final Fragment fragment;

    public MyAdapter(final Fragment fragment, final CallbackManager callbackManager) {
        this.fragment = fragment;
        this.callbackManager = callbackManager;
    }

    public View getView(final ViewGroup parent) {
        final View view;
        final ViewHolder holder;
        final LayoutInflater inflater = LayoutInflater.from(fragment.getActivity());
        view = inflater.inflate(R.layout.campaign_list_element, parent, false);
        holder = new ViewHolder();
        holder.buttonShare = (ImageButton) view.findViewById(R.id.share_button);

        holder.buttonShare.setOnClickListener(new CampaignShareListener(fragment, null, callbackManager));

        return null;
    }

    private static class ViewHolder {
        public ImageButton buttonShare;
    }
}

As you can see, CallbackManager can not be a local variable, yet findbugs reports it as such. Help :(

Thanks!!

Redundant nullcheck in SillynessPotPourri.checkTrimDupStore

checkTrimDupStore performs a nullcheck on uv twice.

Is this just a mistaken copy-and-paste, or was the second check supposed to be doing something else (eg null-checking item)?

/**
 * determines whether this operation is storing the result of a trim() call, where the trimmed string was duplicated on the stack. If it was, it clears any
 * trim uservalue that was left behind in the dupped stack object
 */
private void checkTrimDupStore() {
    if ((stack.getStackDepth() >= 2) && (getPrevOpcode(1) == Constants.DUP)) {
        OpcodeStack.Item item = stack.getStackItem(0);
        SPPUserValue uv = (SPPUserValue) item.getUserValue();
        if ((uv == null) || (uv.getMethod() != SPPMethod.TRIM)) {
            return;
        }

        item = stack.getStackItem(1);
**** this is redundant **** if ((uv == null) || (uv.getMethod() != SPPMethod.TRIM)) {
            return;
        }

        item.setUserValue(null);
    }
}

Bogus Exception should handle all methods, not only private/static/etc

Current javadoc for Bogus Exception says:

looks for constructors, private methods or static methods

But it should handle all methods, especially public ones (those are the ones that cause the most trouble for the library users).

I'm trying this rule on following class:

package test;
class Main {
    public void foo() throws InterruptedException {
        System.out.println("test");
    }
}

For the above class (and even when I change method to private) this bug is not reported.

WOC_WRITE_ONLY_COLLECTION_LOCAL false positive if collection is read in lambda

If data is read from collections in lambda WOC_WRITE_ONLY_COLLECTION_LOCAL is generated.

UTAO_JUNIT_ASSERTION_ODDITIES_NO_ASSERT does not handle tests expecting exceptions

The documentation for UTAO_JUNIT_ASSERTION_ODDITIES_NO_ASSERT states:

"While a unit test could still be valid if it relies on whether or not an exception is thrown, it is usually a sign of a weak test if there are no assertions."

However, in the case where the unit test is indeed checking whether an exception is thrown, this makes the whole functionality of expecting an exception (which is built into JUnit and TestNG) nearly useless; the programmer must either use a try-catch block in every test, or add a redundant 'fail' instruction at the point where the exception should have been thrown.

Is it feasible for this detector to ignore methods that are specifically annotated to expect an exception?

@Test(expected = FooException.class)
public void shouldThrowFooException() {
    ...blah...

Build with Maven?

Is there a specific blocker preventing fb-contrib from having a Maven build? After all, it already exists in the Maven repository ecosystem...

This would eliminate the need to manually download JARs first; anyone with Maven could just clone the repository and run mvn clean install.

CSI_CHAR_SET_ISSUES_USE_STANDARD_CHARSET_NAME is case-sensitive (false negative)

The detector is case-sensitive, therefore, it reports usages of strings such as "UTF-8", but not usages of "utf-8".

This is wrong, since charsets in Java are case-insensitive, as seen in the documentation.

Charset names are not case-sensitive; that is, case is always ignored when comparing charset names.

Should WEM apply to static initializers?

Static initializers have no parameters, and typically no state to log, so should they be an exception to the WEM_WEAK_EXCEPTION_MESSAGING detector? Adding the root cause is about as much information as we can meaningfully give.

For example, we use a static initializer to configure features on a constant javax.xml.parsers.DocumentBuilderFactory. If it fails, all we can really report is a static message and the root cause.

Actually, instance initializers may be in the same position.