diff --git a/etc/bugrank.txt b/etc/bugrank.txt index 047858c80..b088e33d8 100644 --- a/etc/bugrank.txt +++ b/etc/bugrank.txt @@ -282,5 +282,6 @@ +0 BugPattern WEM_OBSCURING_EXCEPTION +0 BugPattern WEM_WEAK_EXCEPTION_MESSAGING +0 BugPattern WI_DUPLICATE_WIRED_TYPES ++0 BugPattern WI_MANUALLY_ALLOCATING_AN_AUTOWIRED_BEAN +0 BugPattern WOC_WRITE_ONLY_COLLECTION_FIELD +0 BugPattern WOC_WRITE_ONLY_COLLECTION_LOCAL diff --git a/etc/findbugs.xml b/etc/findbugs.xml index 9396d7295..c8db33ea0 100755 --- a/etc/findbugs.xml +++ b/etc/findbugs.xml @@ -301,7 +301,7 @@ - + @@ -594,6 +594,7 @@ + diff --git a/etc/messages.xml b/etc/messages.xml index f749bc941..40a79aed5 100755 --- a/etc/messages.xml +++ b/etc/messages.xml @@ -5778,6 +5778,19 @@ if (shouldCalcHalting && (calculateHaltingProbability() > 0) { } + + Method allocates an object with new when the class is defined as an autowireable bean + Method {1} allocates an object with new when the class is defined as an autowireable bean +
+ this method allocates an object with new, but the class of the object that is being created is marked with a Spring annotation + denoting that this class is to be used through an @Autowire annotation. Allocating it with new, will likely mean that fields on the + class will not be autowired, but instead be null. You should just autowire an instance of this class into the class in question, or if + need be, use spring's getBean(name) method to fetch one.

+ ]]> +
+ + Method gets and sets a value of a ConcurrentHashMap in a racy manner Method {1} gets and sets a value of a ConcurrentHashMap in a racy manner diff --git a/src/main/java/com/mebigfatguy/fbcontrib/collect/CollectStatistics.java b/src/main/java/com/mebigfatguy/fbcontrib/collect/CollectStatistics.java index 448301a07..d6fea153c 100755 --- a/src/main/java/com/mebigfatguy/fbcontrib/collect/CollectStatistics.java +++ b/src/main/java/com/mebigfatguy/fbcontrib/collect/CollectStatistics.java @@ -26,6 +26,7 @@ import org.apache.bcel.Const; import org.apache.bcel.classfile.AnnotationEntry; +import org.apache.bcel.classfile.Annotations; import org.apache.bcel.classfile.Code; import org.apache.bcel.classfile.JavaClass; import org.apache.bcel.classfile.Method; @@ -44,12 +45,11 @@ import edu.umd.cs.findbugs.ba.ClassContext; /** - * a first pass detector to collect various statistics used in second pass - * detectors. + * a first pass detector to collect various statistics used in second pass detectors. */ public class CollectStatistics extends BytecodeScanningDetector implements NonReportingDetector { - private static final Set COMMON_METHOD_SIG_PREFIXES = UnmodifiableSet.create( - // @formatter:off + private static final Set COMMON_METHOD_SIG_PREFIXES = UnmodifiableSet.create( + // @formatter:off new SignatureBuilder().withMethodName(Values.CONSTRUCTOR).toString(), new SignatureBuilder().withMethodName(Values.TOSTRING).withReturnType(Values.SLASHED_JAVA_LANG_STRING) .toString(), @@ -57,228 +57,242 @@ public class CollectStatistics extends BytecodeScanningDetector implements NonRe "clone()", "values()", new SignatureBuilder().withMethodName("main").withParamTypes(SignatureBuilder.SIG_STRING_ARRAY).toString() // @formatter:on - ); - - private int numMethodCalls; - private boolean modifiesState; - private boolean classHasAnnotation; - private OpcodeStack stack; - private Map> selfCallTree; - private QMethod curMethod; - - /** - * constructs a CollectStatistics detector which clears the singleton that holds - * the statistics for all classes parsed in the first pass. - * - * @param bugReporter - * unused, but required by reflection contract - */ - // required for reflection - @SuppressWarnings("PMD.UnusedFormalParameter") - public CollectStatistics(BugReporter bugReporter) { - Statistics.getStatistics().clear(); - } - - /** - * implements the visitor to collect statistics on this class - * - * @param classContext - * the currently class - */ - @Override - public void visitClassContext(ClassContext classContext) { - try { - JavaClass cls = classContext.getJavaClass(); - AnnotationEntry[] annotations = cls.getAnnotationEntries(); - classHasAnnotation = !CollectionUtils.isEmpty(annotations); - stack = new OpcodeStack(); - selfCallTree = new HashMap<>(); - super.visitClassContext(classContext); - - performModifyStateClosure(classContext.getJavaClass()); - - } finally { - stack = null; - selfCallTree = null; - curMethod = null; - } - } - - @Override - public void visitCode(Code obj) { - - numMethodCalls = 0; - modifiesState = false; - - byte[] code = obj.getCode(); - if (code == null) { - return; - } - stack.resetForMethodEntry(this); - curMethod = null; - super.visitCode(obj); - String clsName = getClassName(); - Method method = getMethod(); - int accessFlags = method.getAccessFlags(); - MethodInfo mi = Statistics.getStatistics().addMethodStatistics(clsName, getMethodName(), getMethodSig(), - accessFlags, obj.getLength(), numMethodCalls); + ); + + private static final Set BEAN_ANNOTATIONS = UnmodifiableSet.create( + // @formatter:off + "Lorg/springframework/stereotype/Component;", + "Lorg/springframework/stereotype/Controller;", + "Lorg/springframework/stereotype/Repository;", + "Lorg/springframework/stereotype/Service;" + // @formatter:on + ); + + private int numMethodCalls; + private boolean modifiesState; + private boolean classHasAnnotation; + private OpcodeStack stack; + private Map> selfCallTree; + private QMethod curMethod; + + /** + * constructs a CollectStatistics detector which clears the singleton that holds the statistics for all classes parsed in the first pass. + * + * @param bugReporter + * unused, but required by reflection contract + */ + // required for reflection + @SuppressWarnings("PMD.UnusedFormalParameter") + public CollectStatistics(BugReporter bugReporter) { + Statistics.getStatistics().clear(); + } + + /** + * implements the visitor to collect statistics on this class + * + * @param classContext + * the currently class + */ + @Override + public void visitClassContext(ClassContext classContext) { + try { + JavaClass cls = classContext.getJavaClass(); + AnnotationEntry[] annotations = cls.getAnnotationEntries(); + classHasAnnotation = !CollectionUtils.isEmpty(annotations); + stack = new OpcodeStack(); + selfCallTree = new HashMap<>(); + super.visitClassContext(classContext); + + performModifyStateClosure(classContext.getJavaClass()); + + } finally { + stack = null; + selfCallTree = null; + curMethod = null; + } + } + + @Override + public void visitAnnotation(Annotations annotations) { + for (AnnotationEntry entry : annotations.getAnnotationEntries()) { + String annotationType = entry.getAnnotationType(); + if (BEAN_ANNOTATIONS.contains(annotationType)) { + Statistics.getStatistics().addAutowiredBean(getDottedClassName()); + } + } + } + + @Override + public void visitCode(Code obj) { + + numMethodCalls = 0; + modifiesState = false; + + byte[] code = obj.getCode(); + if (code == null) { + return; + } + stack.resetForMethodEntry(this); + curMethod = null; + super.visitCode(obj); + String clsName = getClassName(); + Method method = getMethod(); + int accessFlags = method.getAccessFlags(); + MethodInfo mi = Statistics.getStatistics().addMethodStatistics(clsName, getMethodName(), getMethodSig(), accessFlags, obj.getLength(), numMethodCalls); if (clsName.contains("$") || ((accessFlags & (Const.ACC_ABSTRACT | Const.ACC_INTERFACE | Const.ACC_ANNOTATION)) != 0)) { mi.addCallingAccess(Const.ACC_PUBLIC); } else if ((accessFlags & Const.ACC_PRIVATE) == 0) { - if (isAssociationedWithAnnotations(method)) { + if (isAssociationedWithAnnotations(method)) { mi.addCallingAccess(Const.ACC_PUBLIC); - } else { - String methodSig = getMethodName() + getMethodSig(); - for (String sig : COMMON_METHOD_SIG_PREFIXES) { - if (methodSig.startsWith(sig)) { + } else { + String methodSig = getMethodName() + getMethodSig(); + for (String sig : COMMON_METHOD_SIG_PREFIXES) { + if (methodSig.startsWith(sig)) { mi.addCallingAccess(Const.ACC_PUBLIC); - break; - } - } - } - } - - mi.setModifiesState(modifiesState); - } - - @Override - public void sawOpcode(int seen) { - try { - switch (seen) { + break; + } + } + } + } + + mi.setModifiesState(modifiesState); + } + + @Override + public void sawOpcode(int seen) { + try { + switch (seen) { case Const.INVOKEVIRTUAL: case Const.INVOKEINTERFACE: case Const.INVOKESPECIAL: case Const.INVOKESTATIC: case Const.INVOKEDYNAMIC: - numMethodCalls++; + numMethodCalls++; if (seen != Const.INVOKESTATIC) { - int numParms = SignatureUtils.getNumParameters(getSigConstantOperand()); - if (stack.getStackDepth() > numParms) { - OpcodeStack.Item itm = stack.getStackItem(numParms); - if (itm.getRegisterNumber() == 0) { - Set calledMethods; - - if (curMethod == null) { - curMethod = new QMethod(getMethodName(), getMethodSig()); - calledMethods = new HashSet<>(); - selfCallTree.put(curMethod, calledMethods); - } else { - calledMethods = selfCallTree.get(curMethod); - } - - calledMethods.add( - new CalledMethod(new QMethod(getNameConstantOperand(), getSigConstantOperand()), + int numParms = SignatureUtils.getNumParameters(getSigConstantOperand()); + if (stack.getStackDepth() > numParms) { + OpcodeStack.Item itm = stack.getStackItem(numParms); + if (itm.getRegisterNumber() == 0) { + Set calledMethods; + + if (curMethod == null) { + curMethod = new QMethod(getMethodName(), getMethodSig()); + calledMethods = new HashSet<>(); + selfCallTree.put(curMethod, calledMethods); + } else { + calledMethods = selfCallTree.get(curMethod); + } + + calledMethods.add(new CalledMethod(new QMethod(getNameConstantOperand(), getSigConstantOperand()), seen == INVOKESPECIAL)); seen == Const.INVOKESPECIAL)); - } - } - } - break; + } + } + } + break; case Const.PUTSTATIC: case Const.PUTFIELD: - modifiesState = true; - break; - - default: - break; - } - } finally { - stack.sawOpcode(this, seen); - } - } - - private void performModifyStateClosure(JavaClass cls) { - boolean foundNewCall = true; - Statistics statistics = Statistics.getStatistics(); - - String clsName = cls.getClassName(); - while (foundNewCall && !selfCallTree.isEmpty()) { - foundNewCall = false; - - Iterator>> callerIt = selfCallTree.entrySet().iterator(); - while (callerIt.hasNext()) { - Map.Entry> callerEntry = callerIt.next(); - QMethod caller = callerEntry.getKey(); - - MethodInfo callerMi = statistics.getMethodStatistics(clsName, caller.getMethodName(), - caller.getSignature()); - if (callerMi == null) { - // odd, shouldn't happen - foundNewCall = true; - } else if (callerMi.getModifiesState()) { - foundNewCall = true; - } else { - - for (CalledMethod calledMethod : callerEntry.getValue()) { - - if (calledMethod.isSuper) { - callerMi.setModifiesState(true); - foundNewCall = true; - break; - } - MethodInfo calleeMi = statistics.getMethodStatistics(clsName, - calledMethod.callee.getMethodName(), calledMethod.callee.getSignature()); - if (calleeMi == null) { - // a super or sub class probably implements this method so just assume it - // modifies state - callerMi.setModifiesState(true); - foundNewCall = true; - break; - } - - if (calleeMi.getModifiesState()) { - callerMi.setModifiesState(true); - foundNewCall = true; - break; - } - } - } - - if (foundNewCall) { - callerIt.remove(); - } - } - } - - selfCallTree.clear(); - } - - private boolean isAssociationedWithAnnotations(Method m) { - if (classHasAnnotation) { - return true; - } - - return !CollectionUtils.isEmpty(m.getAnnotationEntries()); - } - - /** - * represents a method that is called, and whether it is in the super class - */ - static class CalledMethod { - private QMethod callee; - private boolean isSuper; - - public CalledMethod(QMethod c, boolean s) { - callee = c; - isSuper = s; - } - - @Override - public int hashCode() { - return callee.hashCode() & (isSuper ? 0 : 1); - } - - @Override - public boolean equals(Object obj) { - if (!(obj instanceof CalledMethod)) { - return false; - } - - CalledMethod that = (CalledMethod) obj; - - return (isSuper == that.isSuper) && callee.equals(that.callee); - } - } + modifiesState = true; + break; + + default: + break; + } + } finally { + stack.sawOpcode(this, seen); + } + } + + private void performModifyStateClosure(JavaClass cls) { + boolean foundNewCall = true; + Statistics statistics = Statistics.getStatistics(); + + String clsName = cls.getClassName(); + while (foundNewCall && !selfCallTree.isEmpty()) { + foundNewCall = false; + + Iterator>> callerIt = selfCallTree.entrySet().iterator(); + while (callerIt.hasNext()) { + Map.Entry> callerEntry = callerIt.next(); + QMethod caller = callerEntry.getKey(); + + MethodInfo callerMi = statistics.getMethodStatistics(clsName, caller.getMethodName(), caller.getSignature()); + if (callerMi == null) { + // odd, shouldn't happen + foundNewCall = true; + } else if (callerMi.getModifiesState()) { + foundNewCall = true; + } else { + + for (CalledMethod calledMethod : callerEntry.getValue()) { + + if (calledMethod.isSuper) { + callerMi.setModifiesState(true); + foundNewCall = true; + break; + } + MethodInfo calleeMi = statistics.getMethodStatistics(clsName, calledMethod.callee.getMethodName(), calledMethod.callee.getSignature()); + if (calleeMi == null) { + // a super or sub class probably implements this method so just assume it + // modifies state + callerMi.setModifiesState(true); + foundNewCall = true; + break; + } + + if (calleeMi.getModifiesState()) { + callerMi.setModifiesState(true); + foundNewCall = true; + break; + } + } + } + + if (foundNewCall) { + callerIt.remove(); + } + } + } + + selfCallTree.clear(); + } + + private boolean isAssociationedWithAnnotations(Method m) { + if (classHasAnnotation) { + return true; + } + + return !CollectionUtils.isEmpty(m.getAnnotationEntries()); + } + + /** + * represents a method that is called, and whether it is in the super class + */ + static class CalledMethod { + private QMethod callee; + private boolean isSuper; + + public CalledMethod(QMethod c, boolean s) { + callee = c; + isSuper = s; + } + + @Override + public int hashCode() { + return callee.hashCode() & (isSuper ? 0 : 1); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof CalledMethod)) { + return false; + } + + CalledMethod that = (CalledMethod) obj; + + return (isSuper == that.isSuper) && callee.equals(that.callee); + } + } } diff --git a/src/main/java/com/mebigfatguy/fbcontrib/collect/Statistics.java b/src/main/java/com/mebigfatguy/fbcontrib/collect/Statistics.java index 299f80e82..d298b6a8f 100755 --- a/src/main/java/com/mebigfatguy/fbcontrib/collect/Statistics.java +++ b/src/main/java/com/mebigfatguy/fbcontrib/collect/Statistics.java @@ -19,12 +19,16 @@ package com.mebigfatguy.fbcontrib.collect; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.Map; +import java.util.Set; import com.mebigfatguy.fbcontrib.utils.FQMethod; import com.mebigfatguy.fbcontrib.utils.ToString; +import edu.umd.cs.findbugs.internalAnnotations.DottedClassName; + /** * holds statistics about classes and methods collected in the first pass. */ @@ -35,6 +39,8 @@ public final class Statistics implements Iterable methodStatistics = new HashMap<>(); + private final Set autowiredBeans = new HashSet<>(); + private Statistics() { } @@ -84,6 +90,14 @@ public void addImmutabilityStatus(String className, String methodName, String si mi.setImmutabilityType(imType); } + public void addAutowiredBean(@DottedClassName String beanClass) { + autowiredBeans.add(beanClass); + } + + public boolean isAutowiredBean(@DottedClassName String beanClass) { + return autowiredBeans.contains(beanClass); + } + @Override public String toString() { return ToString.build(this); diff --git a/src/main/java/com/mebigfatguy/fbcontrib/detect/WiringIssues.java b/src/main/java/com/mebigfatguy/fbcontrib/detect/WiringIssues.java index af21045b4..8386cd18e 100644 --- a/src/main/java/com/mebigfatguy/fbcontrib/detect/WiringIssues.java +++ b/src/main/java/com/mebigfatguy/fbcontrib/detect/WiringIssues.java @@ -26,16 +26,17 @@ import org.apache.bcel.classfile.Field; import org.apache.bcel.classfile.JavaClass; +import com.mebigfatguy.fbcontrib.collect.Statistics; import com.mebigfatguy.fbcontrib.utils.BugType; import com.mebigfatguy.fbcontrib.utils.ToString; import com.mebigfatguy.fbcontrib.utils.Values; import edu.umd.cs.findbugs.BugInstance; import edu.umd.cs.findbugs.BugReporter; +import edu.umd.cs.findbugs.BytecodeScanningDetector; import edu.umd.cs.findbugs.Detector; import edu.umd.cs.findbugs.FieldAnnotation; import edu.umd.cs.findbugs.ba.ClassContext; -import edu.umd.cs.findbugs.visitclass.PreorderVisitor; /** * looks for various issues around @Autowired/@Inject fields in DI classes @@ -43,10 +44,11 @@ *
  • Injecting the same bean twice into the same class hierarchy, even with different field names
    • * */ -public class WiringIssues extends PreorderVisitor implements Detector { +public class WiringIssues extends BytecodeScanningDetector implements Detector { private static final String SPRING_AUTOWIRED = "Lorg/springframework/beans/factory/annotation/Autowired;"; private static final String SPRING_QUALIFIER = "Lorg/springframework/beans/factory/annotation/Qualifier;"; + private BugReporter bugReporter; /** @@ -105,14 +107,23 @@ public void visitClassContext(ClassContext classContext) { } } } + + super.visitClassContext(classContext); } catch (ClassNotFoundException e) { bugReporter.reportMissingClass(e); } } @Override - public void report() { - // required by the interface, but not used + public void sawOpcode(int seen) { + if ((seen == INVOKESPECIAL) && Values.CONSTRUCTOR.equals(getNameConstantOperand())) { + String clsName = getClassConstantOperand(); + if (Statistics.getStatistics().isAutowiredBean(clsName.replace('/', '.'))) { + bugReporter.reportBug(new BugInstance(this, BugType.WI_MANUALLY_ALLOCATING_AN_AUTOWIRED_BEAN.name(), NORMAL_PRIORITY).addClass(this) + .addMethod(this).addSourceLine(this)); + + } + } } /** diff --git a/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java b/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java index 6a19ed3fd..2f95099a0 100644 --- a/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java +++ b/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java @@ -323,6 +323,7 @@ public enum BugType { WEM_OBSCURING_EXCEPTION, WEM_WEAK_EXCEPTION_MESSAGING, WI_DUPLICATE_WIRED_TYPES, + WI_MANUALLY_ALLOCATING_AN_AUTOWIRED_BEAN, WOC_WRITE_ONLY_COLLECTION_FIELD, WOC_WRITE_ONLY_COLLECTION_LOCAL; // @formatter:on diff --git a/src/samples/java/ex/WI_Sample.java b/src/samples/java/ex/WI_Sample.java index 7d2a82943..ce69349f6 100644 --- a/src/samples/java/ex/WI_Sample.java +++ b/src/samples/java/ex/WI_Sample.java @@ -1,6 +1,8 @@ package ex; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.stereotype.Component; public class WI_Sample extends Parent { @Autowired @@ -15,6 +17,10 @@ public class WI_Sample extends Parent { @Autowired GenerifiedBean fpIntBean; + + public void foo() { + MyBean b = new MyBean(); + } } class Parent { @@ -32,3 +38,7 @@ interface SingletonBean { interface GenerifiedBean { } + +@Component +class MyBean { +}