diff --git a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java index 265dac37fbf..e1c4bbade1c 100644 --- a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java +++ b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java @@ -27,6 +27,7 @@ import org.checkerframework.dataflow.cfg.node.Node; import org.checkerframework.dataflow.expression.JavaExpression; import org.checkerframework.framework.flow.CFAbstractStore; +import org.checkerframework.framework.flow.CFAbstractValue; import org.checkerframework.framework.type.AnnotatedTypeMirror; import org.checkerframework.framework.util.JavaExpressionParseUtil; import org.checkerframework.framework.util.StringToJavaExpression; @@ -185,21 +186,25 @@ public void accumulate( * allows propagation, along those paths, of the fact that the method being invoked in {@code * node} was definitely called. * + * @param the type of store + * @param the type of abstract values * @param node a method invocation * @param input the transfer input associated with the method invocation * @return a map from types to stores. The keys are the same keys used by {@link * ExceptionBlock#getExceptionalSuccessors()}. The values are copies of the regular store from * {@code input}. */ - private Map makeExceptionalStores( - MethodInvocationNode node, TransferInput input) { + public static , S extends CFAbstractStore> + Map makeExceptionalStores( + MethodInvocationNode node, TransferInput input) { if (!(node.getBlock() instanceof ExceptionBlock)) { // This can happen in some weird (buggy?) cases: // see https://github.com/typetools/checker-framework/issues/3585 return Collections.emptyMap(); } + ExceptionBlock block = (ExceptionBlock) node.getBlock(); - Map result = new LinkedHashMap<>(); + Map result = new LinkedHashMap<>(); block .getExceptionalSuccessors() .forEach((tm, b) -> result.put(tm, input.getRegularStore().copy())); diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnalysis.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnalysis.java new file mode 100644 index 00000000000..6878ae60b87 --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnalysis.java @@ -0,0 +1,46 @@ +package org.checkerframework.checker.mustcall; + +import com.google.common.collect.ImmutableSet; +import com.sun.tools.javac.code.Type; +import java.util.Set; +import javax.lang.model.type.TypeMirror; +import org.checkerframework.checker.signature.qual.CanonicalName; +import org.checkerframework.framework.flow.CFAnalysis; + +/** + * The analysis for the Must Call Checker. The analysis is specialized to ignore certain exception + * types; see {@link #isIgnoredExceptionType(TypeMirror)}. + */ +public class MustCallAnalysis extends CFAnalysis { + + /** + * Constructs an MustCallAnalysis. + * + * @param checker the checker + * @param factory the type factory + */ + public MustCallAnalysis(MustCallChecker checker, MustCallAnnotatedTypeFactory factory) { + super(checker, factory); + } + + /** + * The fully-qualified names of the exception types that are ignored by this checker when + * computing dataflow stores. + */ + protected static final Set<@CanonicalName String> ignoredExceptionTypes = + ImmutableSet.of( + // Use the Nullness Checker instead. + NullPointerException.class.getCanonicalName(), + // Ignore run-time errors, which cannot be predicted statically. Doing + // so is unsound in the sense that they could still occur - e.g., the + // program could run out of memory - but if they did, the checker's + // results would be useless anyway. + Error.class.getCanonicalName(), + RuntimeException.class.getCanonicalName()); + + @Override + public boolean isIgnoredExceptionType(TypeMirror exceptionType) { + return ignoredExceptionTypes.contains( + ((Type) exceptionType).tsym.getQualifiedName().toString()); + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java index 26137ddd6b0..bdde9b3f745 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java @@ -23,6 +23,7 @@ import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.TypeElement; +import javax.lang.model.type.TypeKind; import javax.lang.model.type.TypeMirror; import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; import org.checkerframework.checker.mustcall.qual.InheritableMustCall; @@ -164,6 +165,11 @@ protected Set> createSupportedTypeQualifiers() { Arrays.asList(MustCall.class, MustCallUnknown.class, PolyMustCall.class)); } + @Override + protected MustCallAnalysis createFlowAnalysis() { + return new MustCallAnalysis((MustCallChecker) checker, this); + } + @Override protected TreeAnnotator createTreeAnnotator() { return new ListTreeAnnotator(super.createTreeAnnotator(), new MustCallTreeAnnotator(this)); @@ -226,6 +232,16 @@ protected void constructorFromUsePreSubstitution( super.constructorFromUsePreSubstitution(tree, type, resolvePolyQuals); } + @Override + public boolean isIgnoredExceptionType(TypeMirror exceptionType) { + if (exceptionType.getKind() == TypeKind.DECLARED) { + return ((MustCallChecker) checker) + .getIgnoredExceptions() + .contains(analysis.getTypes(), exceptionType); + } + return false; + } + /** * Class to implement the customized semantics of {@link MustCallAlias} (and {@link PolyMustCall}) * annotations; see the {@link MustCallAlias} documentation for details. diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java index 617fe571db6..684b2ff64c2 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java @@ -1,6 +1,17 @@ package org.checkerframework.checker.mustcall; +import com.google.common.collect.ImmutableSet; +import java.io.UnsupportedEncodingException; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.lang.model.element.TypeElement; +import javax.lang.model.type.TypeMirror; +import javax.tools.Diagnostic; import org.checkerframework.checker.mustcall.qual.MustCall; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; +import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.framework.qual.StubFiles; import org.checkerframework.framework.source.SupportedOptions; @@ -19,10 +30,14 @@ @SupportedOptions({ MustCallChecker.NO_CREATES_MUSTCALLFOR, MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP, - MustCallChecker.NO_RESOURCE_ALIASES + MustCallChecker.NO_RESOURCE_ALIASES, + MustCallChecker.IGNORED_EXCEPTIONS }) public class MustCallChecker extends BaseTypeChecker { + /** Creates a MustCallChecker. */ + public MustCallChecker() {} + /** * Disables @CreatesMustCallFor support. Not of interest to most users. Not documented in the * manual. @@ -39,4 +54,180 @@ public class MustCallChecker extends BaseTypeChecker { * Disables @MustCallAlias support. Not of interest to most users. Not documented in the manual. */ public static final String NO_RESOURCE_ALIASES = "noResourceAliases"; + + /** + * The exception types in this set are ignored in the CFG when determining if a resource leaks + * along an exceptional path. These kinds of errors fall into a few categories: runtime errors, + * errors that the JVM can issue on any statement, and errors that can be prevented by running + * some other CF checker. + */ + public static final SetOfTypes DEFAULT_IGNORED_EXCEPTIONS = + SetOfTypes.anyOfTheseNames( + ImmutableSet.of( + // Any method call has a CFG edge for Throwable/RuntimeException/Error + // to represent run-time misbehavior. Ignore it. + Throwable.class.getCanonicalName(), + Error.class.getCanonicalName(), + RuntimeException.class.getCanonicalName(), + // Use the Nullness Checker to prove this won't happen. + NullPointerException.class.getCanonicalName(), + // These errors can't be predicted statically, so ignore them and assume + // they won't happen. + ClassCircularityError.class.getCanonicalName(), + ClassFormatError.class.getCanonicalName(), + NoClassDefFoundError.class.getCanonicalName(), + OutOfMemoryError.class.getCanonicalName(), + // It's not our problem if the Java type system is wrong. + ClassCastException.class.getCanonicalName(), + // It's not our problem if the code is going to divide by zero. + ArithmeticException.class.getCanonicalName(), + // Use the Index Checker to prevent these errors. + ArrayIndexOutOfBoundsException.class.getCanonicalName(), + NegativeArraySizeException.class.getCanonicalName(), + // Most of the time, this exception is infeasible, as the charset used + // is guaranteed to be present by the Java spec (e.g., "UTF-8"). + // Eventually, this exclusion could be refined by looking at the charset + // being requested. + UnsupportedEncodingException.class.getCanonicalName())); + + /** + * Command-line option for controlling which exceptions are ignored. + * + * @see #DEFAULT_IGNORED_EXCEPTIONS + * @see #getIgnoredExceptions() + */ + public static final String IGNORED_EXCEPTIONS = "resourceLeakIgnoredExceptions"; + + /** + * A pattern that matches one or more consecutive commas, optionally preceded and followed by + * whitespace. + */ + public static final Pattern COMMAS = Pattern.compile("\\s*(?:" + Pattern.quote(",") + "\\s*)+"); + + /** + * A pattern that matches an exception specifier for the {@link #IGNORED_EXCEPTIONS} option: an + * optional "=" followed by a qualified name. The whole thing can be padded with whitespace. + */ + public static final Pattern EXCEPTION_SPECIFIER = + Pattern.compile( + "^\\s*" + "(" + Pattern.quote("=") + "\\s*" + ")?" + "(\\w+(?:\\.\\w+)*)" + "\\s*$"); + + /** + * The cached set of ignored exceptions parsed from {@link #IGNORED_EXCEPTIONS}. Caching this + * field prevents the checker from issuing duplicate warnings about missing exception types. + * + * @see #getIgnoredExceptions() + */ + private @MonotonicNonNull SetOfTypes ignoredExceptions = null; + + /** + * Get the set of exceptions that should be ignored. This set comes from the {@link + * #IGNORED_EXCEPTIONS} option if it was provided, or {@link #DEFAULT_IGNORED_EXCEPTIONS} if not. + * + * @return the set of exceptions to ignore + */ + public SetOfTypes getIgnoredExceptions() { + SetOfTypes result = ignoredExceptions; + if (result == null) { + String ignoredExceptionsOptionValue = getOption(IGNORED_EXCEPTIONS); + result = + ignoredExceptionsOptionValue == null + ? DEFAULT_IGNORED_EXCEPTIONS + : parseIgnoredExceptions(ignoredExceptionsOptionValue); + ignoredExceptions = result; + } + return result; + } + + /** + * Parse the argument given for the {@link #IGNORED_EXCEPTIONS} option. Warnings will be issued + * for any problems in the argument, for instance if any of the named exceptions cannot be found. + * + * @param ignoredExceptionsOptionValue the value given for {@link #IGNORED_EXCEPTIONS} + * @return the set of ignored exceptions + */ + protected SetOfTypes parseIgnoredExceptions(String ignoredExceptionsOptionValue) { + String[] exceptions = COMMAS.split(ignoredExceptionsOptionValue); + List sets = new ArrayList<>(); + for (String e : exceptions) { + SetOfTypes set = parseExceptionSpecifier(e, ignoredExceptionsOptionValue); + if (set != null) { + sets.add(set); + } + } + return SetOfTypes.union(sets.toArray(new SetOfTypes[0])); + } + + /** + * Parse a single exception specifier from the {@link #IGNORED_EXCEPTIONS} option and issue + * warnings if it does not parse. See {@link #EXCEPTION_SPECIFIER} for a description of the + * syntax. + * + * @param exceptionSpecifier the exception specifier to parse + * @param ignoredExceptionsOptionValue the whole value of the {@link #IGNORED_EXCEPTIONS} option; + * only used for error reporting + * @return the parsed set of types, or null if the value does not parse + */ + @SuppressWarnings({ + // user input might not be a legal @CanonicalName, but it should be safe to pass to + // `SetOfTypes.anyOfTheseNames` + "signature:type.arguments.not.inferred", + }) + protected @Nullable SetOfTypes parseExceptionSpecifier( + String exceptionSpecifier, String ignoredExceptionsOptionValue) { + Matcher m = EXCEPTION_SPECIFIER.matcher(exceptionSpecifier); + if (m.matches()) { + @Nullable String equalsSign = m.group(1); + String qualifiedName = m.group(2); + + if (qualifiedName.equalsIgnoreCase("default")) { + return DEFAULT_IGNORED_EXCEPTIONS; + } + TypeMirror type = checkCanonicalName(qualifiedName); + if (type == null) { + // There is a chance that the user named a real type, but the class is not + // accessible for some reason. We'll issue a warning (in case this was a typo) but + // add the type as ignored anyway (in case it's just an inaccessible type). + // + // Note that if the user asked to ignore subtypes of this exception, this code won't + // do it because we can't know what those subtypes are. We have to treat this as if + // it were "=qualifiedName" even if no equals sign was provided. + message( + Diagnostic.Kind.WARNING, + "The exception '%s' appears in the -A%s=%s option, but it does not seem to exist", + exceptionSpecifier, + IGNORED_EXCEPTIONS, + ignoredExceptionsOptionValue); + return SetOfTypes.anyOfTheseNames(ImmutableSet.of(qualifiedName)); + } else { + return equalsSign == null ? SetOfTypes.allSubtypes(type) : SetOfTypes.singleton(type); + } + } else if (!exceptionSpecifier.trim().isEmpty()) { + message( + Diagnostic.Kind.WARNING, + "The string '%s' appears in the -A%s=%s option," + + " but it is not a legal exception specifier", + exceptionSpecifier, + IGNORED_EXCEPTIONS, + ignoredExceptionsOptionValue); + } + return null; + } + + /** + * Check if the given String refers to an actual type. + * + * @param s any string + * @return the referenced type, or null if it does not exist + */ + @SuppressWarnings({ + "signature:argument", // `s` is not a qualified name, but we pass it to getTypeElement + }) + protected @Nullable TypeMirror checkCanonicalName(String s) { + TypeElement elem = getProcessingEnvironment().getElementUtils().getTypeElement(s); + if (elem == null) { + return null; + } + return types.getDeclaredType(elem); + } } diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java index dd088585231..700794d5066 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java @@ -6,16 +6,21 @@ import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicLong; import javax.annotation.processing.ProcessingEnvironment; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.type.TypeMirror; +import org.checkerframework.checker.calledmethods.CalledMethodsTransfer; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.dataflow.analysis.ConditionalTransferResult; +import org.checkerframework.dataflow.analysis.RegularTransferResult; import org.checkerframework.dataflow.analysis.TransferInput; import org.checkerframework.dataflow.analysis.TransferResult; +import org.checkerframework.dataflow.cfg.block.ExceptionBlock; import org.checkerframework.dataflow.cfg.node.LocalVariableNode; import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.dataflow.cfg.node.Node; @@ -24,7 +29,6 @@ import org.checkerframework.dataflow.cfg.node.SwitchExpressionNode; import org.checkerframework.dataflow.cfg.node.TernaryExpressionNode; import org.checkerframework.dataflow.expression.JavaExpression; -import org.checkerframework.framework.flow.CFAnalysis; import org.checkerframework.framework.flow.CFStore; import org.checkerframework.framework.flow.CFTransfer; import org.checkerframework.framework.flow.CFValue; @@ -67,7 +71,7 @@ public class MustCallTransfer extends CFTransfer { * * @param analysis the analysis */ - public MustCallTransfer(CFAnalysis analysis) { + public MustCallTransfer(MustCallAnalysis analysis) { super(analysis); atypeFactory = (MustCallAnnotatedTypeFactory) analysis.getTypeFactory(); noCreatesMustCallFor = @@ -137,7 +141,27 @@ public TransferResult visitMethodInvocation( lubWithStoreValue(store, targetExpr, defaultType); } } + + if (n.getBlock() instanceof ExceptionBlock) { + Map exceptionalStores = + CalledMethodsTransfer.makeExceptionalStores(n, in); + // Update stores for the exceptional paths to handle cases like: + // https://github.com/typetools/checker-framework/issues/6050 + if (result.containsTwoStores()) { + result = + new ConditionalTransferResult<>( + result.getResultValue(), + result.getThenStore().copy(), + result.getElseStore().copy(), + exceptionalStores); + } else { + result = + new RegularTransferResult<>( + result.getResultValue(), result.getRegularStore().copy(), exceptionalStores); + } + } } + return result; } diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java b/checker/src/main/java/org/checkerframework/checker/mustcall/SetOfTypes.java similarity index 98% rename from checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java rename to checker/src/main/java/org/checkerframework/checker/mustcall/SetOfTypes.java index 4187adfc506..c0ff74bdd44 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/SetOfTypes.java @@ -1,4 +1,4 @@ -package org.checkerframework.checker.resourceleak; +package org.checkerframework.checker.mustcall; import com.google.common.collect.ImmutableSet; import com.sun.tools.javac.code.Type; diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java index d17dcfc8c6a..94fc239f989 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -2121,17 +2121,7 @@ private void propagateObligationsToSuccessorBlock( // set in the successor store, use the current block's MC store, // which contain whatever information is true before this (empty) block. // 2. if the current block has one or more nodes, always use the CM store after - // the last node. To decide which MC store to use: - // 2a. if the last node in the block is the invocation of an - // @CreatesMustCallFor method that might throw an exception, and the - // consistency check is for an exceptional path, use the MC store - // immediately before the method invocation, because the method threw an - // exception rather than finishing and therefore did not actually create - // any must-call obligation, so the MC store after might contain - // must-call obligations that do not need to be fulfilled along this - // path. - // 2b. in all other cases, use the MC store from after the last node in - // the block. + // the last node. CFStore mcStore; AccumulationStore cmStore; if (currentBlockNodes.size() == 0 /* currentBlock is special or conditional */) { @@ -2159,18 +2149,12 @@ private void propagateObligationsToSuccessorBlock( cmStore = typeFactory.getStoreAfter(last); cmStoreAfter.put(last, cmStore); } - // If this is an exceptional block, check the MC store beforehand to avoid - // issuing an error about a call to a CreatesMustCallFor method that might - // throw an exception. Otherwise, use the store after. - if (exceptionType != null && isInvocationOfCreatesMustCallForMethod(last)) { - mcStore = mcAtf.getStoreBefore(last); // 2a. (MC) + + if (mcStoreAfter.containsKey(last)) { + mcStore = mcStoreAfter.get(last); } else { - if (mcStoreAfter.containsKey(last)) { - mcStore = mcStoreAfter.get(last); - } else { - mcStore = mcAtf.getStoreAfter(last); // 2b. (MC) - mcStoreAfter.put(last, mcStore); - } + mcStore = mcAtf.getStoreAfter(last); // 2b. (MC) + mcStoreAfter.put(last, mcStore); } } @@ -2234,21 +2218,6 @@ private boolean aliasInScopeInSuccessor(AccumulationStore successorStore, Resour return successorStore.getValue(alias.reference) != null; } - /** - * Returns true if node is a MethodInvocationNode of a method with a CreatesMustCallFor - * annotation. - * - * @param node a node - * @return true if node is a MethodInvocationNode of a method with a CreatesMustCallFor annotation - */ - private boolean isInvocationOfCreatesMustCallForMethod(Node node) { - if (!(node instanceof MethodInvocationNode)) { - return false; - } - MethodInvocationNode miNode = (MethodInvocationNode) node; - return typeFactory.hasCreatesMustCallFor(miNode); - } - /** * Finds {@link Owning} formal parameters for the method corresponding to a CFG. * diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java index 1dab92a1da8..ba03a6d69a0 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java @@ -3,17 +3,19 @@ import javax.lang.model.type.TypeMirror; import org.checkerframework.checker.calledmethods.CalledMethodsAnalysis; import org.checkerframework.checker.calledmethods.CalledMethodsAnnotatedTypeFactory; +import org.checkerframework.checker.mustcall.MustCallChecker; +import org.checkerframework.checker.mustcall.MustCallNoCreatesMustCallForChecker; +import org.checkerframework.checker.mustcall.SetOfTypes; /** * This variant of CFAnalysis extends the set of ignored exception types. * - * @see ResourceLeakChecker#getIgnoredExceptions() + * @see MustCallChecker#getIgnoredExceptions() */ public class ResourceLeakAnalysis extends CalledMethodsAnalysis { /** - * The set of exceptions to ignore, cached from {@link - * ResourceLeakChecker#getIgnoredExceptions()}. + * The set of exceptions to ignore, cached from {@link MustCallChecker#getIgnoredExceptions()}. */ private final SetOfTypes ignoredExceptions; @@ -26,7 +28,11 @@ public class ResourceLeakAnalysis extends CalledMethodsAnalysis { protected ResourceLeakAnalysis( ResourceLeakChecker checker, CalledMethodsAnnotatedTypeFactory factory) { super(checker, factory); - this.ignoredExceptions = checker.getIgnoredExceptions(); + MustCallChecker mustCallChecker = checker.getSubchecker(MustCallChecker.class); + if (mustCallChecker == null) { + mustCallChecker = checker.getSubchecker(MustCallNoCreatesMustCallForChecker.class); + } + this.ignoredExceptions = mustCallChecker.getIgnoredExceptions(); } @Override diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java index 6476ff4c57d..cfd49457750 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java @@ -1,20 +1,11 @@ package org.checkerframework.checker.resourceleak; -import com.google.common.collect.ImmutableSet; -import java.io.UnsupportedEncodingException; -import java.util.ArrayList; -import java.util.List; import java.util.Set; -import java.util.regex.Matcher; -import java.util.regex.Pattern; -import javax.lang.model.element.TypeElement; -import javax.lang.model.type.TypeMirror; import javax.tools.Diagnostic; import org.checkerframework.checker.calledmethods.CalledMethodsChecker; import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey; import org.checkerframework.checker.mustcall.MustCallChecker; import org.checkerframework.checker.mustcall.MustCallNoCreatesMustCallForChecker; -import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.common.basetype.BaseTypeVisitor; @@ -30,7 +21,7 @@ "permitStaticOwning", "permitInitializationLeak", ResourceLeakChecker.COUNT_MUST_CALL, - ResourceLeakChecker.IGNORED_EXCEPTIONS, + MustCallChecker.IGNORED_EXCEPTIONS, MustCallChecker.NO_CREATES_MUSTCALLFOR, MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP, MustCallChecker.NO_RESOURCE_ALIASES, @@ -49,63 +40,6 @@ public ResourceLeakChecker() {} */ public static final String COUNT_MUST_CALL = "countMustCall"; - /** - * The exception types in this set are ignored in the CFG when determining if a resource leaks - * along an exceptional path. These kinds of errors fall into a few categories: runtime errors, - * errors that the JVM can issue on any statement, and errors that can be prevented by running - * some other CF checker. - */ - private static final SetOfTypes DEFAULT_IGNORED_EXCEPTIONS = - SetOfTypes.anyOfTheseNames( - ImmutableSet.of( - // Any method call has a CFG edge for Throwable/RuntimeException/Error - // to represent run-time misbehavior. Ignore it. - Throwable.class.getCanonicalName(), - Error.class.getCanonicalName(), - RuntimeException.class.getCanonicalName(), - // Use the Nullness Checker to prove this won't happen. - NullPointerException.class.getCanonicalName(), - // These errors can't be predicted statically, so ignore them and assume - // they won't happen. - ClassCircularityError.class.getCanonicalName(), - ClassFormatError.class.getCanonicalName(), - NoClassDefFoundError.class.getCanonicalName(), - OutOfMemoryError.class.getCanonicalName(), - // It's not our problem if the Java type system is wrong. - ClassCastException.class.getCanonicalName(), - // It's not our problem if the code is going to divide by zero. - ArithmeticException.class.getCanonicalName(), - // Use the Index Checker to prevent these errors. - ArrayIndexOutOfBoundsException.class.getCanonicalName(), - NegativeArraySizeException.class.getCanonicalName(), - // Most of the time, this exception is infeasible, as the charset used - // is guaranteed to be present by the Java spec (e.g., "UTF-8"). - // Eventually, this exclusion could be refined by looking at the charset - // being requested. - UnsupportedEncodingException.class.getCanonicalName())); - - /** - * Command-line option for controlling which exceptions are ignored. - * - * @see #DEFAULT_IGNORED_EXCEPTIONS - * @see #getIgnoredExceptions() - */ - public static final String IGNORED_EXCEPTIONS = "resourceLeakIgnoredExceptions"; - - /** - * A pattern that matches one or more consecutive commas, optionally preceded and followed by - * whitespace. - */ - private static final Pattern COMMAS = Pattern.compile("\\s*(?:" + Pattern.quote(",") + "\\s*)+"); - - /** - * A pattern that matches an exception specifier for the {@link #IGNORED_EXCEPTIONS} option: an - * optional "=" followed by a qualified name. The whole thing can be padded with whitespace. - */ - private static final Pattern EXCEPTION_SPECIFIER = - Pattern.compile( - "^\\s*" + "(" + Pattern.quote("=") + "\\s*" + ")?" + "(\\w+(?:\\.\\w+)*)" + "\\s*$"); - /** * Ordinarily, when the -Ainfer flag is used, whole-program inference is run for every checker and * sub-checker. However, the Resource Leak Checker is different. The -Ainfer flag enables the @@ -127,14 +61,6 @@ public ResourceLeakChecker() {} */ private int numMustCallFailed = 0; - /** - * The cached set of ignored exceptions parsed from {@link #IGNORED_EXCEPTIONS}. Caching this - * field prevents the checker from issuing duplicate warnings about missing exception types. - * - * @see #getIgnoredExceptions() - */ - private @MonotonicNonNull SetOfTypes ignoredExceptions = null; - @Override protected Set> getImmediateSubcheckerClasses() { Set> checkers = super.getImmediateSubcheckerClasses(); @@ -178,115 +104,4 @@ public void typeProcessingOver() { } super.typeProcessingOver(); } - - /** - * Get the set of exceptions that should be ignored. This set comes from the {@link - * #IGNORED_EXCEPTIONS} option if it was provided, or {@link #DEFAULT_IGNORED_EXCEPTIONS} if not. - * - * @return the set of exceptions to ignore - */ - public SetOfTypes getIgnoredExceptions() { - SetOfTypes result = ignoredExceptions; - if (result == null) { - String ignoredExceptionsOptionValue = getOption(IGNORED_EXCEPTIONS); - result = - ignoredExceptionsOptionValue == null - ? DEFAULT_IGNORED_EXCEPTIONS - : parseIgnoredExceptions(ignoredExceptionsOptionValue); - ignoredExceptions = result; - } - return result; - } - - /** - * Parse the argument given for the {@link #IGNORED_EXCEPTIONS} option. Warnings will be issued - * for any problems in the argument, for instance if any of the named exceptions cannot be found. - * - * @param ignoredExceptionsOptionValue the value given for {@link #IGNORED_EXCEPTIONS} - * @return the set of ignored exceptions - */ - protected SetOfTypes parseIgnoredExceptions(String ignoredExceptionsOptionValue) { - String[] exceptions = COMMAS.split(ignoredExceptionsOptionValue); - List sets = new ArrayList<>(); - for (String e : exceptions) { - SetOfTypes set = parseExceptionSpecifier(e, ignoredExceptionsOptionValue); - if (set != null) { - sets.add(set); - } - } - return SetOfTypes.union(sets.toArray(new SetOfTypes[0])); - } - - /** - * Parse a single exception specifier from the {@link #IGNORED_EXCEPTIONS} option and issue - * warnings if it does not parse. See {@link #EXCEPTION_SPECIFIER} for a description of the - * syntax. - * - * @param exceptionSpecifier the exception specifier to parse - * @param ignoredExceptionsOptionValue the whole value of the {@link #IGNORED_EXCEPTIONS} option; - * only used for error reporting - * @return the parsed set of types, or null if the value does not parse - */ - @SuppressWarnings({ - // user input might not be a legal @CanonicalName, but it should be safe to pass to - // `SetOfTypes.anyOfTheseNames` - "signature:type.arguments.not.inferred", - }) - protected @Nullable SetOfTypes parseExceptionSpecifier( - String exceptionSpecifier, String ignoredExceptionsOptionValue) { - Matcher m = EXCEPTION_SPECIFIER.matcher(exceptionSpecifier); - if (m.matches()) { - @Nullable String equalsSign = m.group(1); - String qualifiedName = m.group(2); - - if (qualifiedName.equalsIgnoreCase("default")) { - return DEFAULT_IGNORED_EXCEPTIONS; - } - TypeMirror type = checkCanonicalName(qualifiedName); - if (type == null) { - // There is a chance that the user named a real type, but the class is not - // accessible for some reason. We'll issue a warning (in case this was a typo) but - // add the type as ignored anyway (in case it's just an inaccessible type). - // - // Note that if the user asked to ignore subtypes of this exception, this code won't - // do it because we can't know what those subtypes are. We have to treat this as if - // it were "=qualifiedName" even if no equals sign was provided. - message( - Diagnostic.Kind.WARNING, - "The exception '%s' appears in the -A%s=%s option, but it does not seem to exist", - exceptionSpecifier, - IGNORED_EXCEPTIONS, - ignoredExceptionsOptionValue); - return SetOfTypes.anyOfTheseNames(ImmutableSet.of(qualifiedName)); - } else { - return equalsSign == null ? SetOfTypes.allSubtypes(type) : SetOfTypes.singleton(type); - } - } else if (!exceptionSpecifier.trim().isEmpty()) { - message( - Diagnostic.Kind.WARNING, - "The string '%s' appears in the -A%s=%s option," - + " but it is not a legal exception specifier", - exceptionSpecifier, - IGNORED_EXCEPTIONS, - ignoredExceptionsOptionValue); - } - return null; - } - - /** - * Check if the given String refers to an actual type. - * - * @param s any string - * @return the referenced type, or null if it does not exist - */ - @SuppressWarnings({ - "signature:argument", // `s` is not a qualified name, but we pass it to getTypeElement - }) - protected @Nullable TypeMirror checkCanonicalName(String s) { - TypeElement elem = getProcessingEnvironment().getElementUtils().getTypeElement(s); - if (elem == null) { - return null; - } - return types.getDeclaredType(elem); - } } diff --git a/checker/tests/resourceleak/ACSocketTest.java b/checker/tests/resourceleak/ACSocketTest.java index 5f6dfb0c8a8..571e8cc94ac 100644 --- a/checker/tests/resourceleak/ACSocketTest.java +++ b/checker/tests/resourceleak/ACSocketTest.java @@ -360,10 +360,13 @@ void createNewServerSocket(InetSocketAddress address, boolean b, boolean c) thro ServerSocket socket; if (b) { + // :: error: required.method.not.called socket = new ServerSocket(); } else if (c) { + // :: error: required.method.not.called socket = new ServerSocket(); } else { + // :: error: required.method.not.called socket = new ServerSocket(); } diff --git a/checker/tests/resourceleak/CMFOnExceptionalSucessors.java b/checker/tests/resourceleak/CMFOnExceptionalSucessors.java new file mode 100644 index 00000000000..f9daf531562 --- /dev/null +++ b/checker/tests/resourceleak/CMFOnExceptionalSucessors.java @@ -0,0 +1,101 @@ +// This test checks whether methods with the @CreatesMustCallFor annotation create obligations on +// both normal and exceptional successors. +// See https://github.com/typetools/checker-framework/issues/6050 + +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class CMFOnExceptionalSucessors { + + @InheritableMustCall("a") + class Foo { + void a() {} + } + + @InheritableMustCall("a") + class Bar { + @Owning Foo f; + + // :: error: inconsistent.constructor.type + public @MustCall({}) Bar() { + // nothing to do + } + + public Bar(int i) throws Exception { + throw new Exception(); + } + + @CreatesMustCallFor + public void overwrite() throws Exception { + f.a(); + f = new Foo(); + throw new Exception(); + } + + @EnsuresCalledMethods( + value = {"this.f"}, + methods = {"a"}) + void a() { + this.f.a(); + } + } + + public void test() throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + SocketAddress addr = new InetSocketAddress("127.0.0.1", 6010); + try { + s.bind(addr); + } catch (Exception e) { + // socket might still be open on this path + return; + } + s.close(); + } + + public void test2() throws Exception { + // :: error: required.method.not.called + Bar b = new Bar(); + try { + b.overwrite(); + } catch (Exception e) { + return; + } + b.a(); + } + + public Bar test3() throws Exception { + // :: error: required.method.not.called + Bar b = new Bar(); + try { + b.overwrite(); + return b; + } catch (Exception e) { + } + return null; + } + + public Bar test4() throws Exception { + try { + // :: error: required.method.not.called + Bar b = new Bar(); + b.overwrite(); + return b; + } catch (Exception e) { + + } + return null; + } + + public Bar test5() throws Exception { + Bar b = new Bar(); + try { + b.overwrite(); + return b; + } catch (Exception e) { + b.a(); + } + return null; + } +} diff --git a/checker/tests/resourceleak/IndexMode.java b/checker/tests/resourceleak/IndexMode.java index b41a251310d..4d2e4434464 100644 --- a/checker/tests/resourceleak/IndexMode.java +++ b/checker/tests/resourceleak/IndexMode.java @@ -20,15 +20,7 @@ public static Object getMode(Map indexOptions) { // This copy of getMode() adds an explicit `@MustCall` annotation to the String. public static Object getMode2(Map indexOptions) { try { - // TODO: a required.method.not.called error should be issued on this line, but currently - // it is not. The reason is an interaction between type variable defaulting, - // local dataflow, and the rules that the RLC uses for choosing a variable's must-call - // obligations: local inference defaults literalOption to @MustCallUnknown (i.e., the - // top MustCall type) even though the RHS expression's type is @MustCall("hashCode"). - // Then, the rule for obligations says that if a variable has the top must-call type, - // use the type's default must-call type instead. For String, this is @MustCall({}), - // so no error is issued. This rule is important to avoid false positives in realistic - // code (such as the first getMode() method in this class). + // :: error: required.method.not.called String literalOption = indexOptions.get("is_literal"); } catch (Exception e) { } diff --git a/checker/tests/resourceleak/NotCheckingDeadCode.java b/checker/tests/resourceleak/NotCheckingDeadCode.java new file mode 100644 index 00000000000..aa2ec719a71 --- /dev/null +++ b/checker/tests/resourceleak/NotCheckingDeadCode.java @@ -0,0 +1,79 @@ +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.checker.nullness.qual.*; + +class NotCheckingDeadCode { + + static class Foo {} + + static Foo makeFoo() { + return new Foo(); + } + + static Foo makeFoo1() { + throw new UnsupportedOperationException(); + } + + static Foo makeFoo2() throws UnsupportedOperationException { + return new Foo(); + } + + static Foo makeFoo3() throws UnsupportedOperationException { + throw new UnsupportedOperationException(); + } + + Foo fooField; + + void test1() { + try { + fooField = makeFoo(); + } catch (Exception e) { + Foo f = null; + fooField = f; + } + } + + void test2() { + try { + fooField = makeFoo1(); + } catch (Exception e) { + Foo f = null; + fooField = f; + } + } + + void test3() { + Foo f = null; + try { + fooField = makeFoo1(); + } catch (Exception e) { + fooField = f; + } + } + + void test4() { + try { + fooField = makeFoo2(); + } catch (Exception e) { + Foo f = null; + fooField = f; + } + } + + void test5() { + try { + fooField = makeFoo3(); + } catch (Exception e) { + Foo f = null; + fooField = f; + } + } + + void test6() { + Foo f = null; + try { + fooField = makeFoo3(); + } catch (Exception e) { + fooField = f; + } + } +} diff --git a/checker/tests/resourceleak/ZookeeperReport3.java b/checker/tests/resourceleak/ZookeeperReport3.java index 43907dcc88f..8484af9fcb8 100644 --- a/checker/tests/resourceleak/ZookeeperReport3.java +++ b/checker/tests/resourceleak/ZookeeperReport3.java @@ -14,6 +14,7 @@ ServerSocket createServerSocket_easy( InetSocketAddress address, boolean portUnification, boolean sslQuorum) { ServerSocket serverSocket; try { + // :: error: required.method.not.called serverSocket = new ServerSocket(); serverSocket.setReuseAddress(true); serverSocket.bind(address); @@ -29,8 +30,10 @@ ServerSocket createServerSocket( ServerSocket serverSocket; try { if (portUnification || sslQuorum) { + // :: error: required.method.not.called serverSocket = new UnifiedServerSocket(portUnification); } else { + // :: error: required.method.not.called serverSocket = new ServerSocket(); } serverSocket.setReuseAddress(true); @@ -48,11 +51,14 @@ private ServerSocket createNewServerSocket( if (portUnification) { System.out.println("Creating TLS-enabled quorum server socket"); + // :: error: required.method.not.called socket = new UnifiedServerSocket(true); } else if (sslQuorum) { System.out.println("Creating TLS-only quorum server socket"); + // :: error: required.method.not.called socket = new UnifiedServerSocket(false); } else { + // :: error: required.method.not.called socket = new ServerSocket(); }