-
Notifications
You must be signed in to change notification settings - Fork 578
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
New bug type: AA_ASSERTION_OF_ARGUMENTS
Using assertions to validate arguments of public functions is a bad and dangerous practice because the validation does not happen if assertions are disabled, thus the behavior of the program depends on this compilation option. Therefore, we introduced a new detector `FindArgumentAssertions` to detect such cases and report `AA_ASSERTION_OFR_ARGUMENTS` for each of them. Co-authored-by: Gábor Kutas <@vodorok>
- Loading branch information
Ádám Balogh
committed
Jul 29, 2022
1 parent
b1c2a5e
commit 0494ea4
Showing
7 changed files
with
460 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
62 changes: 62 additions & 0 deletions
62
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindArgumentAssertionsTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
51 changes: 51 additions & 0 deletions
51
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/AbstractAssertDetector.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |
145 changes: 145 additions & 0 deletions
145
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindArgumentAssertions.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| 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 paramaeter 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) { | ||
| boolean methodCall = false; | ||
| if (seen == Const.INVOKESTATIC || | ||
| seen == Const.INVOKEVIRTUAL || | ||
| seen == Const.INVOKEINTERFACE || | ||
| seen == Const.INVOKESPECIAL) { | ||
| methodCall = true; | ||
| } | ||
| return methodCall; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the numbor of arguments that is popped from the stack for the given opcode | ||
| */ | ||
| private int checkSeen(int seen) { | ||
| int stackSize = 0; | ||
| 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: | ||
| break; | ||
| } | ||
| return stackSize; | ||
| } | ||
|
|
||
| /** | ||
| * Finds MET01 rule violating assertions. | ||
| */ | ||
| @Override | ||
| protected void detect(int seen) { | ||
| boolean wasArg = false; | ||
| // Handle method call | ||
| if (isMethodCall(seen)) { | ||
| // If wasArg have not been found - Nested methods | ||
| if (!wasArg) | ||
| wasArg = isInitialArg(); | ||
| // Handle comparison | ||
| } else { | ||
| 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) { | ||
| BugInstance bug = new BugInstance(this, "AA_ASSERTION_OF_ARGUMENTS", LOW_PRIORITY) | ||
| .addClassAndMethod(this) | ||
| .addSourceLine(this, getPC()); | ||
| reportBug(bug); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.