New bug type: AA_ASSERTION_OF_ARGUMENTS

CHANGELOG.md

@@ -30,6 +30,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - New Detector `FindVulnerableSecurityCheckMethods` for new bug type `VSC_VULNERABLE_SECURITY_CHECK_METHODS`. This bug is reported whenever a non-final and non-private method of a non-final class performs a security check using the `java.lang.SecurityManager`. (See [SEI CERT MET03-J] (https://wiki.sei.cmu.edu/confluence/display/java/MET03-J.+Methods+that+perform+a+security+check+must+be+declared+private+or+final))
 - New function added to detector `SynchronizationOnSharedBuiltinConstant`to detect `DL_SYNCHRONIZATION_ON_INTERNED_STRING` ([#2266](https://github.com/spotbugs/spotbugs/pull/2266))
 - Make TypeQualifierResolver recognize org.apache.avro.reflect.Nullable ([#2066](https://github.com/spotbugs/spotbugs/pull/2066))
+- 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,80 @@
+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(23);
+
+        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)
+        assertNoDABug("literal");
+        assertNoDABug("literalAndMessage");
+        assertNoDABug("literalAndMessageStr");
+        assertNoDABug("conditionallyInMessage");
+        assertNoDABug("privateMethod");
+        assertNoDABug("privateFinalMethod");
+        assertNoDABug("privateStaticMethod");
+        assertDABug("assertingArgInFor", 198);
+        // assertDABug("lambda$assertingArgInStream$0", 206); // assertations inside streams are not supported yet
+    }
+
+    private void assertNumOfDABugs(int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("AA_ASSERTION_OF_ARGUMENTS").build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertNoDABug(String method) {
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("AA_ASSERTION_OF_ARGUMENTS")
+                .inClass("ArgumentAssertions")
+                .inMethod(method)
+                .build();
+        assertThat(getBugCollection(), containsExactly(0, bugInstanceMatcher));
+    }
+
+    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

@@ -675,6 +675,8 @@
                     reports="PA_PUBLIC_PRIMITIVE_ATTRIBUTE,PA_PUBLIC_ARRAY_ATTRIBUTE,PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindVulnerableSecurityCheckMethods" speed="fast"
                     reports="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
+          <Detector class="edu.umd.cs.findbugs.detect.FindArgumentAssertions" speed="fast"
+                    reports="AA_ASSERTION_OF_ARGUMENTS"/>
     <!-- Bug Categories -->
            <BugCategory category="NOISE" hidden="true"/>
 
@@ -705,8 +707,10 @@
           <BugCode abbrev="JUA"/>
           <BugCode abbrev="ASE"/>
           <BugCode abbrev="VSC"/>
+          <BugCode abbrev="DA"/>
           <!-- 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

@@ -1796,6 +1796,13 @@ factory pattern to create these objects.</p>
         </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
@@ -8946,6 +8953,21 @@ 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
@@ -9102,4 +9124,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="ASE">Assertion with side effect</BugCode>
   <BugCode abbrev="PA">Public Attribute</BugCode>
   <BugCode abbrev="VSC">Vulnerable security check performing methods</BugCode>
+  <BugCode abbrev="AA">Misuse of assertions for checking arguments of public methods</BugCode>
 </MessageCollection>

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

@@ -0,0 +1,157 @@
+/*
+ * SpotBugs - Find bugs in Java programs
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+package edu.umd.cs.findbugs.detect;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.JavaClass;
+
+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 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;
+        XMethod method = getXMethod();
+        if (!method.isPublic()) {
+            return;
+        }
+
+        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);
+        }
+    }
+}