From b0d7d801741f18940e5ea630455e3d209c03296a Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Wed, 14 Mar 2018 17:09:39 +0100 Subject: [PATCH] add new SLF4J_GET_STACK_TRACE detector (fixes #70) incl. adjustments for code review feedback * delete useless inline comment * add @Item.SpecialKind annotation * rename getIsOfInterestKind to getKindOfInterest * rename isGetStackTrace to getStackTraceKind * rename AbstractDetectorForParameterArray2 to AbstractExtendedDetectorForParameterArray --- CHANGELOG.md | 2 + README.md | 20 +++++ bug-pattern/.gitignore | 1 + .../slf4j/ManualGetStackTraceDetector.java | 38 +++++++++ .../findbugs/slf4j/ManualMessageDetector.java | 78 +++---------------- .../AbstractDetectorForParameterArray2.java | 65 ---------------- ...ractExtendedDetectorForParameterArray.java | 65 ++++++++++++++++ bug-pattern/src/main/resources/bugrank.txt | 1 + bug-pattern/src/main/resources/findbugs.xml | 3 + bug-pattern/src/main/resources/messages.xml | 18 +++++ test-case/.gitignore | 1 + .../src/main/java/pkg/UsingGetStackTrace.java | 13 ++++ .../slf4j/UsingGetStackTraceTest.java | 25 ++++++ 13 files changed, 199 insertions(+), 131 deletions(-) create mode 100644 bug-pattern/.gitignore create mode 100644 bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualGetStackTraceDetector.java delete mode 100644 bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractDetectorForParameterArray2.java create mode 100644 bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractExtendedDetectorForParameterArray.java create mode 100644 test-case/.gitignore create mode 100644 test-case/src/main/java/pkg/UsingGetStackTrace.java create mode 100644 test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingGetStackTraceTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ca50d199..0fe7dd62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ This changelog follows [Keep a Changelog v1.0.0](https://keepachangelog.com/en/1 ## Unreleased +- SLF4J_GET_STACK_TRACE bug pattern + ## 1.4.1 - 2018-06-17 ### Changed diff --git a/README.md b/README.md index 76303ded..46fc0ce7 100644 --- a/README.md +++ b/README.md @@ -192,6 +192,26 @@ class Foo { } ``` +## SLF4J_GET_STACK_TRACE + +This pattern reports on the use of `Throwable#getStackTrace()`. This is most +typically wrong and a misunderstanding of the slf4j API, as normally you just +provide the Throwable instance as the last argument, instead of using getStackTrace(). + +```java +class Foo { + private final Logger logger = LoggerFactory.getLogger(getClass()); + void method() { + // invalid: needless 'e.getStackTrace()' + logger.info("Error occured. Stack is {}", e.getStackTrace()); + + // valid + logger.info("Error occured.", e); + } +} +``` + + # How to use with Maven To use this product, please configure your spotbugs-maven-plugin like below. diff --git a/bug-pattern/.gitignore b/bug-pattern/.gitignore new file mode 100644 index 00000000..da7560e0 --- /dev/null +++ b/bug-pattern/.gitignore @@ -0,0 +1 @@ +/.apt_generated_tests/ diff --git a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualGetStackTraceDetector.java b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualGetStackTraceDetector.java new file mode 100644 index 00000000..bb115af3 --- /dev/null +++ b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualGetStackTraceDetector.java @@ -0,0 +1,38 @@ +package jp.skypencil.findbugs.slf4j; + +import static org.apache.bcel.Const.INVOKEVIRTUAL; + +import com.google.common.base.Objects; +import edu.umd.cs.findbugs.BugReporter; +import edu.umd.cs.findbugs.OpcodeStack.Item; +import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray; + +/** + * FindBugs (now SpotBugs) Detector for (ab)use of {@link Throwable#getStackTrace()} in SFL4j logger. + * + * @author Michael Vorburger.ch + */ +public class ManualGetStackTraceDetector extends AbstractExtendedDetectorForParameterArray { + + @Item.SpecialKind + private final int getStackTraceKind = Item.defineNewSpecialKind("use of throwable getStackTrace"); + + public ManualGetStackTraceDetector(BugReporter bugReporter) { + super(bugReporter, "SLF4J_GET_STACK_TRACE"); + } + + @Override + @Item.SpecialKind + protected int getKindOfInterest() { + return getStackTraceKind; + } + + @Override + protected boolean isWhatWeWantToDetect(int seen) { + return seen == INVOKEVIRTUAL + && !stack.isTop() + && getThrowableHandler().checkThrowable(getStack().getStackItem(0)) + && Objects.equal("getStackTrace", getNameConstantOperand()); + } + +} diff --git a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualMessageDetector.java b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualMessageDetector.java index 4db7f526..23fe93c1 100644 --- a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualMessageDetector.java +++ b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualMessageDetector.java @@ -1,18 +1,16 @@ package jp.skypencil.findbugs.slf4j; +import static org.apache.bcel.Const.INVOKEVIRTUAL; + import com.google.common.base.Objects; -import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray2; -import jp.skypencil.findbugs.slf4j.parameter.ArrayData; -import edu.umd.cs.findbugs.BugInstance; import edu.umd.cs.findbugs.BugReporter; import edu.umd.cs.findbugs.OpcodeStack.CustomUserValue; import edu.umd.cs.findbugs.OpcodeStack.Item; -import javax.annotation.Nullable; -import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray; -import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy; +import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray; +import jp.skypencil.findbugs.slf4j.parameter.ArrayData; @CustomUserValue -public final class ManualMessageDetector extends AbstractDetectorForParameterArray { +public final class ManualMessageDetector extends AbstractExtendedDetectorForParameterArray { @Item.SpecialKind private final int isMessage = Item.defineSpecialKind("message generated by throwable object"); @@ -22,38 +20,8 @@ public ManualMessageDetector(BugReporter bugReporter) { } @Override - protected Strategy createArrayCheckStrategy() { - return (storedItem, arrayData, index) -> { - if (arrayData == null) { - return; - } - - if (storedItem.getSpecialKind() == isMessage) { - arrayData.mark(true); - } - - // Let developer logs exception message, only when argument does not have throwable instance - // https://github.com/KengoTODA/findbugs-slf4j/issues/31 - if (index == arrayData.getSize() - 1 && !getThrowableHandler().checkThrowable(storedItem)) { - arrayData.mark(false); - } - }; - } - - @Override - public void afterOpcode(int seen) { - boolean isInvokingGetMessage = isInvokingGetMessage(seen); - super.afterOpcode(seen); - - if (isInvokingGetMessage && !stack.isTop()) { - stack.getStackItem(0).setSpecialKind(isMessage); - } - } - - - @Override - protected int getIsOfInterestKind() { - return isMessage; + protected int getKindOfInterest() { + return isMessage; } @Override @@ -64,33 +32,11 @@ protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int i && !getThrowableHandler().checkThrowable(storedItem)); } - private boolean isInvokingGetMessage(int seen) { - return seen == INVOKEVIRTUAL - && !stack.isTop() - && getThrowableHandler().checkThrowable(getStack().getStackItem(0)) - && (Objects.equal("getMessage", getNameConstantOperand()) - || Objects.equal("getLocalizedMessage", getNameConstantOperand())); - } - @Override - protected void onLog(@Nullable String format, @Nullable ArrayData arrayData) { - if (arrayData == null || !arrayData.isMarked()) { - return; + protected boolean isWhatWeWantToDetect(int seen) { + return seen == INVOKEVIRTUAL && !stack.isTop() + && getThrowableHandler().checkThrowable(getStack().getStackItem(0)) + && (Objects.equal("getMessage", getNameConstantOperand()) + || Objects.equal("getLocalizedMessage", getNameConstantOperand())); } - BugInstance bugInstance = - new BugInstance(this, "SLF4J_MANUALLY_PROVIDED_MESSAGE", NORMAL_PRIORITY) - .addSourceLine(this) - .addClassAndMethod(this) - .addCalledMethod(this); - getBugReporter().reportBug(bugInstance); - } - - @Override - protected boolean isWhatWeWantToDetect(int seen) { - return seen == INVOKEVIRTUAL && !stack.isTop() - && getThrowableHandler().checkThrowable(getStack().getStackItem(0)) - && (Objects.equal("getMessage", getNameConstantOperand()) - || Objects.equal("getLocalizedMessage", getNameConstantOperand())); - } - } diff --git a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractDetectorForParameterArray2.java b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractDetectorForParameterArray2.java deleted file mode 100644 index 590f7d5c..00000000 --- a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractDetectorForParameterArray2.java +++ /dev/null @@ -1,65 +0,0 @@ -package jp.skypencil.findbugs.slf4j.parameter; - -import edu.umd.cs.findbugs.BugInstance; -import edu.umd.cs.findbugs.BugReporter; -import edu.umd.cs.findbugs.OpcodeStack.Item; -import javax.annotation.Nullable; -import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy; - -public abstract class AbstractDetectorForParameterArray2 extends AbstractDetectorForParameterArray { - - private final String bugPatternName; - - public AbstractDetectorForParameterArray2(BugReporter bugReporter, String bugPatternName) { - super(bugReporter); - this.bugPatternName = bugPatternName; - } - - @Override - protected final Strategy createArrayCheckStrategy() { - return (storedItem, arrayData, index) -> { - if (arrayData == null) { - return; - } - - if (storedItem.getSpecialKind() == getIsOfInterestKind()) { - arrayData.mark(true); - } - - if (!isReallyOfInterest(storedItem, arrayData, index)) { - arrayData.mark(false); - } - }; - } - - protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int index) { - return true; - } - - @Override - public final void afterOpcode(int seen) { - boolean isInvokingGetMessage = isWhatWeWantToDetect(seen); - super.afterOpcode(seen); - - if (isInvokingGetMessage && !stack.isTop()) { - stack.getStackItem(0).setSpecialKind(getIsOfInterestKind()); - } - } - - @Override - protected final void onLog(@Nullable String format, @Nullable ArrayData arrayData) { - if (arrayData == null || !arrayData.isMarked()) { - return; - } - BugInstance bugInstance = new BugInstance(this, - bugPatternName, NORMAL_PRIORITY) - .addSourceLine(this).addClassAndMethod(this) - .addCalledMethod(this); - getBugReporter().reportBug(bugInstance); - } - - abstract protected boolean isWhatWeWantToDetect(int seen); - - abstract protected int getIsOfInterestKind(); - -} diff --git a/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractExtendedDetectorForParameterArray.java b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractExtendedDetectorForParameterArray.java new file mode 100644 index 00000000..0fa5e025 --- /dev/null +++ b/bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractExtendedDetectorForParameterArray.java @@ -0,0 +1,65 @@ +package jp.skypencil.findbugs.slf4j.parameter; + +import edu.umd.cs.findbugs.BugInstance; +import edu.umd.cs.findbugs.BugReporter; +import edu.umd.cs.findbugs.OpcodeStack.Item; +import javax.annotation.Nullable; +import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy; + +public abstract class AbstractExtendedDetectorForParameterArray extends AbstractDetectorForParameterArray { + + private final String bugPatternName; + + public AbstractExtendedDetectorForParameterArray(BugReporter bugReporter, String bugPatternName) { + super(bugReporter); + this.bugPatternName = bugPatternName; + } + + @Override + protected final Strategy createArrayCheckStrategy() { + return (storedItem, arrayData, index) -> { + if (arrayData == null) { + return; + } + + if (storedItem.getSpecialKind() == getKindOfInterest()) { + arrayData.mark(true); + } + + if (!isReallyOfInterest(storedItem, arrayData, index)) { + arrayData.mark(false); + } + }; + } + + protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int index) { + return true; + } + + @Override + public final void afterOpcode(int seen) { + boolean isInvokingGetMessage = isWhatWeWantToDetect(seen); + super.afterOpcode(seen); + + if (isInvokingGetMessage && !stack.isTop()) { + stack.getStackItem(0).setSpecialKind(getKindOfInterest()); + } + } + + @Override + protected final void onLog(@Nullable String format, @Nullable ArrayData arrayData) { + if (arrayData == null || !arrayData.isMarked()) { + return; + } + BugInstance bugInstance = new BugInstance(this, + bugPatternName, NORMAL_PRIORITY) + .addSourceLine(this).addClassAndMethod(this) + .addCalledMethod(this); + getBugReporter().reportBug(bugInstance); + } + + abstract protected boolean isWhatWeWantToDetect(int seen); + + abstract protected int getKindOfInterest(); + +} diff --git a/bug-pattern/src/main/resources/bugrank.txt b/bug-pattern/src/main/resources/bugrank.txt index 0084d2f3..ff0848c4 100644 --- a/bug-pattern/src/main/resources/bugrank.txt +++ b/bug-pattern/src/main/resources/bugrank.txt @@ -7,3 +7,4 @@ 0 BugPattern SLF4J_ILLEGAL_PASSED_CLASS 0 BugPattern SLF4J_SIGN_ONLY_FORMAT 0 BugPattern SLF4J_MANUALLY_PROVIDED_MESSAGE +0 BugPattern SLF4J_GET_STACK_TRACE diff --git a/bug-pattern/src/main/resources/findbugs.xml b/bug-pattern/src/main/resources/findbugs.xml index 1b352742..980bf0c1 100644 --- a/bug-pattern/src/main/resources/findbugs.xml +++ b/bug-pattern/src/main/resources/findbugs.xml @@ -11,6 +11,8 @@ + + @@ -20,5 +22,6 @@ + diff --git a/bug-pattern/src/main/resources/messages.xml b/bug-pattern/src/main/resources/messages.xml index ca9e091a..a7eef1a3 100644 --- a/bug-pattern/src/main/resources/messages.xml +++ b/bug-pattern/src/main/resources/messages.xml @@ -43,6 +43,12 @@ + +
+ Finds log which uses Throwable#getStackTrace. +
+ + Count of placeholder is not equal to count of parameter Count of placeholder({5}) is not equal to count of parameter({4}) @@ -146,5 +152,17 @@ You cannot use array which is provided as method argument or returned from other + + Do not use Throwable#getStackTrace. + + Do not have use Throwable#getStackTrace. It is enough to provide throwable instance as the last argument, then binding will log its message. + +
+Do not use Throwable#getStackTrace.

+]]> +
+ + SLF4J diff --git a/test-case/.gitignore b/test-case/.gitignore new file mode 100644 index 00000000..da7560e0 --- /dev/null +++ b/test-case/.gitignore @@ -0,0 +1 @@ +/.apt_generated_tests/ diff --git a/test-case/src/main/java/pkg/UsingGetStackTrace.java b/test-case/src/main/java/pkg/UsingGetStackTrace.java new file mode 100644 index 00000000..feb0b14a --- /dev/null +++ b/test-case/src/main/java/pkg/UsingGetStackTrace.java @@ -0,0 +1,13 @@ +package pkg; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class UsingGetStackTrace { + + private final Logger logger = LoggerFactory.getLogger(getClass()); + + void method(RuntimeException ex) { + logger.error("Failed to determine The Answer to Life, The Universe and Everything: {}; cause: {}", 27, ex.getStackTrace()); + } +} diff --git a/test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingGetStackTraceTest.java b/test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingGetStackTraceTest.java new file mode 100644 index 00000000..2d2651dd --- /dev/null +++ b/test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingGetStackTraceTest.java @@ -0,0 +1,25 @@ +package jp.skypencil.findbugs.slf4j; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.Multimap; +import java.util.Collections; +import java.util.Map; +import org.junit.jupiter.api.Test; + +/** + * Test for {@link ManualGetStackTraceDetector}. + * + * @author Michael Vorburger.ch + */ +public class UsingGetStackTraceTest { + + @Test + public void testExceptionMethods() throws Exception { + Map expected = Collections.singletonMap("SLF4J_GET_STACK_TRACE", 1); + Multimap longMessages = new XmlParser().expect(pkg.UsingGetStackTrace.class, expected); + assertThat(longMessages).containsEntry("SLF4J_GET_STACK_TRACE", + "Do not have use Throwable#getStackTrace. It is enough to provide throwable instance as the last argument, then binding will log its message."); + } + +}