From 4ec15db27d948bd51fe87fe9c1787b9ff04247f7 Mon Sep 17 00:00:00 2001 From: Judit Knoll Date: Mon, 22 Apr 2024 18:18:05 +0200 Subject: [PATCH] add lazy init check to instance field init --- .../MultipleInstantiationsOfSingletons.java | 44 +++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletons.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletons.java index d9445ebf8e7..a2ac2dfeb1e 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletons.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletons.java @@ -19,7 +19,16 @@ import edu.umd.cs.findbugs.BugReporter; import edu.umd.cs.findbugs.BugInstance; +import edu.umd.cs.findbugs.ba.CFGBuilderException; +import edu.umd.cs.findbugs.ba.DataflowAnalysisException; import edu.umd.cs.findbugs.ba.SignatureParser; +import edu.umd.cs.findbugs.ba.npe.IsNullValue; +import edu.umd.cs.findbugs.ba.npe.IsNullValueDataflow; +import edu.umd.cs.findbugs.ba.npe.IsNullValueFrame; +import edu.umd.cs.findbugs.ba.vna.AvailableLoad; +import edu.umd.cs.findbugs.ba.vna.ValueNumber; +import edu.umd.cs.findbugs.ba.vna.ValueNumberDataflow; +import edu.umd.cs.findbugs.ba.vna.ValueNumberFrame; import edu.umd.cs.findbugs.bcel.OpcodeStackDetector; import edu.umd.cs.findbugs.OpcodeStack; import edu.umd.cs.findbugs.util.ClassName; @@ -57,8 +66,8 @@ public class MultipleInstantiationsOfSingletons extends OpcodeStackDetector { private boolean isSerializable; private boolean isInstanceAssignOk; private boolean hasNoFactoryMethod; + private boolean isInstanceFieldLazilyInitialized; private XField instanceField; - private final Set lazilyInitializedFields = new HashSet<>(); private final Set eagerlyInitializedFields = new HashSet<>(); private final Map instanceGetterMethods = new HashMap<>(); private final List methodsUsingMonitor = new ArrayList<>(); @@ -86,10 +95,10 @@ public void visit(JavaClass obj) { isSerializable = false; isInstanceAssignOk = false; hasNoFactoryMethod = true; + isInstanceFieldLazilyInitialized = false; instanceField = null; cloneMethod = null; - lazilyInitializedFields.clear(); eagerlyInitializedFields.clear(); instanceGetterMethods.clear(); methodsUsingMonitor.clear(); @@ -135,9 +144,6 @@ public void sawOpcode(int seen) { if (isInstanceField(field, getClassName())) { if (Const.STATIC_INITIALIZER_NAME.equals(getMethodName())) { eagerlyInitializedFields.add(field); - } else { - // TODO check if it's real lazy init - lazilyInitializedFields.add(field); } if (stack.getStackDepth() > 0) { OpcodeStack.Item item = stack.getStackItem(0); @@ -146,6 +152,27 @@ public void sawOpcode(int seen) { && calledMethod.getClassName().equals(getDottedClassName())) { isInstanceAssignOk = true; instanceField = field; + + try { + ValueNumberDataflow vnaDataflow = getClassContext().getValueNumberDataflow(getMethod()); + IsNullValueDataflow invDataflow = getClassContext().getIsNullValueDataflow(getMethod()); + ValueNumberFrame vFrame = vnaDataflow.getAnalysis().getFactAtPC(vnaDataflow.getCFG(), getPC()); + IsNullValueFrame iFrame = invDataflow.getAnalysis().getFactAtPC(invDataflow.getCFG(), getPC()); + AvailableLoad l = new AvailableLoad(field); + ValueNumber[] availableLoads = vFrame.getAvailableLoad(l); + if (availableLoads != null && iFrame.isTrackValueNumbers()) { + for (ValueNumber v : availableLoads) { + IsNullValue knownValue = iFrame.getKnownValue(v); + if (knownValue != null && knownValue.isDefinitelyNull()) { + isInstanceFieldLazilyInitialized = true; + } + } + } + + } catch (DataflowAnalysisException | CFGBuilderException e) { + bugReporter.logError(String.format("Detector %s caught an exception while analyzing %s.", + this.getClass().getName(), getClassContext().getJavaClass().getClassName()), e); + } } } } @@ -186,6 +213,7 @@ private boolean isInstanceField(XField field, String clsName) { @Override public void visitAfter(JavaClass javaClass) { XMethod instanceGetterMethod = instanceGetterMethods.get(instanceField); + boolean isInstanceFieldEagerlyInitialized = eagerlyInitializedFields.contains(instanceField); // a class is considered a singleton, if // - the user clearly indicated, that it's intended to be a singleton @@ -201,7 +229,7 @@ public void visitAfter(JavaClass javaClass) { && isInstanceAssignOk && hasNoFactoryMethod && instanceGetterMethod != null - && (lazilyInitializedFields.contains(instanceField) || eagerlyInitializedFields.contains(instanceField))))) { + && (isInstanceFieldEagerlyInitialized || isInstanceFieldLazilyInitialized)))) { return; } @@ -217,8 +245,8 @@ public void visitAfter(JavaClass javaClass) { boolean isGetterMethodUsingMonitor = methodsUsingMonitor.contains(instanceGetterMethod); - if (instanceGetterMethod != null && !instanceGetterMethod.isSynchronized() && !isGetterMethodUsingMonitor && lazilyInitializedFields.contains( - instanceField)) { + if (instanceGetterMethod != null && !instanceGetterMethod.isSynchronized() && !isGetterMethodUsingMonitor + && isInstanceFieldLazilyInitialized) { bugReporter.reportBug(new BugInstance(this, "SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", NORMAL_PRIORITY).addClass(this) .addMethod(instanceGetterMethod)); }