New bug type: AA_ASSERTION_OF_ARGUMENTS

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+    "java.compile.nullAnalysis.mode": "automatic"
+}

CHANGELOG.md

@@ -21,6 +21,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 
 ### Added
 * New simple name-based AnnotationMatcher for exclude files (now bug annotations store the class java annotations in an attribute called `classAnnotationNames`). For example, use like <Match><Annotation name="org.immutables.value.Generated" /></Match> in an excludeFilter.xml to ignore classes generated by the Immutable framework. This ignores all class, method or field bugs in classes with that annotation.
+* New detector `FindArgumentAssertions` detecting bug `ASSERTION_OF_ARGUMENTS` in case of validation of arguments of public functions using assertions (See [MET01-J. Never use assertions to validate method arguments](https://wiki.sei.cmu.edu/confluence/display/java/MET01-J.+Never+use+assertions+to+validate+method+arguments))
 
 ### Security
 - Disable access to external entities when processing XML ([#2217](https://github.com/spotbugs/spotbugs/pull/2217))

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindArgumentAssertionsTest.java

@@ -0,0 +1,62 @@
+package edu.umd.cs.findbugs.detect;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.annotations.Confidence;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+public class FindArgumentAssertionsTest extends AbstractIntegrationTest {
+    @Test
+    public void testArgumentAssertions() {
+        performAnalysis("ArgumentAssertions.class");
+
+        assertNumOfDABugs(22);
+
+        assertDABug("getAbsAdd", 4);
+        assertDABug("getAbsAdd", 5);
+        assertDABug("getAbsAdd2", 14);
+        assertDABug("getAbsAdd2", 15);
+        assertDABug("getAbsAdd3", 28);
+        assertDABug("getAbsAdd3", 29);
+        assertDABug("getAbsAdd3", 30);
+        assertDABug("getAbsAdd4", 39);
+        assertDABug("print", 47);
+        assertDABug("indirect", 59);
+        assertDABug("compBool1", 64);
+        assertDABug("compBool2", 70);
+        assertDABug("getAbs", 76);
+        assertDABug("getAbs", 83);
+        assertDABug("compShor", 90);
+        assertDABug("getAbs", 96);
+        assertDABug("compLong", 103);
+        assertDABug("getAbs", 109);
+        assertDABug("compFloat", 116);
+        assertDABug("getAbs", 122);
+        assertDABug("compDouble", 129);
+        assertDABug("indirect", 136);
+        // assertDABug("indirect2", 143); -- indirect assertions of arguments are not supported yet (except copies)
+    }
+
+    private void assertNumOfDABugs(int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("AA_ASSERTION_OF_ARGUMENTS").build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertDABug(String method, int line) {
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("AA_ASSERTION_OF_ARGUMENTS")
+                .inClass("ArgumentAssertions")
+                .inMethod(method)
+                .atLine(line)
+                .withConfidence(Confidence.LOW)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -669,6 +669,8 @@
                     reports="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindPotentialSecurityCheckBasedOnUntrustedSource" speed="fast"
                     reports="USC_POTENTIAL_SECURITY_CHECK_BASED_ON_UNTRUSTED_SOURCE"/>
+          <Detector class="edu.umd.cs.findbugs.detect.FindArgumentAssertions" speed="fast"
+                    reports="AA_ASSERTION_OF_ARGUMENTS"/>
             <!-- Bug Categories -->
            <BugCategory category="NOISE" hidden="true"/>
 
@@ -695,10 +697,12 @@
           <BugCode abbrev="STCAL" cweid="366"/>
           <BugCode abbrev="ICAST" cweid="192"/>
           <BugCode abbrev="FB"/>
+          <BugCode abbrev="DA"/>
           <BugCode abbrev="AT"/>
           <BugCode abbrev="JUA"/>
           <!-- Bug patterns -->
           <BugPattern abbrev="JUA" type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS" category="BAD_PRACTICE" experimental="true" />
+          <BugPattern abbrev="AA" type="AA_ASSERTION_OF_ARGUMENTS" category="BAD_PRACTICE" />
           <BugPattern abbrev="CN" type="OVERRIDING_METHODS_MUST_INVOKE_SUPER" category="CORRECTNESS" />
           <BugPattern abbrev="CNT" type="CNT_ROUGH_CONSTANT_VALUE" category="BAD_PRACTICE"/>
 

spotbugs/etc/messages.xml

@@ -1772,12 +1772,18 @@ factory pattern to create these objects.</p>
       ]]>
     </Details>
   </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.FindArgumentAssertions">
+    <Details>
+      <![CDATA[
+            <p>Finds assertions that validate public method arguments.</p>
+      ]]>
+    </Details>
+  </Detector>
   <!--
   **********************************************************************
   BugPatterns
   **********************************************************************
    -->
-
   <BugPattern type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS">
     <ShortDescription> Asserting value of instanceof in tests is not recommended. </ShortDescription>
     <LongDescription> Assertion of type {0} in {2} at {3} may hide useful information about why a cast may have failed.</LongDescription>
@@ -8785,6 +8791,22 @@ Using floating-point variables should not be used as loop counters, as they are
       </p>]]>
     </Details>
   </BugPattern>
+
+  <BugPattern type="AA_ASSERTION_OF_ARGUMENTS">
+    <ShortDescription>Assertion is used to validate an argument of a public method</ShortDescription>
+    <LongDescription>Assertion validates method argument at {1}. If assertions are disabled, there won't be any argument validation.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>Asssertions must not be used to validate arguments of public methods because the validations are
+        not performed if assertions are disabled.</p>
+
+        <p>
+        See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/MET01-J.+Never+use+assertions+to+validate+method+arguments">MET01-J. Never use assertions to validate method arguments</a>
+        for more information.
+        </p>
+        ]]>
+    </Details>
+  </BugPattern>
   <!--
   **********************************************************************
    BugCodes
@@ -8938,4 +8960,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="THROWS">Exception throwing related problems</BugCode>
   <BugCode abbrev="PERM">Custom class loader does not call its superclass's getPermissions()</BugCode>
   <BugCode abbrev="USC">Potential security check based on untrusted source</BugCode>
+  <BugCode abbrev="AA">Misuse of assertions for checking arguments of public methods</BugCode>
 </MessageCollection>

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/AbstractAssertDetector.java

@@ -0,0 +1,51 @@
+package edu.umd.cs.findbugs.detect;
+
+import org.apache.bcel.Const;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
+
+/**
+ * Abstract base class for finding assertions
+ */
+public abstract class AbstractAssertDetector extends OpcodeStackDetector {
+
+    private final BugReporter bugReporter;
+
+    protected boolean inAssert = false;
+
+    public AbstractAssertDetector(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+    }
+
+    /**
+     * Implement this method in a concrete detector
+     */
+    abstract protected void detect(int seen);
+
+    /**
+     * Searches for assertion opening, and closing points.
+     * When in assert, will call the detect method.
+     */
+    @Override
+    public void sawOpcode(int seen) {
+        if (inAssert) {
+            detect(seen);
+        }
+        if (seen == Const.GETSTATIC && "$assertionsDisabled".equals(getNameConstantOperand())) {
+            inAssert = true;
+        }
+        if (seen == Const.NEW && getClassConstantOperand().equals("java/lang/AssertionError")) {
+            resetState();
+        }
+    }
+
+    protected void resetState() {
+        inAssert = false;
+    }
+
+    protected void reportBug(BugInstance bug) {
+        bugReporter.reportBug(bug);
+    }
+}

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindArgumentAssertions.java

@@ -0,0 +1,143 @@
+package edu.umd.cs.findbugs.detect;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.OpcodeStack;
+import edu.umd.cs.findbugs.OpcodeStack.Item;
+import edu.umd.cs.findbugs.ba.ClassContext;
+import edu.umd.cs.findbugs.ba.XMethod;
+
+/**
+ * This detector can find Assertions that try to validate method
+ * arguments.
+ */
+public class FindArgumentAssertions extends AbstractAssertDetector {
+
+    public FindArgumentAssertions(BugReporter bugReporter) {
+        super(bugReporter);
+    }
+
+    /**
+     * Only interested in public methods
+     */
+    @Override
+    public void visit(Method obj) {
+        if (!obj.isPublic())
+            return;
+    }
+
+    /**
+     * Only interested in public classes
+     */
+    @Override
+    public void visitClassContext(ClassContext classContext) {
+        JavaClass ctx = classContext.getJavaClass();
+        // Break out of analyzing this class if not public
+        if (!ctx.isPublic())
+            return;
+        super.visitClassContext(classContext);
+    }
+
+    /**
+     * Checks if the methods parameter is an initial arg.
+     */
+    private boolean isInitialArg() {
+        XMethod m = getXMethodOperand();
+        int numPar = m.getNumParams();
+        // Get values from the stack
+        for (int i = 0; i < numPar; i++) {
+            Item item = stack.getStackItem(i);
+            if (item.isInitialParameter())
+                return true;
+        }
+        return false;
+    }
+
+    /**
+     * Returns true if the opcode is a method invocation false otherwise
+     */
+    private boolean isMethodCall(int seen) {
+        return seen == Const.INVOKESTATIC ||
+                seen == Const.INVOKEVIRTUAL ||
+                seen == Const.INVOKEINTERFACE ||
+                seen == Const.INVOKESPECIAL;
+    }
+
+    /**
+     * Returns the number of arguments that is popped from the stack for the given opcode
+     */
+    private int checkSeen(int seen) {
+        int stackSize;
+        switch (seen) {
+        // Handle nullchecks
+        case Const.IFNONNULL:
+        case Const.IFNULL:
+        case Const.IFEQ:
+        case Const.IFNE:
+        case Const.IFLT:
+        case Const.IFLE:
+        case Const.IFGT:
+        case Const.IFGE:
+            stackSize = 1;
+            break;
+        // Handle integer comparison
+        case Const.IF_ICMPEQ:
+        case Const.IF_ICMPNE:
+        case Const.IF_ICMPLT:
+        case Const.IF_ICMPLE:
+        case Const.IF_ICMPGT:
+        case Const.IF_ICMPGE:
+            // Handle long
+        case Const.LCMP:
+            // Handle float comparison
+        case Const.FCMPG:
+        case Const.FCMPL:
+            // Handle double comparison
+        case Const.DCMPG:
+        case Const.DCMPL:
+            // Reference equality
+        case Const.IF_ACMPEQ:
+        case Const.IF_ACMPNE:
+            stackSize = 2;
+            break;
+        default:
+            stackSize = 0;
+            break;
+        }
+        return stackSize;
+    }
+
+    /**
+     * Finds MET01 rule violating assertions. 
+     */
+    @Override
+    protected void detect(int seen) {
+        boolean wasArg = false;
+        if (isMethodCall(seen)) {
+            // Handle method call
+            wasArg = isInitialArg();
+        } else {
+            // Handle comparison
+            int stackSize = checkSeen(seen);
+            if (stackSize > 0) {
+                for (int i = 0; i < stackSize; i++) {
+                    OpcodeStack.Item item = stack.getStackItem(i);
+                    wasArg = item.isInitialParameter();
+                    if (wasArg) {
+                        break;
+                    }
+                }
+            }
+        }
+        if (wasArg) {
+            BugInstance bug = new BugInstance(this, "AA_ASSERTION_OF_ARGUMENTS", LOW_PRIORITY)
+                    .addClassAndMethod(this)
+                    .addSourceLine(this, getPC());
+            reportBug(bug);
+        }
+    }
+}