From e8cbd0e3f23cd202eb35f8bc1a28e5949f4969a0 Mon Sep 17 00:00:00 2001 From: Martin Kellogg Date: Fri, 18 Jun 2021 20:49:59 +0000 Subject: [PATCH] Resource Leak Checker (#4687) --- .../checker/mustcall/qual/MustCallAlias.java | 2 +- .../checker/mustcall/qual/NotOwning.java | 2 +- .../checker/mustcall/qual/Owning.java | 5 +- checker/bin-devel/count-suppressions | 7 +- checker/build.gradle | 10 + .../CalledMethodsAnnotatedTypeFactory.java | 6 +- .../calledmethods/CalledMethodsChecker.java | 2 +- .../MustCallAnnotatedTypeFactory.java | 12 +- .../checker/mustcall/MustCallChecker.java | 2 +- .../checker/mustcall/MustCallTransfer.java | 26 +- .../MustCallConsistencyAnalyzer.java | 1715 +++++++++++++++++ .../ResourceLeakAnnotatedTypeFactory.java | 362 ++++ .../resourceleak/ResourceLeakChecker.java | 88 + .../resourceleak/ResourceLeakTransfer.java | 136 ++ .../resourceleak/ResourceLeakVisitor.java | 235 +++ .../checker/resourceleak/messages.properties | 5 + ...> MustCallNoLightweightOwnershipTest.java} | 8 +- .../ResourceLeakNoCreatesMustCallForTest.java | 27 + ...esourceLeakNoLightweightOwnershipTest.java | 27 + .../ResourceLeakNoResourceAliasesTest.java | 27 + .../checker/test/junit/ResourceLeakTest.java | 26 + .../BorrowOnReturn.java | 0 .../NonOwningPolyInteraction.java | 0 .../OwningParams.java | 0 checker/tests/mustcall/SimpleSocketField.java | 21 + .../ConnectingServerSockets.java | 50 + .../ConnectingSockets.java | 57 + .../CreatesMustCallForSimpler.java | 30 + .../SocketContainer.java | 33 + .../ACOwning.java | 59 + .../MustCallAliasExamples.java | 62 + .../MustCallAliasPassthroughLocal.java | 25 + .../ACExceptionalExitPointTest.java | 38 + .../resourceleak/ACMethodInvocationTest.java | 97 + checker/tests/resourceleak/ACOwning.java | 80 + .../resourceleak/ACRegularExitPointTest.java | 400 ++++ checker/tests/resourceleak/ACSocketTest.java | 438 +++++ checker/tests/resourceleak/BindChannel.java | 40 + .../tests/resourceleak/COAnonymousClass.java | 53 + checker/tests/resourceleak/COInSubtype.java | 23 + checker/tests/resourceleak/CheckFields.java | 166 ++ .../tests/resourceleak/CloseableAndMore.java | 28 + .../tests/resourceleak/CommonModuleCrash.java | 11 + .../resourceleak/ConnectingServerSockets.java | 46 + .../tests/resourceleak/ConnectingSockets.java | 58 + .../CreatesMustCallForIndirect.java | 63 + .../CreatesMustCallForInnerClass.java | 26 + .../CreatesMustCallForOverride.java | 31 + .../CreatesMustCallForOverride2.java | 177 ++ .../CreatesMustCallForRepeat.java | 70 + .../CreatesMustCallForSimple.java | 76 + .../CreatesMustCallForSimpler.java | 29 + .../CreatesMustCallForTargets.java | 70 + checker/tests/resourceleak/DoubleIf.java | 54 + checker/tests/resourceleak/Enclosing.java | 21 + checker/tests/resourceleak/EnhancedFor.java | 35 + .../resourceleak/FileDescriptorTest.java | 53 + .../tests/resourceleak/GetChannelOnLocks.java | 39 + checker/tests/resourceleak/HBaseReport1.java | 36 + checker/tests/resourceleak/HdfsReport3.java | 26 + .../tests/resourceleak/InheritanceStream.java | 22 + .../resourceleak/InputOutputStreams.java | 134 ++ checker/tests/resourceleak/IsClosed.java | 28 + .../tests/resourceleak/MCANotOwningField.java | 17 + .../tests/resourceleak/MCAOwningField.java | 24 + checker/tests/resourceleak/MCAWithThis.java | 18 + .../ManualMustCallEmptyOnConstructor.java | 24 + .../resourceleak/MustCallAliasExamples.java | 53 + .../tests/resourceleak/MustCallAliasImpl.java | 23 + .../resourceleak/MustCallAliasImplWrong1.java | 25 + .../resourceleak/MustCallAliasImplWrong2.java | 20 + .../MustCallAliasLayeredStreams.java | 20 + .../MustCallAliasOwningField.java | 29 + .../MustCallAliasPassthrough.java | 12 + .../MustCallAliasPassthroughChain.java | 53 + .../MustCallAliasPassthroughLocal.java | 22 + .../MustCallAliasPassthroughThis.java | 15 + .../MustCallAliasPassthroughWrong1.java | 14 + .../MustCallAliasPassthroughWrong2.java | 21 + .../MustCallAliasPassthroughWrong3.java | 28 + .../MustCallAliasPassthroughWrong4.java | 17 + .../MustCallAliasSocketException.java | 28 + .../tests/resourceleak/MustCallNullStore.java | 20 + .../resourceleak/MustCloseIntoObject.java | 11 + .../NonFinalFieldOnlyOverwrittenIfNull.java | 87 + .../NonFinalFieldOnlyOverwrittenIfNull2.java | 66 + .../tests/resourceleak/OptionalSocket.java | 25 + checker/tests/resourceleak/PaperExample.java | 16 + .../resourceleak/ReassignmentWithMCA.java | 49 + .../RequiresCalledMethodsTest.java | 51 + .../resourceleak/SSLSocketFactoryTest.java | 17 + checker/tests/resourceleak/SelfAssign.java | 36 + .../resourceleak/SimpleSocketExample.java | 17 + .../tests/resourceleak/SocketContainer.java | 30 + .../tests/resourceleak/SocketContainer2.java | 23 + .../tests/resourceleak/SocketContainer3.java | 20 + checker/tests/resourceleak/SocketField.java | 65 + .../resourceleak/SocketNullOverwrite.java | 16 + .../tests/resourceleak/StringFromObject.java | 20 + .../resourceleak/TryWithResourcesSimple.class | Bin 0 -> 1776 bytes .../resourceleak/TryWithResourcesSimple.java | 34 + .../TwoConstructorsCloseable.java | 19 + .../resourceleak/UnconnectedSocketAlias.java | 14 + checker/tests/resourceleak/WrapperStream.java | 10 + .../tests/resourceleak/WrapperStreamPoly.java | 18 + .../ZookeeperByteBufferInputStream.java | 62 + .../tests/resourceleak/ZookeeperReport1.java | 71 + .../tests/resourceleak/ZookeeperReport3.java | 72 + .../ZookeeperReport3WithOptional.java | 52 + .../tests/resourceleak/ZookeeperReport6.java | 15 + .../resourceleak/ZookeeperTernaryCrash.java | 74 + docs/CHANGELOG.md | 4 + docs/manual/advanced-features.tex | 3 + docs/manual/called-methods-checker.tex | 1 + docs/manual/introduction.tex | 3 + docs/manual/manual.tex | 1 + docs/manual/must-call-checker.tex | 4 + docs/manual/resource-leak-checker.tex | 381 ++++ 118 files changed, 7390 insertions(+), 15 deletions(-) create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakTransfer.java create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java create mode 100644 checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties rename checker/src/test/java/org/checkerframework/checker/test/junit/{NoLightweightOwnershipTest.java => MustCallNoLightweightOwnershipTest.java} (65%) create mode 100644 checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoCreatesMustCallForTest.java create mode 100644 checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoLightweightOwnershipTest.java create mode 100644 checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoResourceAliasesTest.java create mode 100644 checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakTest.java rename checker/tests/{nolightweightownership => mustcall-nolightweightownership}/BorrowOnReturn.java (100%) rename checker/tests/{nolightweightownership => mustcall-nolightweightownership}/NonOwningPolyInteraction.java (100%) rename checker/tests/{nolightweightownership => mustcall-nolightweightownership}/OwningParams.java (100%) create mode 100644 checker/tests/mustcall/SimpleSocketField.java create mode 100644 checker/tests/resourceleak-nocreatesmustcallfor/ConnectingServerSockets.java create mode 100644 checker/tests/resourceleak-nocreatesmustcallfor/ConnectingSockets.java create mode 100644 checker/tests/resourceleak-nocreatesmustcallfor/CreatesMustCallForSimpler.java create mode 100644 checker/tests/resourceleak-nocreatesmustcallfor/SocketContainer.java create mode 100644 checker/tests/resourceleak-nolightweightownership/ACOwning.java create mode 100644 checker/tests/resourceleak-noresourcealiases/MustCallAliasExamples.java create mode 100644 checker/tests/resourceleak-noresourcealiases/MustCallAliasPassthroughLocal.java create mode 100644 checker/tests/resourceleak/ACExceptionalExitPointTest.java create mode 100644 checker/tests/resourceleak/ACMethodInvocationTest.java create mode 100644 checker/tests/resourceleak/ACOwning.java create mode 100644 checker/tests/resourceleak/ACRegularExitPointTest.java create mode 100644 checker/tests/resourceleak/ACSocketTest.java create mode 100644 checker/tests/resourceleak/BindChannel.java create mode 100644 checker/tests/resourceleak/COAnonymousClass.java create mode 100644 checker/tests/resourceleak/COInSubtype.java create mode 100644 checker/tests/resourceleak/CheckFields.java create mode 100644 checker/tests/resourceleak/CloseableAndMore.java create mode 100644 checker/tests/resourceleak/CommonModuleCrash.java create mode 100644 checker/tests/resourceleak/ConnectingServerSockets.java create mode 100644 checker/tests/resourceleak/ConnectingSockets.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForIndirect.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForInnerClass.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForOverride.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForOverride2.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForRepeat.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForSimple.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForSimpler.java create mode 100644 checker/tests/resourceleak/CreatesMustCallForTargets.java create mode 100644 checker/tests/resourceleak/DoubleIf.java create mode 100644 checker/tests/resourceleak/Enclosing.java create mode 100644 checker/tests/resourceleak/EnhancedFor.java create mode 100644 checker/tests/resourceleak/FileDescriptorTest.java create mode 100644 checker/tests/resourceleak/GetChannelOnLocks.java create mode 100644 checker/tests/resourceleak/HBaseReport1.java create mode 100644 checker/tests/resourceleak/HdfsReport3.java create mode 100644 checker/tests/resourceleak/InheritanceStream.java create mode 100644 checker/tests/resourceleak/InputOutputStreams.java create mode 100644 checker/tests/resourceleak/IsClosed.java create mode 100644 checker/tests/resourceleak/MCANotOwningField.java create mode 100644 checker/tests/resourceleak/MCAOwningField.java create mode 100644 checker/tests/resourceleak/MCAWithThis.java create mode 100644 checker/tests/resourceleak/ManualMustCallEmptyOnConstructor.java create mode 100644 checker/tests/resourceleak/MustCallAliasExamples.java create mode 100644 checker/tests/resourceleak/MustCallAliasImpl.java create mode 100644 checker/tests/resourceleak/MustCallAliasImplWrong1.java create mode 100644 checker/tests/resourceleak/MustCallAliasImplWrong2.java create mode 100644 checker/tests/resourceleak/MustCallAliasLayeredStreams.java create mode 100644 checker/tests/resourceleak/MustCallAliasOwningField.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthrough.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughChain.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughLocal.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughThis.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughWrong1.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughWrong2.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughWrong3.java create mode 100644 checker/tests/resourceleak/MustCallAliasPassthroughWrong4.java create mode 100644 checker/tests/resourceleak/MustCallAliasSocketException.java create mode 100644 checker/tests/resourceleak/MustCallNullStore.java create mode 100644 checker/tests/resourceleak/MustCloseIntoObject.java create mode 100644 checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull.java create mode 100644 checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull2.java create mode 100644 checker/tests/resourceleak/OptionalSocket.java create mode 100644 checker/tests/resourceleak/PaperExample.java create mode 100644 checker/tests/resourceleak/ReassignmentWithMCA.java create mode 100644 checker/tests/resourceleak/RequiresCalledMethodsTest.java create mode 100644 checker/tests/resourceleak/SSLSocketFactoryTest.java create mode 100644 checker/tests/resourceleak/SelfAssign.java create mode 100644 checker/tests/resourceleak/SimpleSocketExample.java create mode 100644 checker/tests/resourceleak/SocketContainer.java create mode 100644 checker/tests/resourceleak/SocketContainer2.java create mode 100644 checker/tests/resourceleak/SocketContainer3.java create mode 100644 checker/tests/resourceleak/SocketField.java create mode 100644 checker/tests/resourceleak/SocketNullOverwrite.java create mode 100644 checker/tests/resourceleak/StringFromObject.java create mode 100644 checker/tests/resourceleak/TryWithResourcesSimple.class create mode 100644 checker/tests/resourceleak/TryWithResourcesSimple.java create mode 100644 checker/tests/resourceleak/TwoConstructorsCloseable.java create mode 100644 checker/tests/resourceleak/UnconnectedSocketAlias.java create mode 100644 checker/tests/resourceleak/WrapperStream.java create mode 100644 checker/tests/resourceleak/WrapperStreamPoly.java create mode 100644 checker/tests/resourceleak/ZookeeperByteBufferInputStream.java create mode 100644 checker/tests/resourceleak/ZookeeperReport1.java create mode 100644 checker/tests/resourceleak/ZookeeperReport3.java create mode 100644 checker/tests/resourceleak/ZookeeperReport3WithOptional.java create mode 100644 checker/tests/resourceleak/ZookeeperReport6.java create mode 100644 checker/tests/resourceleak/ZookeeperTernaryCrash.java create mode 100644 docs/manual/resource-leak-checker.tex diff --git a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/MustCallAlias.java b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/MustCallAlias.java index 26876ad0929..0143085d564 100644 --- a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/MustCallAlias.java +++ b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/MustCallAlias.java @@ -49,7 +49,7 @@ * When the -AnoResourceAliases command-line argument is passed to the checker, this annotation is * treated identically to {@link PolyMustCall}. * - * @checker_framework.manual #must-call-checker Must Call Checker + * @checker_framework.manual #resource-leak-checker Resource Leak Checker * @checker_framework.manual #qualifier-polymorphism Qualifier polymorphism */ @Documented diff --git a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/NotOwning.java b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/NotOwning.java index 4090ac8ca7f..66ecd11a101 100644 --- a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/NotOwning.java +++ b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/NotOwning.java @@ -15,7 +15,7 @@ *

When the -AnoLightweightOwnership command-line argument is passed to the checker, this * annotation and {@link Owning} are ignored. * - * @checker_framework.manual #must-call-checker Must Call Checker + * @checker_framework.manual #resource-leak-checker Resource Leak Checker */ @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD}) diff --git a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/Owning.java b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/Owning.java index 429a76e311d..f6f0c75471d 100644 --- a/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/Owning.java +++ b/checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/Owning.java @@ -7,7 +7,8 @@ /** * Annotation indicating that ownership should be transferred to the annotated parameter, field, or - * (when written on a method) return type, for the purposes of Must Call checking. + * (when written on a method) return type, for the purposes of Must Call checking. Static fields + * cannot be owning. * *

Method return types are treated as if they have this annotation by default unless their method * is annotated as {@link NotOwning}. @@ -15,7 +16,7 @@ *

When the -AnoLightweightOwnership command-line argument is passed to the checker, this * annotation and {@link NotOwning} are ignored. * - * @checker_framework.manual #must-call-checker Must Call Checker + * @checker_framework.manual #resource-leak-checker Resource Leak Checker */ @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD}) diff --git a/checker/bin-devel/count-suppressions b/checker/bin-devel/count-suppressions index 9a774da3670..98b320f5923 100755 --- a/checker/bin-devel/count-suppressions +++ b/checker/bin-devel/count-suppressions @@ -54,6 +54,9 @@ fi if [ "$regex" = "index" ]; then regex="\(index\|lessthan\|lowerbound\|samelen\|searchindex\|substringindex\|upperbound\)" fi +if [ "$regex" = "resourceleak" ]; then + regex="\(mustcall\|objectconstruction\|resourceleak\)" +fi # Diagnostics # echo "regex=${regex}" @@ -67,10 +70,10 @@ countedreasons=$(mktemp /tmp/count-suppressions."$(date +%Y%m%d-%H%M%S)"-XXX) # include "@SuppressWarnings" because it might appear on the previous line. # * @AssumeAssertion(checkername) # This grep command captures a few stray lines; users should ignore them. -# This grep command assumes that tests are not annotated, and it hard-codes ignoring "annotated-jdk", "jdk", and "true positive". +# This grep command assumes that tests are not annotated, and it hard-codes ignoring "annotated-jdk", "jdk", "true positive", "// TP" (as an alias for "true positive"), and "count-suppressions-ignore". ${GREP} -n --recursive --include='*.java' "\"${regex}[:\"]\(.*[^;]\)\?\(\$\|//\)\|@AssumeAssertion(${regex})" \ | grep -v "@AnnotatedFor" | grep -v "/tests/" \ - | grep -v "/annotated-jdk/" | grep -v "/jdk/" | grep -v "^jdk/" | grep -v "true positive" > "${greplines}" + | grep -v "/annotated-jdk/" | grep -v "/jdk/" | grep -v "^jdk/" | grep -v "true positive" | grep -v "// TP" | grep -v "count-suppressions-ignore" > "${greplines}" total=$(wc -l < "${greplines}") ## Don't output a total, to avoid people using this approximate count. diff --git a/checker/build.gradle b/checker/build.gradle index 8d3acaefd25..143b730003f 100644 --- a/checker/build.gradle +++ b/checker/build.gradle @@ -40,6 +40,16 @@ dependencies { implementation 'org.plumelib:reflection-util:1.0.3' implementation 'org.plumelib:plume-util:1.5.5' + // TODO: it's a bug that annotatedlib:guava requires the error_prone_annotations dependency. + // Update the next two version numbers in tandem. Get the Error Prone version from the "compile + // dependencies" section of https://mvnrepository.com/artifact/com.google.guava/guava/28.2-jre . + // (It isn't at https://mvnrepository.com/artifact/org.checkerframework.annotatedlib/guava/28.2-jre, which is the bug.) + implementation 'com.google.errorprone:error_prone_annotations:2.7.1' + implementation ('org.checkerframework.annotatedlib:guava:30.1.1-jre') { + // So long as Guava only uses annotations from checker-qual, excluding it should not cause problems. + exclude group: 'org.checkerframework' + } + // Dependencies added to "shadow" appear as dependencies in Maven Central. shadow project(':checker-qual') shadow project(':checker-util') diff --git a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnnotatedTypeFactory.java index 462a8da3e8e..9c87e982ea0 100644 --- a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnnotatedTypeFactory.java @@ -91,7 +91,11 @@ public CalledMethodsAnnotatedTypeFactory(BaseTypeChecker checker) { // therefore treat it as top. addAliasedTypeAnnotation( "org.checkerframework.checker.builder.qual.NotCalledMethods", this.top); - this.postInit(); + + // Don't call postInit() for subclasses. + if (this.getClass() == CalledMethodsAnnotatedTypeFactory.class) { + this.postInit(); + } } /** diff --git a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsChecker.java b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsChecker.java index dfba90c44e3..bc66ba1edc5 100644 --- a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsChecker.java +++ b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsChecker.java @@ -60,7 +60,7 @@ public class CalledMethodsChecker extends AccumulationChecker { /** * The number of calls to build frameworks supported by this invocation. Incremented only if the - * {@link #COUNT_FRAMEWORK_BUILD_CALLS} option was supplied. + * {@link #COUNT_FRAMEWORK_BUILD_CALLS} command-line option was supplied. */ int numBuildCalls = 0; 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 729cd62717b..fdf3f01f857 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java @@ -184,7 +184,7 @@ protected TypeAnnotator createTypeAnnotator() { * @param elt an element; may be null, in which case this method always returns false * @return true iff the given element represents a resource variable */ - private boolean isResourceVariable(@Nullable Element elt) { + /* package-private*/ boolean isResourceVariable(@Nullable Element elt) { return elt != null && elt.getKind() == ElementKind.RESOURCE_VARIABLE; } @@ -248,6 +248,16 @@ protected DefaultQualifierForUseTypeAnnotator createDefaultForUseTypeAnnotator() return new MustCallDefaultQualifierForUseTypeAnnotator(); } + /** + * Returns the {@link MustCall#value} element. For use with {@link + * AnnotationUtils#getElementValueArray}. + * + * @return the {@link MustCall#value} element + */ + public ExecutableElement getMustCallValueElement() { + return mustCallValueElement; + } + /** Support @InheritableMustCall meaning @MustCall on all subtype elements. */ class MustCallDefaultQualifierForUseTypeAnnotator extends DefaultQualifierForUseTypeAnnotator { 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 6bb855911c3..4c353318bc7 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java @@ -7,7 +7,7 @@ /** * This typechecker ensures that {@code @}{@link MustCall} annotations are consistent with one - * another. The Object Construction Checker verifies that the given methods are actually called. + * another. The Resource Leak Checker verifies that the given methods are actually called. */ @StubFiles({ "JavaEE.astub", 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 c290dbc55de..c094008187d 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java @@ -10,10 +10,12 @@ import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.type.TypeMirror; +import org.checkerframework.checker.mustcall.qual.MustCall; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.dataflow.analysis.TransferInput; import org.checkerframework.dataflow.analysis.TransferResult; +import org.checkerframework.dataflow.cfg.node.AssignmentNode; import org.checkerframework.dataflow.cfg.node.LocalVariableNode; import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.dataflow.cfg.node.Node; @@ -94,6 +96,28 @@ private AnnotationMirror getDefaultStringType(StringConversionNode n) { return this.defaultStringType; } + @Override + public TransferResult visitAssignment( + AssignmentNode n, TransferInput in) { + TransferResult result = super.visitAssignment(n, in); + // Remove "close" from the type in the store for resource variables. + // The Resource Leak Checker relies on this code to avoid checking that + // resource variables are closed. + if (atypeFactory.isResourceVariable(TreeUtils.elementFromTree(n.getTarget().getTree()))) { + CFStore store = result.getRegularStore(); + JavaExpression expr = JavaExpression.fromNode(n.getTarget()); + CFValue value = store.getValue(expr); + AnnotationMirror withClose = + atypeFactory.getAnnotationByClass(value.getAnnotations(), MustCall.class); + if (withClose == null) { + return result; + } + AnnotationMirror withoutClose = atypeFactory.withoutClose(withClose); + insertIntoStores(result, expr, withoutClose); + } + return result; + } + @Override public TransferResult visitMethodInvocation( MethodInvocationNode n, TransferInput in) { @@ -160,7 +184,7 @@ public TransferResult visitTernaryExpression( /** * This method either creates or looks up the temp var t for node, and then updates the store to - * give t the same type as node. + * give t the same type as {@code node}. * * @param node the node to be assigned to a temporary variable * @param result the transfer result containing the store to be modified diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java new file mode 100644 index 00000000000..1fa6deb7e1c --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -0,0 +1,1715 @@ +package org.checkerframework.checker.resourceleak; + +import com.google.common.base.Preconditions; +import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Type; +import java.io.UnsupportedEncodingException; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Deque; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Name; +import javax.lang.model.element.VariableElement; +import javax.lang.model.type.TypeKind; +import javax.lang.model.type.TypeMirror; +import org.checkerframework.checker.calledmethods.qual.CalledMethods; +import org.checkerframework.checker.mustcall.CreatesMustCallForElementSupplier; +import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory; +import org.checkerframework.checker.mustcall.MustCallChecker; +import org.checkerframework.checker.mustcall.qual.MustCall; +import org.checkerframework.checker.mustcall.qual.NotOwning; +import org.checkerframework.checker.mustcall.qual.Owning; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.checkerframework.checker.signature.qual.FullyQualifiedName; +import org.checkerframework.dataflow.cfg.ControlFlowGraph; +import org.checkerframework.dataflow.cfg.UnderlyingAST; +import org.checkerframework.dataflow.cfg.block.Block; +import org.checkerframework.dataflow.cfg.block.ExceptionBlock; +import org.checkerframework.dataflow.cfg.block.SingleSuccessorBlock; +import org.checkerframework.dataflow.cfg.block.SpecialBlockImpl; +import org.checkerframework.dataflow.cfg.node.AssignmentNode; +import org.checkerframework.dataflow.cfg.node.FieldAccessNode; +import org.checkerframework.dataflow.cfg.node.LocalVariableNode; +import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.dataflow.cfg.node.Node; +import org.checkerframework.dataflow.cfg.node.NullLiteralNode; +import org.checkerframework.dataflow.cfg.node.ObjectCreationNode; +import org.checkerframework.dataflow.cfg.node.ReturnNode; +import org.checkerframework.dataflow.cfg.node.TernaryExpressionNode; +import org.checkerframework.dataflow.cfg.node.ThisNode; +import org.checkerframework.dataflow.cfg.node.TypeCastNode; +import org.checkerframework.dataflow.expression.FieldAccess; +import org.checkerframework.dataflow.expression.JavaExpression; +import org.checkerframework.dataflow.expression.LocalVariable; +import org.checkerframework.dataflow.expression.ThisReference; +import org.checkerframework.framework.flow.CFAnalysis; +import org.checkerframework.framework.flow.CFStore; +import org.checkerframework.framework.flow.CFValue; +import org.checkerframework.framework.util.JavaExpressionParseUtil.JavaExpressionParseException; +import org.checkerframework.framework.util.StringToJavaExpression; +import org.checkerframework.javacutil.AnnotationUtils; +import org.checkerframework.javacutil.BugInCF; +import org.checkerframework.javacutil.ElementUtils; +import org.checkerframework.javacutil.Pair; +import org.checkerframework.javacutil.TreePathUtil; +import org.checkerframework.javacutil.TreeUtils; +import org.checkerframework.javacutil.TypesUtils; +import org.plumelib.util.StringsPlume; + +/** + * An analyzer that checks consistency of {@code @MustCall} and {@code @CalledMethods} types, + * thereby detecting resource leaks. For any expression e, the analyzer ensures that when + * e goes out of scope, there exists a resource alias r of e (which might + * be e itself) such that MustCall(r) is contained in CalledMethods(r). For any e + * for which this property does not hold, the analyzer reports a {@code + * "required.method.not.called"} error, indicating a possible resource leak. + * + *

Mechanically, the analysis tracks dataflow facts about the obligations of sets of + * resource-aliases that refer to the same resource, and checks their must-call and called-methods + * types when the last reference to those sets goes out of scope. That is, this class implements a + * lightweight alias analysis that tracks must-alias sets for resources. + * + *

Throughout this class, variables named "obligation" or "obligations" are dataflow facts of + * type {@code ImmutableSet}, each representing a set of resource aliases for some + * value with a non-empty {@code @MustCall} obligation. These obligations can be resolved either via + * ownership transfer (e.g. by being assigned into an owning field) or via their must-call + * obligations being contained in their called-methods type when the last reference in a set goes + * out of scope. + * + *

The algorithm here adds, modifies, or removes obligations from those it is tracking when + * certain code patterns are encountered. (Because they are immutable, obligations are modified by + * removing them and creating a new, related obligation.) Here are non-exhaustive examples: + * + *

+ * + *

Throughout, this class uses the temporary-variable facilities provided by the Must Call and + * Resource Leak type factories to both emulate a three-address-form IR, simplifying some analysis + * logic, and to permit expressions to have their types refined in their respective checkers' + * stores. These temporary variables can be members of resource-alias sets. Without temporary + * variables, the checker wouldn't be able to verify code such as {@code new Socket(host, + * port).close()}, which would cause false positives. Temporaries are created for {@code new} + * expressions, method calls (for the return value), and ternary expressions. Other types of + * expressions may also be supported in the future. + */ +/* package-private */ +class MustCallConsistencyAnalyzer { + + /** {@code @MustCall} errors reported thus far, to avoid duplicate reports. */ + private final Set reportedMustCallErrors = new HashSet<>(); + + /** + * The type factory for the Resource Leak Checker, which is used to get called methods types and + * to access the Must Call Checker. + */ + private final ResourceLeakAnnotatedTypeFactory typeFactory; + + /** The Resource Leak Checker, used to issue errors. */ + private final ResourceLeakChecker checker; + + /** The analysis from the Resource Leak Checker, used to get input stores based on CFG blocks. */ + private final CFAnalysis analysis; + + /** + * Creates a consistency analyzer. Typically, the type factory's postAnalyze method would + * instantiate a new consistency analyzer using this constructor and then call {@link + * #analyze(ControlFlowGraph)}. + * + * @param typeFactory the type factory + * @param analysis the analysis from the type factory. Usually this would have protected access, + * so this constructor cannot get it directly. + */ + /* package-private */ + MustCallConsistencyAnalyzer(ResourceLeakAnnotatedTypeFactory typeFactory, CFAnalysis analysis) { + this.typeFactory = typeFactory; + this.checker = (ResourceLeakChecker) typeFactory.getChecker(); + this.analysis = analysis; + } + + /** + * The main function of the consistency dataflow analysis. The analysis tracks dataflow facts + * ("obligations") of type {@code ImmutableSet}, each representing a set of + * resource aliases for some value with a non-empty {@code @MustCall} obligation. (It is not + * necessary to track expressions with empty {@code @MustCall} obligations, because they are + * trivially fulfilled.) + * + * @param cfg the control flow graph of the method to check + */ + // TODO: This analysis is currently implemented directly using a worklist; in the future, it + // should be rewritten to use the dataflow framework of the Checker Framework. + /* package-private */ + void analyze(ControlFlowGraph cfg) { + // The `visited` set contains everything that has been added to the worklist, even if it has not + // yet been removed and analyzed. + Set visited = new LinkedHashSet<>(); + Deque worklist = new ArrayDeque<>(); + + // Add any owning parameters to the initial set of variables to track. + BlockWithObligations entry = + new BlockWithObligations(cfg.getEntryBlock(), computeOwningParameters(cfg)); + worklist.add(entry); + visited.add(entry); + + while (!worklist.isEmpty()) { + BlockWithObligations current = worklist.remove(); + List nodes = current.block.getNodes(); + // A *mutable* set that eventually holds the set of obligations to be propagated to successor + // blocks. The set is initialized to the current obligations and updated by the methods + // invoked in the for loop below. + Set> obligations = new LinkedHashSet<>(current.obligations); + + for (Node node : nodes) { + if (node instanceof AssignmentNode) { + handleAssignment((AssignmentNode) node, obligations); + } else if (node instanceof ReturnNode) { + handleReturn((ReturnNode) node, cfg, obligations); + } else if (node instanceof MethodInvocationNode || node instanceof ObjectCreationNode) { + handleInvocation(obligations, node); + } + // All other types of nodes are ignored. This is safe, because other kinds of + // nodes cannot create or modify the resource-alias sets that the algorithm is tracking. + } + + handleSuccessorBlocks(visited, worklist, obligations, current.block); + } + } + + /** + * Update a set of obligations to account for a method or constructor invocation. + * + * @param obligations the obligations to update + * @param node the method or constructor invocation + */ + private void handleInvocation(Set> obligations, Node node) { + transferOwnershipToParameters(obligations, node); + if (node instanceof MethodInvocationNode + && typeFactory.canCreateObligations() + && typeFactory.hasCreatesMustCallFor((MethodInvocationNode) node)) { + checkCreatesMustCallForInvocation(obligations, (MethodInvocationNode) node); + // Count calls to @CreatesMustCallFor methods as creating new resources. Doing so could + // result in slightly over-counting, because @CreatesMustCallFor doesn't guarantee that a + // new resource is created: it just means that a new resource might have been created. + incrementNumMustCall(node); + } + + if (!shouldTrackInvocationResult(obligations, node)) { + return; + } + + if (typeFactory.hasDeclaredMustCall(node.getTree())) { + // The incrementNumMustCall call above increments the count for the target of the + // @CreatesMustCallFor annotation. By contrast, this call increments the count for the return + // value of the method (which can't be the target of the annotation, because our syntax + // doesn't support that). + incrementNumMustCall(node); + } + trackInvocationResult(obligations, node); + } + + /** + * If node is an invocation of a this or super constructor that has a MustCallAlias return type + * and a MustCallAlias parameter, check if any variable in the current set of obligations is being + * passed to the other constructor. If so, remove it from the obligations. + * + * @param obligations current obligations + * @param node a super or this constructor invocation + */ + private void handleThisOrSuperConstructorMustCallAlias( + Set> obligations, Node node) { + Node mustCallAliasArgument = getMustCallAliasArgumentNode(node); + // If the MustCallAlias argument is also in the set of obligations, then remove it -- its + // obligation has been fulfilled by being passed on to the MustCallAlias constructor (because we + // must be in a constructor body if we've encountered a this/super constructor call). + if (mustCallAliasArgument instanceof LocalVariableNode) { + removeObligationContainingVar(obligations, (LocalVariableNode) mustCallAliasArgument); + } + } + + /** + * Checks that an invocation of a CreatesMustCallFor method is valid. Such an invocation is valid + * if one of the following conditions is true: 1) the target is an owning pointer, 2) the target + * already has a tracked obligation, or 3) the method in which the invocation occurs also has + * an @CreatesMustCallFor annotation, with the same target. + * + *

If none of the above are true, this method issues a reset.not.owning error. + * + *

For soundness, this method also guarantees that if the target has a tracked obligation, any + * tracked aliases will be removed (lest the analysis conclude that it is already closed because + * one of these aliases was closed before the method was invoked). Aliases created after the + * CreatesMustCallFor method is invoked are still permitted. + * + * @param obligations the currently-tracked obligations. This value is side-effected if it + * contains the target of the reset method. + * @param node a method invocation node, invoking a method with a CreatesMustCallFor annotation + */ + private void checkCreatesMustCallForInvocation( + Set> obligations, MethodInvocationNode node) { + + TreePath currentPath = typeFactory.getPath(node.getTree()); + List targetExprs = + CreatesMustCallForElementSupplier.getCreatesMustCallForExpressions( + node, typeFactory, typeFactory); + Set missing = new HashSet<>(); + for (JavaExpression target : targetExprs) { + boolean validInvocation = false; + if (target instanceof FieldAccess) { + Element elt = ((FieldAccess) target).getField(); + if (!checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) + && typeFactory.getDeclAnnotation(elt, Owning.class) != null) { + // The target is an Owning field. This satisfies case 1. + validInvocation = true; + } + } else if (target instanceof LocalVariable) { + Element elt = ((LocalVariable) target).getElement(); + if (!checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) + && typeFactory.getDeclAnnotation(elt, Owning.class) != null) { + // The target is an Owning param. This satisfies case 1. + validInvocation = true; + } else { + ImmutableSet toRemoveSet = null; + ImmutableSet toAddSet = null; + for (ImmutableSet resourceAliasSet : obligations) { + for (LocalVarWithTree alias : resourceAliasSet) { + if (target.equals(alias.localVar)) { + // This satisfies case 2 above. Remove all its aliases, then return below. + if (toRemoveSet != null) { + throw new BugInCF( + "tried to remove multiple sets containing a reset target at once"); + } + toRemoveSet = resourceAliasSet; + toAddSet = ImmutableSet.of(alias); + } + } + } + + if (toRemoveSet != null) { + obligations.remove(toRemoveSet); + obligations.add(toAddSet); + // This satisfies case 2. + validInvocation = true; + } + } + } + + if (!validInvocation) { + // TODO: Getting this every time is inefficient if a method has many @CreatesMustCallFor + // annotations, but that should be a rare path. + MethodTree enclosingMethodTree = TreePathUtil.enclosingMethod(currentPath); + if (enclosingMethodTree != null) { + ExecutableElement enclosingMethodElt = + TreeUtils.elementFromDeclaration(enclosingMethodTree); + MustCallAnnotatedTypeFactory mcAtf = + typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + List enclosingCmcfValues = + ResourceLeakVisitor.getCreatesMustCallForValues( + enclosingMethodElt, mcAtf, typeFactory); + if (!enclosingCmcfValues.isEmpty()) { + for (String enclosingCmcfValue : enclosingCmcfValues) { + JavaExpression enclosingTarget; + try { + enclosingTarget = + StringToJavaExpression.atMethodBody( + enclosingCmcfValue, enclosingMethodTree, checker); + } catch (JavaExpressionParseException e) { + // Do not issue an error here, because it would be a duplicate. + // The error will be issued by the Transfer class of the checker, + // via the CreatesMustCallForElementSupplier interface. + enclosingTarget = null; + } + + if (representSame(target, enclosingTarget)) { + // This satisifies case 3. + validInvocation = true; + } + } + } + } + } + if (!validInvocation) { + missing.add(target); + } + } + + if (missing.isEmpty()) { + // all targets were valid + return; + } + + String missingStrs = StringsPlume.join(", ", missing); + checker.reportError(node.getTree(), "reset.not.owning", missingStrs); + } + + /** + * Checks whether the two JavaExpressions are the same. This is identical to calling equals() on + * one of them, with two exceptions: the second expression can be null, and "this" references are + * compared using their underlying type. (ThisReference#equals always returns true, which is + * probably a bug and isn't accurate in the case of nested classes.) + * + * @param target a JavaExpression + * @param enclosingTarget another, possibly null, JavaExpression + * @return true iff they represent the same program element + */ + private boolean representSame(JavaExpression target, @Nullable JavaExpression enclosingTarget) { + if (enclosingTarget == null) { + return false; + } + if (enclosingTarget instanceof ThisReference && target instanceof ThisReference) { + return enclosingTarget.getType().toString().equals(target.getType().toString()); + } else { + return enclosingTarget.equals(target); + } + } + + /** + * Given a node representing a method or constructor call, checks that if the result of the call + * has a non-empty {@code @MustCall} type, then the result is pseudo-assigned to some location + * that can take ownership of the result. Searches for the set of same resources in {@code + * obligations} and adds the new LocalVarWithTree to it if one exists. Otherwise creates a new + * set. + * + * @param obligations the currently-tracked obligations. This is always side-effected: an + * obligation is either modified (by removing it from the obligation set and adding a new one) + * to include a new resource alias (the result of the invocation being tracked) or a new + * resource alias set (i.e. obligation) is created and added. + * @param node the node whose result is to be tracked + */ + private void trackInvocationResult(Set> obligations, Node node) { + Tree tree = node.getTree(); + // We need to track the result of the call iff there is a temporary variable for the call node + // (because we only create temporaries for expressions that could actually have must-call + // values). + LocalVariableNode tmpVar = typeFactory.getTempVarForNode(node); + if (tmpVar == null) { + return; + } + + // `mustCallAlias` is the argument passed in the MustCallAlias position, if any exists, + // otherwise null. + Node mustCallAlias = getMustCallAliasArgumentNode(node); + // If mustCallAlias is null and call returns @This, set mustCallAlias to the receiver. + if (mustCallAlias == null + && node instanceof MethodInvocationNode + && typeFactory.returnsThis((MethodInvocationTree) tree)) { + mustCallAlias = + removeCastsAndGetTmpVarIfPresent(((MethodInvocationNode) node).getTarget().getReceiver()); + } + + LocalVarWithTree tmpVarWithTree = new LocalVarWithTree(new LocalVariable(tmpVar), tree); + if (mustCallAlias instanceof FieldAccessNode) { + // We do not track the call result if the MustCallAlias argument is a field. Handling of + // @Owning fields is a completely separate check, and we never need to track an alias of + // non-@Owning fields. + } else if (mustCallAlias instanceof LocalVariableNode) { + ImmutableSet resourceAliasSetContainingMustCallAlias = + getResourceAliasSetForVar(obligations, (LocalVariableNode) mustCallAlias); + // If mustCallAlias is a local variable already being tracked, add tmpVarWithTree + // to the set containing mustCallAlias. + if (resourceAliasSetContainingMustCallAlias != null) { + ImmutableSet newResourceAliasSet = + FluentIterable.from(resourceAliasSetContainingMustCallAlias) + .append(tmpVarWithTree) + .toSet(); + obligations.remove(resourceAliasSetContainingMustCallAlias); + obligations.add(newResourceAliasSet); + } + } else { + // If mustCallAlias is neither a field nor a local already in the set of obligations, + // add it to a new set. + obligations.add(ImmutableSet.of(tmpVarWithTree)); + } + } + + /** + * Checks for cases where we do not need to track the result of a method call (that is, cases + * where the obligations are already satisfied in some other way or where there cannot possibly be + * obligations because of the structure of the code). Specifically, an invocation result does not + * need to be tracked if the method invocation is a call to a constructor `this()` or `super()`, + * if the method's return type is annotated with MustCallAlias and the argument in the + * corresponding position is an owning field, or if the method's return type is non-owning, which + * can either be because the method has no return type or because it is annotated with {@link + * NotOwning}. + * + *

This method can also side-effect obligations, if node is a super or this constructor call + * with MustCallAlias annotations, by removing that obligation. + * + * @param obligations the current set of obligations + * @param node the invocation node to check + * @return true iff the result of node should be tracked in obligations + */ + private boolean shouldTrackInvocationResult( + Set> obligations, Node node) { + Tree callTree = node.getTree(); + if (callTree.getKind() == Tree.Kind.METHOD_INVOCATION) { + MethodInvocationTree methodInvokeTree = (MethodInvocationTree) callTree; + + if (TreeUtils.isSuperConstructorCall(methodInvokeTree) + || TreeUtils.isThisConstructorCall(methodInvokeTree)) { + handleThisOrSuperConstructorMustCallAlias(obligations, node); + return false; + } + return !returnTypeIsMustCallAliasWithUntrackable((MethodInvocationNode) node) + && !hasNotOwningReturnType((MethodInvocationNode) node); + } + return true; + } + + /** + * Returns true if this node represents a method invocation of a must-call-alias method, where the + * other must call alias is untrackable: an owning field or a pointer that is guaranteed to be + * non-owning, such as "`this`" or a non-owning field. Owning fields are handled by the rest of + * the checker, not by this algorithm, so they are "untrackable". Non-owning fields and this nodes + * are guaranteed to be non-owning, and therefore do not need to be tracked, either. + * + * @param node a method invocation node + * @return true if this is the invocation of a method whose return type is MCA with an owning + * field or a definitely non-owning pointer + */ + private boolean returnTypeIsMustCallAliasWithUntrackable(MethodInvocationNode node) { + Node mustCallAliasArgument = getMustCallAliasArgumentNode(node); + return mustCallAliasArgument instanceof FieldAccessNode + || mustCallAliasArgument instanceof ThisNode; + } + + /** + * Checks if {@code node} is either directly enclosed by a {@link TypeCastNode} or is the then or + * else operand of a {@link TernaryExpressionNode}, by looking at the successor block in the CFG. + * These are all the cases where the enclosing operator is a "no-op" that evaluates to the same + * value as {@code node} (in the appropriate case for a ternary expression). This method is only + * used within {@link #handleSuccessorBlocks(Set, Deque, Set, Block)} to ensure obligations are + * propagated to cast / ternary nodes properly. It relies on the assumption that a {@link + * TypeCastNode} or {@link TernaryExpressionNode} will only appear in a CFG as the first node in a + * block. + * + * @param node the CFG node + * @return {@code true} if {@code node} is in a {@link SingleSuccessorBlock} {@code b}, the first + * {@link Node} in {@code b}'s successor block is a {@link TypeCastNode} or a {@link + * TernaryExpressionNode}, and {@code node} is an operand of the successor node; {@code false} + * otherwise + */ + private boolean inCastOrTernary(Node node) { + if (!(node.getBlock() instanceof SingleSuccessorBlock)) { + return false; + } + Block successorBlock = ((SingleSuccessorBlock) node.getBlock()).getSuccessor(); + if (successorBlock != null) { + List succNodes = successorBlock.getNodes(); + if (succNodes.size() > 0) { + Node succNode = succNodes.get(0); + if (succNode instanceof TypeCastNode) { + return ((TypeCastNode) succNode).getOperand().equals(node); + } else if (succNode instanceof TernaryExpressionNode) { + TernaryExpressionNode ternaryExpressionNode = (TernaryExpressionNode) succNode; + return ternaryExpressionNode.getThenOperand().equals(node) + || ternaryExpressionNode.getElseOperand().equals(node); + } + } + } + return false; + } + + /** + * Transfers ownership of locals to {@code @Owning} parameters at a method or constructor call. + * + * @param obligations the current set of obligations, which is side-effected to remove obligations + * for locals that are passed as owning parameters to the method or constructor + * @param node a method or constructor invocation node + */ + private void transferOwnershipToParameters( + Set> obligations, Node node) { + + if (checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP)) { + // Never transfer ownership to parameters, matching ECJ's default. + return; + } + + List arguments = getArgumentsOfInvocation(node); + List parameters = getParametersOfInvocation(node); + + if (arguments.size() != parameters.size()) { + // This could happen, e.g., with varargs, or with strange cases like generated Enum + // constructors. In the varargs case (i.e. if the varargs parameter is owning), + // only the first of the varargs arguments will actually get transferred: the second + // and later varargs arguments will continue to be tracked at the call-site. + // For now, just skip this case - the worst that will happen is a false positive in + // cases like the varargs one described above. + // TODO allow for ownership transfer here if needed in future + return; + } + for (int i = 0; i < arguments.size(); i++) { + Node n = removeCastsAndGetTmpVarIfPresent(arguments.get(i)); + if (n instanceof LocalVariableNode) { + LocalVariableNode local = (LocalVariableNode) n; + if (varTrackedInObligations(local, obligations)) { + + // check if parameter has an @Owning annotation + VariableElement parameter = parameters.get(i); + Set annotationMirrors = typeFactory.getDeclAnnotations(parameter); + + for (AnnotationMirror anno : annotationMirrors) { + if (AnnotationUtils.areSameByName( + anno, "org.checkerframework.checker.mustcall.qual.Owning")) { + // transfer ownership! + obligations.remove(getResourceAliasSetForVar(obligations, local)); + break; + } + } + } + } + } + } + + /** + * If the return type of the enclosing method is {@code @Owning}, transfer ownership of the return + * value and treat its obligations as satisfied by removing it from {@code obligations}. + * + * @param node a return node + * @param cfg the CFG of the enclosing method + * @param obligations the current set of tracked obligations. If ownership is transferred, it is + * side-effected to remove the obligations of the returned value. + */ + private void handleReturn( + ReturnNode node, ControlFlowGraph cfg, Set> obligations) { + if (isTransferOwnershipAtReturn(cfg)) { + Node returnExpr = node.getResult(); + returnExpr = getTempVarOrNode(returnExpr); + if (returnExpr instanceof LocalVariableNode) { + removeObligationContainingVar(obligations, (LocalVariableNode) returnExpr); + } + } + } + + /** + * Helper method that gets the temporary node corresponding to {@code node}, if one exists. If + * not, this method just returns its input. + * + * @param node a node + * @return the temporary for node, or node if no temporary exists + */ + private Node getTempVarOrNode(final Node node) { + Node temp = typeFactory.getTempVarForNode(node); + if (temp != null) { + return temp; + } + return node; + } + + /** + * Should we transfer ownership to the return type of the method corresponding to a CFG? Returns + * true when there is no {@link NotOwning} annotation on the return type. + * + * @param cfg the CFG of the method + * @return true iff we should transfer ownership to the return type of the method corresponding to + * a CFG + */ + private boolean isTransferOwnershipAtReturn(ControlFlowGraph cfg) { + if (checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP)) { + // If not using LO, default to always transferring at return, just like ECJ does. + return true; + } + + UnderlyingAST underlyingAST = cfg.getUnderlyingAST(); + if (underlyingAST instanceof UnderlyingAST.CFGMethod) { + // TODO: lambdas? I think in that case we'll return false, below. + MethodTree method = ((UnderlyingAST.CFGMethod) underlyingAST).getMethod(); + ExecutableElement executableElement = TreeUtils.elementFromDeclaration(method); + return typeFactory.getDeclAnnotation(executableElement, NotOwning.class) == null; + } + return false; + } + + /** + * Updates a set of obligations to account for an assignment. Assigning to an owning field might + * remove obligations, assigning to a new local variable might modify an obligation (by increasing + * the size of its resource alias set), etc. + * + * @param node the assignment + * @param obligations the set of obligations to update + */ + private void handleAssignment( + AssignmentNode node, Set> obligations) { + Node lhs = node.getTarget(); + Element lhsElement = TreeUtils.elementFromTree(lhs.getTree()); + // Use the temporary variable for the rhs if it exists. + Node rhs = removeCasts(node.getExpression()); + rhs = getTempVarOrNode(rhs); + + // Ownership transfer to @Owning field. + if (lhsElement.getKind() == ElementKind.FIELD) { + boolean isOwningField = + !checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) + && typeFactory.getDeclAnnotation(lhsElement, Owning.class) != null; + // Check that there is no obligation on the lhs, if the field is non-final and owning. + if (isOwningField + && typeFactory.canCreateObligations() + && !ElementUtils.isFinal(lhsElement)) { + checkReassignmentToField(node, obligations); + } + // Remove obligations from local variables, now that the owning field is responsible. + // (When obligation creation is turned off, non-final fields cannot take ownership.) + if (isOwningField + && rhs instanceof LocalVariableNode + && (typeFactory.canCreateObligations() || ElementUtils.isFinal(lhsElement))) { + removeObligationContainingVar(obligations, (LocalVariableNode) rhs); + } + } else if (lhs instanceof LocalVariableNode) { + LocalVariableNode lhsVar = (LocalVariableNode) lhs; + doGenKillForPseudoAssignment(node, obligations, lhsVar, rhs); + } + } + + /** + * Remove any obligations that contain {@code var} in their resource-alias set. + * + * @param obligations the set of obligations + * @param var a variable + */ + private void removeObligationContainingVar( + Set> obligations, LocalVariableNode var) { + Set obligationForVar = getResourceAliasSetForVar(obligations, var); + if (obligationForVar != null) { + obligations.remove(obligationForVar); + } + } + + /** + * Remove any {@link TypeCastNode}s wrapping a node, returning the operand nested within the type + * casts. + * + * @param node a node + * @return node, but with any surrounding typecasts removed + */ + private Node removeCasts(Node node) { + while (node instanceof TypeCastNode) { + node = ((TypeCastNode) node).getOperand(); + } + return node; + } + + /** + * Update a set of tracked obligations to account for a (pseudo-)assignment to some variable, as + * in a gen-kill dataflow analysis problem. Pseudo-assignments may include operations that + * "assign" to a temporary variable, exposing the possible value flow into the variable. E.g., for + * a ternary expression {@code b ? x : y} whose temporary variable is {@code t}, this method may + * process "assignments" {@code t = x} and {@code t = y}, thereby capturing the two possible + * values of {@code t}. + * + * @param node the node performing the pseudo-assignment; it is not necessarily an assignment node + * @param obligations the tracked obligations, which will be side-effected + * @param lhsVar the left-hand side variable for the pseudo-assignment + * @param rhs the right-hand side for the pseudo-assignment, which must have been converted to a + * temporary variable (via a call to {@link + * ResourceLeakAnnotatedTypeFactory#getTempVarForNode(Node)}) + */ + private void doGenKillForPseudoAssignment( + Node node, + Set> obligations, + LocalVariableNode lhsVar, + Node rhs) { + // Replacements to eventually perform in obligations. We keep this map to avoid a + // ConcurrentModificationException in the loop below. + Map, ImmutableSet> replacements = + new LinkedHashMap<>(); + // construct lhsVarWithTreeToGen once outside the loop for efficiency + LocalVarWithTree lhsVarWithTreeToGen = + new LocalVarWithTree(new LocalVariable(lhsVar), node.getTree()); + for (ImmutableSet obligation : obligations) { + Set kill = new LinkedHashSet<>(); + // always kill the lhs var if present + addLocalVarWithTreeToSetIfPresent(obligation, lhsVar.getElement(), kill); + LocalVarWithTree gen = null; + // if rhs is a variable tracked in the obligation's resource alias set, gen the lhs + if (rhs instanceof LocalVariableNode) { + LocalVariableNode rhsVar = (LocalVariableNode) rhs; + for (LocalVarWithTree lvwt : obligation) { + if (lvwt.localVar.getElement().equals(rhsVar.getElement())) { + gen = lhsVarWithTreeToGen; + // We remove temp vars from tracking once they are assigned to another location. + if (typeFactory.isTempVar(rhsVar)) { + addLocalVarWithTreeToSetIfPresent(obligation, rhsVar.getElement(), kill); + } + break; + } + } + } + // Check if there is something to do before creating a new obligation, for efficiency. + if (kill.isEmpty() && gen == null) { + continue; + } + Set newObligation = new LinkedHashSet<>(obligation); + newObligation.removeAll(kill); + if (gen != null) { + newObligation.add(gen); + } + if (newObligation.size() == 0) { + // We have killed the last reference to the resource; check the must-call obligation. + MustCallAnnotatedTypeFactory mcAtf = + typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + checkMustCall( + obligation, + typeFactory.getStoreBefore(node), + mcAtf.getStoreBefore(node), + "variable overwritten by assignment " + node.getTree()); + } + replacements.put(obligation, ImmutableSet.copyOf(newObligation)); + } + // Finally, update obligations according to the replacements. + for (Map.Entry, ImmutableSet> entry : + replacements.entrySet()) { + obligations.remove(entry.getKey()); + if (!entry.getValue().isEmpty()) { + obligations.add(entry.getValue()); + } + } + } + + /** + * Add each {@link LocalVarWithTree} in {@code resourceAliasSet} whose variable element is {@code + * element} to {@code lvwtSet}. + * + * @param resourceAliasSet the resource-alias set of an obligation + * @param element the element to search for + * @param lvwtSet the set to add to + */ + private void addLocalVarWithTreeToSetIfPresent( + ImmutableSet resourceAliasSet, + Element element, + Set lvwtSet) { + for (LocalVarWithTree lvwt : resourceAliasSet) { + if (lvwt.localVar.getElement().equals(element)) { + lvwtSet.add(lvwt); + } + } + } + + /** + * Issues an error if the given re-assignment to a non-final, owning field is not valid. A + * re-assignment is valid if the called methods type of the lhs before the assignment satisfies + * the must-call obligations of the field. + * + * @param node an assignment to a non-final, owning field + * @param obligations current tracked obligations + */ + private void checkReassignmentToField( + AssignmentNode node, Set> obligations) { + + Node lhsNode = node.getTarget(); + + if (!(lhsNode instanceof FieldAccessNode)) { + throw new BugInCF( + "checkReassignmentToField: non-field node " + node + " of type " + node.getClass()); + } + + FieldAccessNode lhs = (FieldAccessNode) lhsNode; + Node receiver = lhs.getReceiver(); + + // TODO: it would be better to defer getting the path until after we check + // for a CreatesMustCallFor annotation, because getting the path can be expensive. + // It might be possible to exploit the CFG structure to find the containing + // method (rather than using the path, as below), because if a method is being + // analyzed then it should be the root of the CFG (I think). + TreePath currentPath = typeFactory.getPath(node.getTree()); + MethodTree enclosingMethodTree = TreePathUtil.enclosingMethod(currentPath); + + if (enclosingMethodTree == null) { + // Assignments outside of methods must be field initializers, which + // are always safe. (Note that we don't support static owning fields, + // so static initializers don't need to be considered here.) + return; + } + + // Check that there is a corresponding CreatesMustCallFor annotation, unless this is + // 1) an assignment to a field of a newly-declared local variable whose scope does not + // extend beyond the method's body (and which therefore could not be targeted by an annotation + // on the method declaration), or 2) the rhs is a null literal (so there's nothing to reset). + if (!(receiver instanceof LocalVariableNode + && varTrackedInObligations((LocalVariableNode) receiver, obligations)) + && !(node.getExpression() instanceof NullLiteralNode)) { + checkEnclosingMethodIsCreatesMustCallFor(node, enclosingMethodTree); + } + + MustCallAnnotatedTypeFactory mcTypeFactory = + typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + AnnotationMirror mcAnno = + mcTypeFactory.getAnnotationFromJavaExpression( + JavaExpression.fromNode(lhs), node.getTree(), MustCall.class); + List mcValues = + AnnotationUtils.getElementValueArray( + mcAnno, mcTypeFactory.getMustCallValueElement(), String.class); + + if (mcValues.isEmpty()) { + return; + } + + // Get the store before the RHS rather than the assignment node, because the CFG always has + // the RHS first. If the RHS has side-effects, then the assignment node's store will have + // had its inferred types erased. + Node rhs = node.getExpression(); + CFStore cmStoreBefore = typeFactory.getStoreBefore(rhs); + CFValue cmValue = cmStoreBefore == null ? null : cmStoreBefore.getValue(lhs); + AnnotationMirror cmAnno = null; + if (cmValue != null) { + for (AnnotationMirror anno : cmValue.getAnnotations()) { + if (AnnotationUtils.areSameByName( + anno, "org.checkerframework.checker.calledmethods.qual.CalledMethods")) { + cmAnno = anno; + break; + } + } + } + if (cmAnno == null) { + cmAnno = typeFactory.top; + } + if (!calledMethodsSatisfyMustCall(mcValues, cmAnno)) { + Element lhsElement = TreeUtils.elementFromTree(lhs.getTree()); + if (!checker.shouldSkipUses(lhsElement)) { + checker.reportError( + node.getTree(), + "required.method.not.called", + formatMissingMustCallMethods(mcValues), + lhsElement.asType().toString(), + " Non-final owning field might be overwritten"); + } + } + } + + /** + * Checks that the method that encloses an assignment is marked with @CreatesMustCallFor + * annotation whose target is the object whose field is being re-assigned. + * + * @param node an assignment node whose lhs is a non-final, owning field + * @param enclosingMethod the MethodTree in which the re-assignment takes place + */ + private void checkEnclosingMethodIsCreatesMustCallFor( + AssignmentNode node, MethodTree enclosingMethod) { + Node lhs = node.getTarget(); + if (!(lhs instanceof FieldAccessNode)) { + return; + } + + String receiverString = receiverAsString((FieldAccessNode) lhs); + if ("this".equals(receiverString) && TreeUtils.isConstructor(enclosingMethod)) { + // Constructors always create must-call obligations, so there is no need for them to + // be annotated. + return; + } + ExecutableElement enclosingMethodElt = TreeUtils.elementFromDeclaration(enclosingMethod); + MustCallAnnotatedTypeFactory mcAtf = + typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + + List cmcfValues = + ResourceLeakVisitor.getCreatesMustCallForValues(enclosingMethodElt, mcAtf, typeFactory); + + if (cmcfValues.isEmpty()) { + checker.reportError( + enclosingMethod, + "missing.creates.mustcall.for", + receiverString, + ((FieldAccessNode) lhs).getFieldName()); + return; + } + + List checked = new ArrayList<>(); + for (String targetStrWithoutAdaptation : cmcfValues) { + String targetStr = null; + try { + targetStr = + StringToJavaExpression.atMethodBody( + targetStrWithoutAdaptation, enclosingMethod, checker) + .toString(); + } catch (JavaExpressionParseException e) { + targetStr = targetStrWithoutAdaptation; + } + if (targetStr.equals(receiverString)) { + // This @CreatesMustCallFor annotation matches. + return; + } + checked.add(targetStr); + } + checker.reportError( + enclosingMethod, + "incompatible.creates.mustcall.for", + receiverString, + ((FieldAccessNode) lhs).getFieldName(), + String.join(", ", checked)); + } + + /** + * Gets a standardized name for an object whose field is being re-assigned. + * + * @param fieldAccessNode a field access node + * @return the name of the object whose field is being accessed (the receiver), as a string + */ + private String receiverAsString(FieldAccessNode fieldAccessNode) { + Node receiver = fieldAccessNode.getReceiver(); + if (receiver instanceof ThisNode) { + return "this"; + } + if (receiver instanceof LocalVariableNode) { + + return ((LocalVariableNode) receiver).getName(); + } + throw new BugInCF( + "unexpected receiver of field assignment: " + receiver + " of type " + receiver.getClass()); + } + + /** + * Finds the argument passed in the {@code @MustCallAlias} position for a call. + * + * @param callNode callNode representing the call + * @return if {@code callNode} invokes a method with a {@code @MustCallAlias} annotation on some + * formal parameter (or the receiver), returns the result of calling {@link + * #removeCastsAndGetTmpVarIfPresent(Node)} on the argument passed in that position. + * Otherwise, returns {@code null}. + */ + private @Nullable Node getMustCallAliasArgumentNode(Node callNode) { + Preconditions.checkArgument( + callNode instanceof MethodInvocationNode || callNode instanceof ObjectCreationNode); + if (!typeFactory.hasMustCallAlias(callNode.getTree())) { + return null; + } + + Node result = null; + List args = getArgumentsOfInvocation(callNode); + List parameters = getParametersOfInvocation(callNode); + for (int i = 0; i < args.size(); i++) { + if (typeFactory.hasMustCallAlias(parameters.get(i))) { + result = args.get(i); + break; + } + } + + // If none of the parameters were @MustCallAlias, it must be the receiver + if (result == null && callNode instanceof MethodInvocationNode) { + result = ((MethodInvocationNode) callNode).getTarget().getReceiver(); + } + + result = removeCastsAndGetTmpVarIfPresent(result); + return result; + } + + /** + * If a temporary variable exists for node after typecasts have been removed, return it. + * Otherwise, return node. + * + * @param node a node + * @return either a tempvar for node's content sans typecasts, or node + */ + private Node removeCastsAndGetTmpVarIfPresent(Node node) { + // TODO: Create temp vars for TypeCastNodes as well, so we don't need to explicitly remove casts + // here. + node = removeCasts(node); + return getTempVarOrNode(node); + } + + /** + * Get the nodes representing the arguments of a method or constructor invocation from the + * invocation node. + * + * @param node a MethodInvocation or ObjectCreation node + * @return the arguments, in order + */ + private List getArgumentsOfInvocation(Node node) { + if (node instanceof MethodInvocationNode) { + MethodInvocationNode invocationNode = (MethodInvocationNode) node; + return invocationNode.getArguments(); + } else if (node instanceof ObjectCreationNode) { + return ((ObjectCreationNode) node).getArguments(); + } else { + throw new BugInCF("unexpected node type " + node.getClass()); + } + } + + /** + * Get the elements representing the formal parameters of a method or constructor, from an + * invocation of that method or constructor. + * + * @param node a method invocation or object creation node + * @return a list of the declarations of the formal parameters of the method or constructor being + * invoked + */ + private List getParametersOfInvocation(Node node) { + ExecutableElement executableElement; + if (node instanceof MethodInvocationNode) { + MethodInvocationNode invocationNode = (MethodInvocationNode) node; + executableElement = TreeUtils.elementFromUse(invocationNode.getTree()); + } else if (node instanceof ObjectCreationNode) { + executableElement = TreeUtils.elementFromUse(((ObjectCreationNode) node).getTree()); + } else { + throw new BugInCF("unexpected node type " + node.getClass()); + } + + return executableElement.getParameters(); + } + + /** + * Does the method being invoked have a not-owning return type? + * + * @param node a method invocation + * @return true iff the checker is not in no-lightweight-ownership mode and (1) the method has a + * void return type, or (2) a NotOwning annotation is present on the method declaration + */ + private boolean hasNotOwningReturnType(MethodInvocationNode node) { + if (checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP)) { + // Default to always transferring at return if not using LO, just like ECJ does. + return false; + } + MethodInvocationTree methodInvocationTree = node.getTree(); + ExecutableElement executableElement = TreeUtils.elementFromUse(methodInvocationTree); + // void methods are "not owning" by construction + return (ElementUtils.getType(executableElement).getKind() == TypeKind.VOID) + || (typeFactory.getDeclAnnotation(executableElement, NotOwning.class) != null); + } + + /** + * Get all successor blocks for some block, except for those corresponding to ignored exception + * types. + * + * @param block input block + * @return set of pairs (b, t), where b is a successor block, and t is the type of exception for + * the CFG edge from block to b, or {@code null} if b is a non-exceptional successor + */ + private Set> getSuccessorsExceptIgnoredExceptions(Block block) { + if (block.getType() == Block.BlockType.EXCEPTION_BLOCK) { + ExceptionBlock excBlock = (ExceptionBlock) block; + Set> result = new LinkedHashSet<>(); + // regular successor + Block regularSucc = excBlock.getSuccessor(); + if (regularSucc != null) { + result.add(Pair.of(regularSucc, null)); + } + // non-ignored exception successors + Map> exceptionalSuccessors = excBlock.getExceptionalSuccessors(); + for (Map.Entry> entry : exceptionalSuccessors.entrySet()) { + TypeMirror exceptionType = entry.getKey(); + if (!isIgnoredExceptionType(((Type) exceptionType).tsym.getQualifiedName())) { + for (Block exSucc : entry.getValue()) { + result.add(Pair.of(exSucc, exceptionType)); + } + } + } + return result; + } else { + Set> result = new LinkedHashSet<>(); + for (Block b : block.getSuccessors()) { + result.add(Pair.of(b, null)); + } + return result; + } + } + + /** + * Propagates a set of obligations to successors, and performs consistency checks when variables + * are going out of scope. + * + * @param visited block-obligations pairs already analyzed or already on the worklist + * @param worklist current worklist + * @param obligations obligations to propagate to successors + * @param curBlock the current block + */ + private void handleSuccessorBlocks( + Set visited, + Deque worklist, + Set> obligations, + Block curBlock) { + List curBlockNodes = curBlock.getNodes(); + for (Pair succAndExcType : + getSuccessorsExceptIgnoredExceptions(curBlock)) { + Block succ = succAndExcType.first; + TypeMirror exceptionType = succAndExcType.second; + Set> curObligations = + handleTernarySuccIfNeeded(curBlock, succ, obligations); + // obligationsForSucc eventually contains the obligations to propagate to succ. The loop + // below mutates it. + Set> obligationsForSucc = new LinkedHashSet<>(); + // A detailed reason to give in the case that a relevant variable goes out of scope with an + // unsatisfied obligation along the current control-flow edge. + String reasonForSucc = + exceptionType == null + ? + // Technically the variable may be going out of scope before the method exit, but that + // doesn't seem to provide additional helpful information. + "regular method exit" + : "possible exceptional exit due to " + + ((ExceptionBlock) curBlock).getNode().getTree() + + " with exception type " + + exceptionType; + CFStore succRegularStore = analysis.getInput(succ).getRegularStore(); + for (ImmutableSet resourceAliasSet : curObligations) { + boolean noInfoInSuccStoreForVars = true; + for (LocalVarWithTree resourceAlias : resourceAliasSet) { + if (!varNotPresentInStoreAndNotForTernary(succRegularStore, resourceAlias)) { + noInfoInSuccStoreForVars = false; + break; + } + } + if (succ instanceof SpecialBlockImpl /* exit block */ || noInfoInSuccStoreForVars) { + MustCallAnnotatedTypeFactory mcAtf = + typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + + // If succ is an exceptional successor, and resourceAliasSet represents the temporary + // variable for + // curBlock's node, do not propagate, as in the exceptional case the "assignment" to + // the temporary variable does not succeed. + if (exceptionType != null) { + Node exceptionalNode = removeCasts(((ExceptionBlock) curBlock).getNode()); + LocalVariableNode tmpVarForExcNode = typeFactory.getTempVarForNode(exceptionalNode); + if (tmpVarForExcNode != null + && resourceAliasSet.size() == 1 + && Iterables.getOnlyElement(resourceAliasSet) + .localVar + .getElement() + .equals(tmpVarForExcNode.getElement())) { + break; + } + } + + // Always propagate resourceAliasSet to successor if current block represents code nested + // in a cast or + // ternary expression. Without this logic, the analysis may report a false positive in + // when the resourceAliasSet represents a temporary variable for a nested expression, as + // the temporary + // may not appear in the successor store and hence seems to be going out of scope. The + // temporary will be handled with special logic; casts are unwrapped at various points in + // the analysis, and ternary expressions are handled by handleTernarySuccIfNeeded. + if (curBlockNodes.size() == 1 && inCastOrTernary(curBlockNodes.get(0))) { + obligationsForSucc.add(resourceAliasSet); + break; + } + + if (curBlockNodes.size() == 0 /* curBlock is special or conditional */) { + // Use the store from the block actually being analyzed, rather than succRegularStore, + // if succRegularStore contains no information about the variables of interest. + // In the case where none of the aliases in resourceAliasSet appear in + // succRegularStore, the resource is going out of scope, and it doesn't make + // sense to pass succRegularStore to checkMustCall - the successor store will + // not have any information about it, by construction, and + // any information in the previous store remains true. If any locals from the resource + // alias set do appear in succRegularStore, we will always use that store. + CFStore cmStore = + noInfoInSuccStoreForVars + ? analysis.getInput(curBlock).getRegularStore() + : succRegularStore; + CFStore mcStore = mcAtf.getStoreForBlock(noInfoInSuccStoreForVars, curBlock, succ); + checkMustCall(resourceAliasSet, cmStore, mcStore, reasonForSucc); + } else { // In this case, current block has at least one node. + // Use the called-methods store immediately after the last node in curBlock. + Node last = curBlockNodes.get(curBlockNodes.size() - 1); + CFStore cmStoreAfter = typeFactory.getStoreAfter(last); + // 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. + CFStore mcStore; + if (exceptionType != null && isInvocationOfCreatesMustCallForMethod(last)) { + mcStore = mcAtf.getStoreBefore(last); + } else { + mcStore = mcAtf.getStoreAfter(last); + } + checkMustCall(resourceAliasSet, cmStoreAfter, mcStore, reasonForSucc); + } + + } else { // In this case, there is info in the successor store about some alias in + // resourceAliasSet. + // Handles the possibility that some resource in the resourceAliasSet may go out of scope. + Set obligationCopy = new LinkedHashSet<>(resourceAliasSet); + obligationCopy.removeIf( + assign -> varNotPresentInStoreAndNotForTernary(succRegularStore, assign)); + obligationsForSucc.add(ImmutableSet.copyOf(obligationCopy)); + } + } + + propagate(new BlockWithObligations(succ, obligationsForSucc), visited, worklist); + } + } + + /** + * Returns true if {@code assign.localVar} has no value in {@code store} and {@code assign.tree} + * is not a {@link ConditionalExpressionTree}. The check for a {@link ConditionalExpressionTree} + * is to accommodate our handling of ternary expressions, where we track the temporary variable + * for the expression at the program point before that expression; see {@link + * #handleTernarySuccIfNeeded(Block, Block, Set)}. + * + * @param store the store to check + * @param assign the lvt to check + * @return true if the variable is not present in store and is not for a ternary + */ + private boolean varNotPresentInStoreAndNotForTernary(CFStore store, LocalVarWithTree assign) { + return store.getValue(assign.localVar) == null + && !(assign.tree instanceof ConditionalExpressionTree); + } + + /** + * Handles control-flow to a block starting with a {@link TernaryExpressionNode}. + * + *

In the Checker Framework's CFG, a ternary expression is represented as a {@link + * org.checkerframework.dataflow.cfg.block.ConditionalBlock}, whose successor blocks are the two + * cases of the ternary expression. The {@link TernaryExpressionNode} is the first node in the + * successor block of the two cases. + * + *

To handle this representation, we treat the control-flow transition from a node for a + * ternary expression case c to the successor {@link TernaryExpressionNode} t as + * a pseudo-assignment from c to the temporary variable for t. With this + * handling, the obligations reaching the successor node of t will properly account for + * the execution of case c. + * + *

If the successor block does not begin with a {@link TernaryExpressionNode} that needs to be + * handled, this method simply returns {@code obligations}. + * + * @param pred the predecessor block, potentially corresponding to the ternary expression case + * @param succ the successor block, potentially starting with a {@link TernaryExpressionNode} + * @param obligations the obligations before the control-flow transition + * @return a new set of obligations to account for the {@link TernaryExpressionNode}, or just + * {@code obligations} if no handling is required. + */ + private Set> handleTernarySuccIfNeeded( + Block pred, Block succ, Set> obligations) { + List succNodes = succ.getNodes(); + if (succNodes.isEmpty() || !(succNodes.get(0) instanceof TernaryExpressionNode)) { + return obligations; + } + TernaryExpressionNode ternaryNode = (TernaryExpressionNode) succNodes.get(0); + LocalVariableNode ternaryTempVar = typeFactory.getTempVarForNode(ternaryNode); + if (ternaryTempVar == null) { + return obligations; + } + List predNodes = pred.getNodes(); + // Right-hand side of the pseudo-assignment to the ternary expression temporary variable. + Node rhs = removeCasts(predNodes.get(predNodes.size() - 1)); + if (!(rhs instanceof LocalVariableNode)) { + rhs = typeFactory.getTempVarForNode(rhs); + if (rhs == null) { + return obligations; + } + } + Set> newDefs = new LinkedHashSet<>(obligations); + doGenKillForPseudoAssignment(ternaryNode, newDefs, ternaryTempVar, rhs); + return newDefs; + } + + /** + * 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. + * + * @param cfg the CFG + * @return the owning formal parameters of the method that corresponds to the given cfg + */ + private Set> computeOwningParameters(ControlFlowGraph cfg) { + Set> result = new LinkedHashSet<>(); + UnderlyingAST underlyingAST = cfg.getUnderlyingAST(); + if (underlyingAST instanceof UnderlyingAST.CFGMethod) { + // TODO what about lambdas? + MethodTree method = ((UnderlyingAST.CFGMethod) underlyingAST).getMethod(); + for (VariableTree param : method.getParameters()) { + Element paramElement = TreeUtils.elementFromDeclaration(param); + boolean isMustCallAlias = typeFactory.hasMustCallAlias(paramElement); + if (isMustCallAlias + || (typeFactory.hasDeclaredMustCall(param) + && !checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) + && paramElement.getAnnotation(Owning.class) != null)) { + result.add(ImmutableSet.of(new LocalVarWithTree(new LocalVariable(paramElement), param))); + // Increment numMustCall for each @Owning parameter tracked by the enclosing method. + incrementNumMustCall(paramElement); + } + } + } + return result; + } + + /** + * Checks whether there is some resource alias set R in {@code obligations} such that + * R contains a {@link LocalVarWithTree} whose local variable is {@code node}. + * + * @param var the local variable to look for + * @param obligations the set of obligations to search + * @return true iff there is a resource alias set in obligations that contains node + */ + private static boolean varTrackedInObligations( + LocalVariableNode var, Set> obligations) { + Element nodeElement = var.getElement(); + for (Set resourceAliasSet : obligations) { + for (LocalVarWithTree alias : resourceAliasSet) { + if (alias.localVar.getElement().equals(nodeElement)) { + return true; + } + } + } + return false; + } + + /** + * Gets the resource alias set that contains the given local variable, if one exists in + * obligations. + * + * @param obligations set of obligations + * @param node variable of interest + * @return the resource alias set in {@code obligations} containing {@code node}, or {@code null} + * if there is no such set + */ + private static @Nullable ImmutableSet getResourceAliasSetForVar( + Set> obligations, LocalVariableNode node) { + Element nodeElement = node.getElement(); + for (ImmutableSet resourceAliasSet : obligations) { + for (LocalVarWithTree lvwt : resourceAliasSet) { + if (lvwt.localVar.getElement().equals(nodeElement)) { + return resourceAliasSet; + } + } + } + return null; + } + + /** + * For the given resourceAliasSet, checks that at least one of its variables has its + * {@code @MustCall} obligation satisfied, based on {@code @CalledMethods} and {@code @MustCall} + * types in the given stores. + * + * @param resourceAliasSet the resourceAliasSet + * @param cmStore the called-methods store + * @param mcStore the must-call store + * @param outOfScopeReason if the {@code @MustCall} obligation is not satisfied, a useful + * explanation to include in the error message + */ + private void checkMustCall( + ImmutableSet resourceAliasSet, + CFStore cmStore, + CFStore mcStore, + String outOfScopeReason) { + + List mustCallValue = typeFactory.getMustCallValue(resourceAliasSet, mcStore); + // optimization: if there are no must-call methods, we do not need to perform the check + if (mustCallValue == null || mustCallValue.isEmpty()) { + return; + } + + boolean mustCallSatisfied = false; + for (LocalVarWithTree alias : resourceAliasSet) { + + // sometimes the store is null! this looks like a bug in checker dataflow. + // TODO track down and report the root-cause bug + CFValue lhsCFValue = cmStore != null ? cmStore.getValue(alias.localVar) : null; + AnnotationMirror cmAnno = null; + + if (lhsCFValue != null) { // When store contains the lhs + for (AnnotationMirror anno : lhsCFValue.getAnnotations()) { + if (AnnotationUtils.areSameByName( + anno, "org.checkerframework.checker.calledmethods.qual.CalledMethods")) { + cmAnno = anno; + } + } + } + if (cmAnno == null) { + cmAnno = + typeFactory + .getAnnotatedType(alias.localVar.getElement()) + .getAnnotationInHierarchy(typeFactory.top); + } + + if (calledMethodsSatisfyMustCall(mustCallValue, cmAnno)) { + mustCallSatisfied = true; + break; + } + } + + if (!mustCallSatisfied) { + // Report the error at the first alias' definition. This choice is arbitrary but consistent. + LocalVarWithTree firstAlias = resourceAliasSet.iterator().next(); + if (!reportedMustCallErrors.contains(firstAlias)) { + if (!checker.shouldSkipUses(TreeUtils.elementFromTree(firstAlias.tree))) { + reportedMustCallErrors.add(firstAlias); + checker.reportError( + firstAlias.tree, + "required.method.not.called", + formatMissingMustCallMethods(mustCallValue), + firstAlias.localVar.getType().toString(), + outOfScopeReason); + } + } + } + } + + /** + * Increment the -AcountMustCall counter. + * + * @param node the node being counted, to extract the type + */ + private void incrementNumMustCall(Node node) { + if (checker.hasOption(ResourceLeakChecker.COUNT_MUST_CALL)) { + TypeMirror type = node.getType(); + incrementMustCallImpl(type); + } + } + + /** + * Increment the -AcountMustCall counter. + * + * @param elt the elt being counted, to extract the type + */ + private void incrementNumMustCall(Element elt) { + if (checker.hasOption(ResourceLeakChecker.COUNT_MUST_CALL)) { + TypeMirror type = elt.asType(); + incrementMustCallImpl(type); + } + } + + /** + * Shared implementation for the two version of countMustCall. Don't call this directly. + * + * @param type the type of the object that has a must call obligation + */ + private void incrementMustCallImpl(TypeMirror type) { + // only count uses of JDK classes, since that's what we report on in the paper + if (!isJdkClass(TypesUtils.getTypeElement(type).getQualifiedName().toString())) { + return; + } + checker.numMustCall++; + } + + /** + * Is the given class a java* class? This is a heuristic for whether the class was defined in the + * JDK. + * + * @param qualifiedName a fully qualified name of a class + * @return true iff the type's fully-qualified name starts with "java", indicating that it is from + * a java.* or javax.* package (probably) + */ + /* package-private */ static boolean isJdkClass(String qualifiedName) { + return qualifiedName.startsWith("java"); + } + + /** + * Do the called methods represented by the {@link CalledMethods} type {@code cmAnno} include all + * the methods in {@code mustCallValues}? + * + * @param mustCallValues the strings representing the must-call obligations + * @param cmAnno an annotation from the called-methods type hierarchy + * @return true iff cmAnno is a subtype of a called-methods annotation with the same values as + * mustCallValues + */ + private boolean calledMethodsSatisfyMustCall( + List mustCallValues, AnnotationMirror cmAnno) { + // Create this annotation and use a subtype test because there's no guarantee that + // cmAnno is actually an instance of CalledMethods: it could be CMBottom or CMPredicate. + AnnotationMirror cmAnnoForMustCallMethods = + typeFactory.createCalledMethods(mustCallValues.toArray(new String[0])); + return typeFactory.getQualifierHierarchy().isSubtype(cmAnno, cmAnnoForMustCallMethods); + } + + /** + * 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 Set ignoredExceptionTypes = + new HashSet<>( + 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 we'll 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, + // we could refine this exclusion by looking at the charset being requested. + UnsupportedEncodingException.class.getCanonicalName())); + + /** + * Is {@code exceptionClassName} an exception type we are ignoring, to avoid excessive false + * positives? For now we ignore {@code java.lang.Throwable}, {@code NullPointerException}, and the + * runtime exceptions that can occur at any point during the program due to something going wrong + * in the JVM, like OutOfMemoryError and ClassCircularityError. + * + * @param exceptionClassName the fully-qualified name of the exception + * @return true if the given exception class should be ignored + */ + private static boolean isIgnoredExceptionType(@FullyQualifiedName Name exceptionClassName) { + return ignoredExceptionTypes.contains(exceptionClassName.toString()); + } + + /** + * If the input {@code state} has not been visited yet, add it to {@code visited} and {@code + * worklist}. + * + * @param state the current state + * @param visited the states that have been analyzed or are already on the worklist + * @param worklist the states that will be analyzed + */ + private static void propagate( + BlockWithObligations state, + Set visited, + Deque worklist) { + + if (visited.add(state)) { + worklist.add(state); + } + } + + /** + * Formats a list of must-call method names to be printed in an error message. + * + * @param mustCallVal the list of must-call strings + * @return a formatted string + */ + /* package-private */ + static String formatMissingMustCallMethods(List mustCallVal) { + int size = mustCallVal.size(); + if (size == 0) { + throw new BugInCF("empty mustCallVal " + mustCallVal); + } else if (size == 1) { + return "method " + mustCallVal.get(0); + } else { + return "methods " + String.join(", ", mustCallVal); + } + } + + /** + * A pair of a {@link Block} and a set of obligations (i.e. dataflow facts) on entry to the block. + * Each obligation is an {@code ImmutableSet}, representing a set of resource + * aliases for some tracked resource. The analyzer's worklist consists of BlockWithObligations + * objects, each representing the need to handle the set of obligations reaching the block during + * analysis. + */ + private static class BlockWithObligations { + + /** The block. */ + public final Block block; + + /** The facts. */ + public final ImmutableSet> obligations; + + /** + * Create a new BlockWithObligations from a block and a set of obligations. + * + * @param b the block + * @param obligations the set of incoming obligations at the start of the block (may be the + * empty set) + */ + public BlockWithObligations(Block b, Set> obligations) { + this.block = b; + this.obligations = ImmutableSet.copyOf(obligations); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + BlockWithObligations that = (BlockWithObligations) o; + return block.equals(that.block) && obligations.equals(that.obligations); + } + + @Override + public int hashCode() { + return Objects.hash(block, obligations); + } + } + + /** + * A pair of a local variable along with a tree in the corresponding method that "assigns" the + * variable. Besides a normal assignment, the tree may be a {@link VariableTree} in the case of a + * formal parameter. We keep the tree for error-reporting purposes (so we can report an error per + * assignment to a local, pinpointing the expression whose MustCall may not be satisfied). + * + *

This class is used to represent a single resource alias in a resource alias set. + */ + /* package-private */ static class LocalVarWithTree { + + /** The variable. */ + public final LocalVariable localVar; + + /** The tree at which it was assigned, for error reporting. */ + public final Tree tree; + + /** + * Create a new LocalVarWithTree. + * + * @param localVar the local variable + * @param tree the tree + */ + public LocalVarWithTree(LocalVariable localVar, Tree tree) { + this.localVar = localVar; + this.tree = tree; + } + + @Override + public String toString() { + return "(LocalVarWithTree: localVar: " + localVar + " |||| tree: " + tree + ")"; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + LocalVarWithTree that = (LocalVarWithTree) o; + return localVar.equals(that.localVar) && tree.equals(that.tree); + } + + @Override + public int hashCode() { + return Objects.hash(localVar, tree); + } + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java new file mode 100644 index 00000000000..55c738bb708 --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java @@ -0,0 +1,362 @@ +package org.checkerframework.checker.resourceleak; + +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; +import com.google.common.collect.ImmutableSet; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import java.lang.annotation.Annotation; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.TypeElement; +import javax.lang.model.type.TypeKind; +import org.checkerframework.checker.calledmethods.CalledMethodsAnnotatedTypeFactory; +import org.checkerframework.checker.calledmethods.qual.CalledMethods; +import org.checkerframework.checker.calledmethods.qual.CalledMethodsBottom; +import org.checkerframework.checker.calledmethods.qual.CalledMethodsPredicate; +import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods; +import org.checkerframework.checker.mustcall.CreatesMustCallForElementSupplier; +import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory; +import org.checkerframework.checker.mustcall.MustCallChecker; +import org.checkerframework.checker.mustcall.MustCallNoCreatesMustCallForChecker; +import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; +import org.checkerframework.checker.mustcall.qual.MustCall; +import org.checkerframework.checker.mustcall.qual.MustCallAlias; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.checkerframework.checker.resourceleak.MustCallConsistencyAnalyzer.LocalVarWithTree; +import org.checkerframework.common.basetype.BaseTypeChecker; +import org.checkerframework.dataflow.cfg.ControlFlowGraph; +import org.checkerframework.dataflow.cfg.node.LocalVariableNode; +import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.dataflow.cfg.node.Node; +import org.checkerframework.dataflow.expression.LocalVariable; +import org.checkerframework.framework.flow.CFStore; +import org.checkerframework.framework.flow.CFValue; +import org.checkerframework.framework.type.AnnotatedTypeMirror; +import org.checkerframework.framework.type.GenericAnnotatedTypeFactory; +import org.checkerframework.javacutil.AnnotationUtils; +import org.checkerframework.javacutil.TreeUtils; +import org.checkerframework.javacutil.TypesUtils; + +/** + * The type factory for the Resource Leak Checker. The main difference between this and the Called + * Methods type factory from which it is derived is that this version's {@link + * #postAnalyze(ControlFlowGraph)} method checks that must-call obligations are fulfilled. + */ +public class ResourceLeakAnnotatedTypeFactory extends CalledMethodsAnnotatedTypeFactory + implements CreatesMustCallForElementSupplier { + + /** The MustCall.value element/field. */ + final ExecutableElement mustCallValueElement = + TreeUtils.getMethod(MustCall.class, "value", 0, processingEnv); + + /** The EnsuresCalledMethods.value element/field. */ + final ExecutableElement ensuresCalledMethodsValueElement = + TreeUtils.getMethod(EnsuresCalledMethods.class, "value", 0, processingEnv); + + /** The EnsuresCalledMethods.methods element/field. */ + final ExecutableElement ensuresCalledMethodsMethodsElement = + TreeUtils.getMethod(EnsuresCalledMethods.class, "methods", 0, processingEnv); + + /** The CreatesMustCallFor.List.value element/field. */ + private final ExecutableElement createsMustCallForListValueElement = + TreeUtils.getMethod(CreatesMustCallFor.List.class, "value", 0, processingEnv); + + /** The CreatesMustCallFor.value element/field. */ + private final ExecutableElement createsMustCallForValueElement = + TreeUtils.getMethod(CreatesMustCallFor.class, "value", 0, processingEnv); + + /** + * Bidirectional map to store temporary variables created for expressions with non-empty @MustCall + * obligations and the corresponding trees. Keys are the artificial local variable nodes created + * as temporary variables; values are the corresponding trees. + */ + private final BiMap tempVarToTree = HashBiMap.create(); + + /** + * Creates a new ResourceLeakAnnotatedTypeFactory. + * + * @param checker the checker associated with this type factory + */ + public ResourceLeakAnnotatedTypeFactory(final BaseTypeChecker checker) { + super(checker); + this.postInit(); + } + + @Override + protected Set> createSupportedTypeQualifiers() { + return getBundledTypeQualifiers( + CalledMethods.class, CalledMethodsBottom.class, CalledMethodsPredicate.class); + } + + /** + * Creates a @CalledMethods annotation whose values are the given strings. + * + * @param val the methods that have been called + * @return an annotation indicating that the given methods have been called + */ + public AnnotationMirror createCalledMethods(final String... val) { + return createAccumulatorAnnotation(Arrays.asList(val)); + } + + @Override + public void postAnalyze(ControlFlowGraph cfg) { + MustCallConsistencyAnalyzer mustCallConsistencyAnalyzer = + new MustCallConsistencyAnalyzer(this, this.analysis); + mustCallConsistencyAnalyzer.analyze(cfg); + super.postAnalyze(cfg); + tempVarToTree.clear(); + } + + /** + * Use the must-call store to get the must-call value of the resource represented by the given + * local variables. + * + * @param localVarWithTreeSet a set of local variables with their assignment trees, all of which + * represent the same resource + * @param mcStore a CFStore produced by the MustCall checker's dataflow analysis. If this is null, + * then the default MustCall type of each variable's class will be used. + * @return the list of must-call method names, or null if the resource's must-call obligations are + * unsatisfiable (i.e. its value in the Must Call store is MustCallUnknown) + */ + public @Nullable List getMustCallValue( + ImmutableSet localVarWithTreeSet, @Nullable CFStore mcStore) { + MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory = + getTypeFactoryOfSubchecker(MustCallChecker.class); + + // Need to get the LUB of the MC values, because if a CreatesMustCallFor method was + // called on just one of the locals then they all need to be treated as if + // they need to call the relevant methods. + AnnotationMirror mcLub = mustCallAnnotatedTypeFactory.BOTTOM; + for (LocalVarWithTree lvt : localVarWithTreeSet) { + AnnotationMirror mcAnno = null; + LocalVariable local = lvt.localVar; + CFValue value = mcStore == null ? null : mcStore.getValue(local); + if (value != null) { + mcAnno = getAnnotationByClass(value.getAnnotations(), MustCall.class); + } + if (mcAnno == null) { + // It wasn't in the store, so fall back to the default must-call type for the class. + // TODO: we currently end up in this case when checking a call to the return type + // of a returns-receiver method on something with a MustCall type; for example, + // see tests/socket/ZookeeperReport6.java. We should instead use a poly type if we + // can; that would probably require us to change the Must Call Checker to also + // track temporaries. + TypeElement typeElt = TypesUtils.getTypeElement(local.getType()); + if (typeElt == null) { + // typeElt is null if local.getType() was not a class, interface, annotation type, or + // enum---that is, was not an annotatable type. + // That shouldn't happen, but if it does fall back to a safe default (i.e. top). + mcAnno = mustCallAnnotatedTypeFactory.TOP; + } else { + // Why does this happen sometimes? + if (typeElt.asType().getKind() == TypeKind.VOID) { + return Collections.emptyList(); + } + mcAnno = + mustCallAnnotatedTypeFactory + .getAnnotatedType(typeElt) + .getAnnotationInHierarchy(mustCallAnnotatedTypeFactory.TOP); + } + } + mcLub = mustCallAnnotatedTypeFactory.getQualifierHierarchy().leastUpperBound(mcLub, mcAnno); + } + if (AnnotationUtils.areSameByName( + mcLub, "org.checkerframework.checker.mustcall.qual.MustCall")) { + return getMustCallValues(mcLub); + } else { + return null; + } + } + + /** + * Returns the {@link MustCall#value} element/argument of the @MustCall annotation on the type of + * {@code tree}. + * + *

If possible, prefer {@link #getMustCallValue(Tree)}, which accounts for flow-sensitive + * refinement. + * + * @param tree a tree + * @return the strings in its must-call type + */ + /* package-private */ List getMustCallValue(Tree tree) { + MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory = + getTypeFactoryOfSubchecker(MustCallChecker.class); + AnnotatedTypeMirror mustCallAnnotatedType = mustCallAnnotatedTypeFactory.getAnnotatedType(tree); + AnnotationMirror mustCallAnnotation = mustCallAnnotatedType.getAnnotation(MustCall.class); + return getMustCallValues(mustCallAnnotation); + } + + /** + * Returns the {@link MustCall#value} element/argument of the @MustCall annotation on the class + * type of {@code element}. + * + *

If possible, prefer {@link #getMustCallValue(Tree)}, which accounts for flow-sensitive + * refinement. + * + * @param element an element + * @return the strings in its must-call type + */ + /* package-private */ List getMustCallValue(Element element) { + MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory = + getTypeFactoryOfSubchecker(MustCallChecker.class); + AnnotatedTypeMirror mustCallAnnotatedType = + mustCallAnnotatedTypeFactory.getAnnotatedType(element); + AnnotationMirror mustCallAnnotation = mustCallAnnotatedType.getAnnotation(MustCall.class); + return getMustCallValues(mustCallAnnotation); + } + + /** + * Helper method for getting the must-call values from a must-call annotation. + * + * @param mustCallAnnotation a {@link MustCall} annotation, or null + * @return the strings in mustCallAnnotation's value element, or the empty list if + * mustCallAnnotation is null + */ + private List getMustCallValues(@Nullable AnnotationMirror mustCallAnnotation) { + if (mustCallAnnotation == null) { + return Collections.emptyList(); + } + return AnnotationUtils.getElementValueArray( + mustCallAnnotation, mustCallValueElement, String.class); + } + + /** + * Helper method to get the temporary variable that represents the given node, if one exists. + * + * @param node a node + * @return the tempvar for node's expression, or null if one does not exist + */ + /* package-private */ + @Nullable LocalVariableNode getTempVarForNode(Node node) { + return tempVarToTree.inverse().get(node.getTree()); + } + + /** + * Is the given node a temporary variable? + * + * @param node a node + * @return true iff the given node is a temporary variable + */ + /* package-private */ boolean isTempVar(Node node) { + return tempVarToTree.containsKey(node); + } + + /** + * Registers a temporary variable by adding it to this type factory's tempvar map. + * + * @param tmpVar a temporary variable + * @param tree the tree of the expression the tempvar represents + */ + /* package-private */ void addTempVar(LocalVariableNode tmpVar, Tree tree) { + tempVarToTree.put(tmpVar, tree); + } + + /** + * Returns true if the type of the tree includes a must-call annotation. Note that this method may + * not consider dataflow, and is only safe to use when you need the declared, rather than + * inferred, type of the tree. Use {@link #getMustCallValue(ImmutableSet, CFStore)} (and check for + * emptiness) if you are trying to determine whether a local variable has must-call obligations. + * + * @param tree a tree + * @return whether the tree has declared must-call obligations + */ + /* package-private */ boolean hasDeclaredMustCall(Tree tree) { + assert tree.getKind() == Kind.METHOD + || tree.getKind() == Kind.VARIABLE + || tree.getKind() == Kind.NEW_CLASS + || tree.getKind() == Kind.METHOD_INVOCATION + : "unexpected declaration tree kind: " + tree.getKind(); + return !getMustCallValue(tree).isEmpty(); + } + + /** + * Returns true if the given tree has an {@link MustCallAlias} annotation and resource-alias + * tracking is not disabled. + * + * @param tree a tree + * @return true if the given tree has an {@link MustCallAlias} annotation + */ + /* package-private */ boolean hasMustCallAlias(Tree tree) { + Element elt = TreeUtils.elementFromTree(tree); + return hasMustCallAlias(elt); + } + + /** + * Returns true if the given element has an {@link MustCallAlias} annotation and resource-alias + * tracking is not disabled. + * + * @param elt an element + * @return true if the given element has an {@link MustCallAlias} annotation + */ + /* package-private */ boolean hasMustCallAlias(Element elt) { + if (checker.hasOption(MustCallChecker.NO_RESOURCE_ALIASES)) { + return false; + } + MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory = + getTypeFactoryOfSubchecker(MustCallChecker.class); + return mustCallAnnotatedTypeFactory.getDeclAnnotationNoAliases(elt, MustCallAlias.class) + != null; + } + + /** + * Returns true if the declaration of the method being invoked has one or more {@link + * CreatesMustCallFor} annotations. + * + * @param node a method invocation node + * @return true iff there is one or more @CreatesMustCallFor annotations on the declaration of the + * invoked method + */ + public boolean hasCreatesMustCallFor(MethodInvocationNode node) { + ExecutableElement decl = TreeUtils.elementFromUse(node.getTree()); + return getDeclAnnotation(decl, CreatesMustCallFor.class) != null + || getDeclAnnotation(decl, CreatesMustCallFor.List.class) != null; + } + + /** + * Does this type factory support {@link CreatesMustCallFor}? + * + * @return true iff the -AnoCreatesMustCallFor command-line argument was not supplied to the + * checker + */ + public boolean canCreateObligations() { + return !checker.hasOption(MustCallChecker.NO_CREATES_MUSTCALLFOR); + } + + @Override + @SuppressWarnings("TypeParameterUnusedInFormals") // Intentional abuse + public > @Nullable T getTypeFactoryOfSubchecker( + Class subCheckerClass) { + if (subCheckerClass == MustCallChecker.class) { + if (!canCreateObligations()) { + return super.getTypeFactoryOfSubchecker(MustCallNoCreatesMustCallForChecker.class); + } + } + return super.getTypeFactoryOfSubchecker(subCheckerClass); + } + + /** + * Returns the {@link CreatesMustCallFor#value} element. + * + * @return the {@link CreatesMustCallFor#value} element + */ + @Override + public ExecutableElement getCreatesMustCallForValueElement() { + return createsMustCallForValueElement; + } + + /** + * Returns the {@link CreatesMustCallFor.List#value} element. + * + * @return the {@link CreatesMustCallFor.List#value} element + */ + @Override + public ExecutableElement getCreatesMustCallForListValueElement() { + return createsMustCallForListValueElement; + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java new file mode 100644 index 00000000000..e0f77dc017d --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java @@ -0,0 +1,88 @@ +package org.checkerframework.checker.resourceleak; + +import java.util.LinkedHashSet; +import javax.tools.Diagnostic.Kind; +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.common.basetype.BaseTypeChecker; +import org.checkerframework.common.basetype.BaseTypeVisitor; +import org.checkerframework.framework.source.SupportedOptions; + +/** + * The entry point for the Resource Leak Checker. This checker is a modifed {@link + * CalledMethodsChecker} that checks that the must-call obligations of each expression (as computed + * via the {@link org.checkerframework.checker.mustcall.MustCallChecker} have been fulfilled. + */ +@SupportedOptions({ + ResourceLeakChecker.COUNT_MUST_CALL, + MustCallChecker.NO_CREATES_MUSTCALLFOR, + MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP, + MustCallChecker.NO_RESOURCE_ALIASES +}) +public class ResourceLeakChecker extends CalledMethodsChecker { + + /** + * Command-line option for counting how many must-call obligations were checked by the Resource + * Leak Checker, and emitting the number after processing all files. Not of interest to most + * users. + */ + public static final String COUNT_MUST_CALL = "countMustCall"; + + /** + * The number of expressions with must-call obligations that were checked. Incremented only if the + * {@link #COUNT_MUST_CALL} command-line option was supplied. + */ + int numMustCall = 0; + + /** + * The number of must-call-related errors issued. The count of verified must-call expressions is + * the difference between this and {@link #numMustCall}. + */ + int numMustCallFailed = 0; + + @Override + protected LinkedHashSet> getImmediateSubcheckerClasses() { + LinkedHashSet> checkers = + super.getImmediateSubcheckerClasses(); + + if (this.processingEnv.getOptions().containsKey(MustCallChecker.NO_CREATES_MUSTCALLFOR)) { + checkers.add(MustCallNoCreatesMustCallForChecker.class); + } else { + checkers.add(MustCallChecker.class); + } + + return checkers; + } + + @Override + protected BaseTypeVisitor createSourceVisitor() { + return new ResourceLeakVisitor(this); + } + + @Override + public void reportError(Object source, @CompilerMessageKey String messageKey, Object... args) { + if (messageKey.equals("required.method.not.called")) { + // This is safe because of the message key. + String qualifiedTypeName = (String) args[1]; + // Only count classes in the JDK, not user-defined classes. + if (MustCallConsistencyAnalyzer.isJdkClass(qualifiedTypeName)) { + numMustCallFailed++; + } + } + super.reportError(source, messageKey, args); + } + + @Override + public void typeProcessingOver() { + if (hasOption(COUNT_MUST_CALL)) { + message(Kind.WARNING, "Found %d must call obligation(s).%n", numMustCall); + message( + Kind.WARNING, + "Successfully verified %d must call obligation(s).%n", + numMustCall - numMustCallFailed); + } + super.typeProcessingOver(); + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakTransfer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakTransfer.java new file mode 100644 index 00000000000..dd8365243c3 --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakTransfer.java @@ -0,0 +1,136 @@ +package org.checkerframework.checker.resourceleak; + +import java.util.List; +import javax.lang.model.element.AnnotationMirror; +import org.checkerframework.checker.calledmethods.CalledMethodsTransfer; +import org.checkerframework.checker.mustcall.CreatesMustCallForElementSupplier; +import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory; +import org.checkerframework.checker.mustcall.MustCallChecker; +import org.checkerframework.dataflow.analysis.TransferInput; +import org.checkerframework.dataflow.analysis.TransferResult; +import org.checkerframework.dataflow.cfg.node.LocalVariableNode; +import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.dataflow.cfg.node.Node; +import org.checkerframework.dataflow.cfg.node.ObjectCreationNode; +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.CFValue; +import org.checkerframework.javacutil.TypesUtils; + +/** The transfer function for the resource-leak extension to the called-methods type system. */ +public class ResourceLeakTransfer extends CalledMethodsTransfer { + + /** + * Shadowed because we must dispatch to the Resource Leak Checker's version of + * getTypefactoryOfSubchecker to get the correct MustCallAnnotatedTypeFactory. + */ + private final ResourceLeakAnnotatedTypeFactory rlTypeFactory; + + /** + * Create a new resource leak transfer function. + * + * @param analysis the analysis. Its type factory must be a {@link + * ResourceLeakAnnotatedTypeFactory}. + */ + public ResourceLeakTransfer(final CFAnalysis analysis) { + super(analysis); + this.rlTypeFactory = (ResourceLeakAnnotatedTypeFactory) analysis.getTypeFactory(); + } + + @Override + public TransferResult visitTernaryExpression( + TernaryExpressionNode node, TransferInput input) { + TransferResult result = super.visitTernaryExpression(node, input); + updateStoreWithTempVar(result, node); + return result; + } + + @Override + public TransferResult visitMethodInvocation( + final MethodInvocationNode node, final TransferInput input) { + + TransferResult result = super.visitMethodInvocation(node, input); + + handleCreatesMustCallFor(node, result); + updateStoreWithTempVar(result, node); + + // If there is a temporary variable for the receiver, update its type. + Node receiver = node.getTarget().getReceiver(); + MustCallAnnotatedTypeFactory mcAtf = + rlTypeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + Node accumulationTarget = mcAtf.getTempVar(receiver); + if (accumulationTarget != null) { + String methodName = node.getTarget().getMethod().getSimpleName().toString(); + methodName = rlTypeFactory.adjustMethodNameUsingValueChecker(methodName, node.getTree()); + accumulate(accumulationTarget, result, methodName); + } + + return result; + } + + /** + * Clears the called-methods store of all information about the target if an @CreatesMustCallFor + * method is invoked and the type factory can create obligations. Otherwise, does nothing. + * + * @param n a method invocation + * @param result the transfer result whose stores should be cleared of information + */ + private void handleCreatesMustCallFor( + MethodInvocationNode n, TransferResult result) { + if (!rlTypeFactory.canCreateObligations()) { + return; + } + + List targetExprs = + CreatesMustCallForElementSupplier.getCreatesMustCallForExpressions( + n, rlTypeFactory, rlTypeFactory); + AnnotationMirror defaultType = rlTypeFactory.top; + for (JavaExpression targetExpr : targetExprs) { + CFValue defaultTypeValue = + analysis.createSingleAnnotationValue(defaultType, targetExpr.getType()); + if (result.containsTwoStores()) { + result.getThenStore().replaceValue(targetExpr, defaultTypeValue); + result.getElseStore().replaceValue(targetExpr, defaultTypeValue); + } else { + result.getRegularStore().replaceValue(targetExpr, defaultTypeValue); + } + } + } + + @Override + public TransferResult visitObjectCreation( + ObjectCreationNode node, TransferInput input) { + TransferResult result = super.visitObjectCreation(node, input); + updateStoreWithTempVar(result, node); + return result; + } + + /** + * This method either creates or looks up the temp var t for node, and then updates the store to + * give t the same type as node. Temporary variables are supported for expressions throughout this + * checker (and the Must Call Checker) to enable refinement of their types. See the documentation + * of {@link MustCallConsistencyAnalyzer} for more details. + * + * @param node the node to be assigned to a temporary variable + * @param result the transfer result containing the store to be modified + */ + public void updateStoreWithTempVar(TransferResult result, Node node) { + // Must-call obligations on primitives are not supported. + if (!TypesUtils.isPrimitiveOrBoxed(node.getType())) { + MustCallAnnotatedTypeFactory mcAtf = + rlTypeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + LocalVariableNode temp = mcAtf.getTempVar(node); + if (temp != null) { + rlTypeFactory.addTempVar(temp, node.getTree()); + JavaExpression localExp = JavaExpression.fromNode(temp); + AnnotationMirror anm = + rlTypeFactory + .getAnnotatedType(node.getTree()) + .getAnnotationInHierarchy(rlTypeFactory.top); + insertIntoStores(result, localExp, anm == null ? rlTypeFactory.top : anm); + } + } + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java new file mode 100644 index 00000000000..ea820480e5b --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java @@ -0,0 +1,235 @@ +package org.checkerframework.checker.resourceleak; + +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.VariableTree; +import java.util.ArrayList; +import java.util.List; +import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import org.checkerframework.checker.calledmethods.CalledMethodsVisitor; +import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods; +import org.checkerframework.checker.mustcall.CreatesMustCallForElementSupplier; +import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory; +import org.checkerframework.checker.mustcall.MustCallChecker; +import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; +import org.checkerframework.checker.mustcall.qual.Owning; +import org.checkerframework.common.basetype.BaseTypeChecker; +import org.checkerframework.javacutil.AnnotationUtils; +import org.checkerframework.javacutil.ElementUtils; +import org.checkerframework.javacutil.TreeUtils; + +/** + * The visitor for the Resource Leak Checker. Responsible for checking that the rules for {@link + * Owning} fields are satisfied, and for checking that {@link CreatesMustCallFor} overrides are + * valid. + */ +public class ResourceLeakVisitor extends CalledMethodsVisitor { + + /** + * Because CalledMethodsVisitor doesn't have a type parameter, we need a reference to the type + * factory that has this static type to access the features that ResourceLeakAnnotatedTypeFactory + * implements but CalledMethodsAnnotatedTypeFactory does not. + */ + private final ResourceLeakAnnotatedTypeFactory rlTypeFactory; + + /** + * Create the visitor. + * + * @param checker the type-checker associated with this visitor + */ + public ResourceLeakVisitor(final BaseTypeChecker checker) { + super(checker); + rlTypeFactory = (ResourceLeakAnnotatedTypeFactory) atypeFactory; + } + + @Override + protected ResourceLeakAnnotatedTypeFactory createTypeFactory() { + return new ResourceLeakAnnotatedTypeFactory(checker); + } + + @Override + public Void visitMethod(MethodTree node, Void p) { + ExecutableElement elt = TreeUtils.elementFromDeclaration(node); + MustCallAnnotatedTypeFactory mcAtf = + rlTypeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); + List cmcfValues = getCreatesMustCallForValues(elt, mcAtf, rlTypeFactory); + if (!cmcfValues.isEmpty()) { + // If this method overrides another method, it must create at least as many + // obligations. Without this check, dynamic dispatch might allow e.g. a field to be + // overwritten by a CMCF method, but the CMCF effect wouldn't occur. + for (ExecutableElement overridden : ElementUtils.getOverriddenMethods(elt, this.types)) { + List overriddenCmcfValues = + getCreatesMustCallForValues(overridden, mcAtf, rlTypeFactory); + if (!overriddenCmcfValues.containsAll(cmcfValues)) { + String foundCmcfValueString = String.join(", ", cmcfValues); + String neededCmcfValueString = String.join(", ", overriddenCmcfValues); + String actualClassname = ElementUtils.getEnclosingClassName(elt); + String overriddenClassname = ElementUtils.getEnclosingClassName(overridden); + checker.reportError( + node, + "creates.mustcall.for.override.invalid", + actualClassname + "#" + elt, + overriddenClassname + "#" + overridden, + foundCmcfValueString, + neededCmcfValueString); + } + } + } + return super.visitMethod(node, p); + } + + /** + * Returns the {@link CreatesMustCallFor#value} element/argument of the given @CreatesMustCallFor + * annotation, or "this" if there is none. + * + *

Does not vipewpoint-adaptation. + * + * @param createsMustCallFor an @CreatesMustCallFor annotation + * @param mcAtf a MustCallAnnotatedTypeFactory, to source the value element + * @return the string value + */ + private static String getCreatesMustCallForValue( + AnnotationMirror createsMustCallFor, MustCallAnnotatedTypeFactory mcAtf) { + return AnnotationUtils.getElementValue( + createsMustCallFor, mcAtf.getCreatesMustCallForValueElement(), String.class, "this"); + } + + /** + * Returns all the {@link CreatesMustCallFor#value} elements/arguments of all @CreatesMustCallFor + * annotations on the given element. + * + *

Does no viewpoint-adaptation, unlike {@link + * CreatesMustCallForElementSupplier#getCreatesMustCallForExpressions} which does. + * + * @param elt an executable element + * @param mcAtf a MustCallAnnotatedTypeFactory, to source the value element + * @param atypeFactory a ResourceLeakAnnotatedTypeFactory + * @return the literal strings present in the @CreatesMustCallFor annotation(s) of that element, + * substituting the default "this" for empty annotations. This method returns the empty list + * iff there are no @CreatesMustCallFor annotations on elt. The returned list is always + * modifiable if it is non-empty. + */ + /*package-private*/ static List getCreatesMustCallForValues( + ExecutableElement elt, + MustCallAnnotatedTypeFactory mcAtf, + ResourceLeakAnnotatedTypeFactory atypeFactory) { + AnnotationMirror createsMustCallForList = + atypeFactory.getDeclAnnotation(elt, CreatesMustCallFor.List.class); + List result = new ArrayList<>(4); + if (createsMustCallForList != null) { + List createsMustCallFors = + AnnotationUtils.getElementValueArray( + createsMustCallForList, + mcAtf.getCreatesMustCallForListValueElement(), + AnnotationMirror.class); + for (AnnotationMirror cmcf : createsMustCallFors) { + result.add(getCreatesMustCallForValue(cmcf, mcAtf)); + } + } + AnnotationMirror createsMustCallFor = + atypeFactory.getDeclAnnotation(elt, CreatesMustCallFor.class); + if (createsMustCallFor != null) { + result.add(getCreatesMustCallForValue(createsMustCallFor, mcAtf)); + } + return result; + } + + @Override + public Void visitVariable(VariableTree node, Void p) { + Element varElement = TreeUtils.elementFromTree(node); + + if (varElement.getKind().isField() + && !checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) + && rlTypeFactory.getDeclAnnotation(varElement, Owning.class) != null) { + checkOwningField(varElement); + } + + return super.visitVariable(node, p); + } + + /** + * Checks validity of a field {@code field} with an {@code @}{@link Owning} annotation. Say the + * type of {@code field} is {@code @MustCall("m"}}. This method checks that the enclosing class of + * {@code field} has a type {@code @MustCall("m2")} for some method {@code m2}, and that {@code + * m2} has an annotation {@code @EnsuresCalledMethods(value = "this.field", methods = "m")}, + * guaranteeing that the {@code @MustCall} obligation of the field will be satisfied. + * + * @param field the declaration of the field to check + */ + private void checkOwningField(Element field) { + + if (checker.shouldSkipUses(field)) { + return; + } + + // This value is side-effected. + List unsatisfiedMustCallObligationsOfOwningField = + rlTypeFactory.getMustCallValue(field); + + if (unsatisfiedMustCallObligationsOfOwningField.isEmpty()) { + return; + } + + String error = ""; + Element enclosingElement = field.getEnclosingElement(); + List enclosingMustCallValues = rlTypeFactory.getMustCallValue(enclosingElement); + + if (enclosingMustCallValues == null) { + error = + " The enclosing element " + + ElementUtils.getQualifiedName(enclosingElement) + + " doesn't have a @MustCall annotation"; + } else { + List siblingsOfOwningField = enclosingElement.getEnclosedElements(); + for (Element siblingElement : siblingsOfOwningField) { + if (siblingElement.getKind() == ElementKind.METHOD + && enclosingMustCallValues.contains(siblingElement.getSimpleName().toString())) { + AnnotationMirror ensuresCalledMethodsAnno = + rlTypeFactory.getDeclAnnotation(siblingElement, EnsuresCalledMethods.class); + + if (ensuresCalledMethodsAnno != null) { + List values = + AnnotationUtils.getElementValueArray( + ensuresCalledMethodsAnno, + rlTypeFactory.ensuresCalledMethodsValueElement, + String.class); + for (String value : values) { + if (value.contains(field.getSimpleName().toString())) { + List methods = + AnnotationUtils.getElementValueArray( + ensuresCalledMethodsAnno, + rlTypeFactory.ensuresCalledMethodsMethodsElement, + String.class); + unsatisfiedMustCallObligationsOfOwningField.removeAll(methods); + } + } + if (unsatisfiedMustCallObligationsOfOwningField.isEmpty()) { + return; + } + } + + if (!unsatisfiedMustCallObligationsOfOwningField.isEmpty()) { + // This variable could be set immediately before reporting the error, but IMO + // it is more clear to set it here. + error = + " @EnsuresCalledMethods written on MustCall methods doesn't contain " + + MustCallConsistencyAnalyzer.formatMissingMustCallMethods( + unsatisfiedMustCallObligationsOfOwningField); + } + } + } + } + + if (!unsatisfiedMustCallObligationsOfOwningField.isEmpty()) { + checker.reportError( + field, + "required.method.not.called", + MustCallConsistencyAnalyzer.formatMissingMustCallMethods( + unsatisfiedMustCallObligationsOfOwningField), + field.asType().toString(), + error); + } + } +} diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties b/checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties new file mode 100644 index 00000000000..b9bd04b4afc --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties @@ -0,0 +1,5 @@ +required.method.not.called=@MustCall %s not invoked. The type of object is: %s. Reason for going out of scope: %s +missing.creates.mustcall.for=This method re-assigns the non-final, owning field %s.%s, but does not have a corresponding @CreatesMustCallFor annotation. +incompatible.creates.mustcall.for=This method re-assigns the non-final, owning field %s.%s, but its @CreatesMustCallFor annotation targets %s. +reset.not.owning=Calling this method resets the must-call obligations of the expression %s, which is non-owning. Either annotate its declaration with an @Owning annotation, extract it into a local variable, or write a corresponding @CreatesMustCallFor annotation on the method that encloses this statement. +creates.mustcall.for.override.invalid=Method %s cannot override method %s, which defines fewer @CreatesMustCallFor targets.\nfound: %s\nrequired: %s diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/NoLightweightOwnershipTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/MustCallNoLightweightOwnershipTest.java similarity index 65% rename from checker/src/test/java/org/checkerframework/checker/test/junit/NoLightweightOwnershipTest.java rename to checker/src/test/java/org/checkerframework/checker/test/junit/MustCallNoLightweightOwnershipTest.java index a1190edda53..2faf80941fe 100644 --- a/checker/src/test/java/org/checkerframework/checker/test/junit/NoLightweightOwnershipTest.java +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/MustCallNoLightweightOwnershipTest.java @@ -5,12 +5,12 @@ import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; import org.junit.runners.Parameterized.Parameters; -public class NoLightweightOwnershipTest extends CheckerFrameworkPerDirectoryTest { - public NoLightweightOwnershipTest(List testFiles) { +public class MustCallNoLightweightOwnershipTest extends CheckerFrameworkPerDirectoryTest { + public MustCallNoLightweightOwnershipTest(List testFiles) { super( testFiles, org.checkerframework.checker.mustcall.MustCallChecker.class, - "nolightweightownership", + "mustcall-nolightweightownership", "-Anomsgtext", "-AnoLightweightOwnership", // "-AstubDebug"); @@ -19,6 +19,6 @@ public NoLightweightOwnershipTest(List testFiles) { @Parameters public static String[] getTestDirs() { - return new String[] {"nolightweightownership"}; + return new String[] {"mustcall-nolightweightownership"}; } } diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoCreatesMustCallForTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoCreatesMustCallForTest.java new file mode 100644 index 00000000000..b2c9437700e --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoCreatesMustCallForTest.java @@ -0,0 +1,27 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized.Parameters; + +/** Tests for the Resource Leak Checker. */ +public class ResourceLeakNoCreatesMustCallForTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakNoCreatesMustCallForTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak-nocreatesmustcallfor", + "-Anomsgtext", + "-AnoCreatesMustCallFor", + "-nowarn", + "-encoding", + "UTF-8"); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak-nocreatesmustcallfor"}; + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoLightweightOwnershipTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoLightweightOwnershipTest.java new file mode 100644 index 00000000000..6c40a2aa7f0 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoLightweightOwnershipTest.java @@ -0,0 +1,27 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized.Parameters; + +/** Tests for the Resource Leak Checker. */ +public class ResourceLeakNoLightweightOwnershipTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakNoLightweightOwnershipTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak-nolightweightownership", + "-Anomsgtext", + "-AnoLightweightOwnership", + "-nowarn", + "-encoding", + "UTF-8"); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak-nolightweightownership"}; + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoResourceAliasesTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoResourceAliasesTest.java new file mode 100644 index 00000000000..62c06e8a6f4 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakNoResourceAliasesTest.java @@ -0,0 +1,27 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized.Parameters; + +/** Tests for the Resource Leak Checker. */ +public class ResourceLeakNoResourceAliasesTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakNoResourceAliasesTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak-noresourcealiases", + "-Anomsgtext", + "-AnoResourceAliases", + "-nowarn", + "-encoding", + "UTF-8"); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak-noresourcealiases"}; + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakTest.java new file mode 100644 index 00000000000..5b7fd2a170a --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakTest.java @@ -0,0 +1,26 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized.Parameters; + +/** Tests for the Resource Leak Checker. */ +public class ResourceLeakTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak", + "-Anomsgtext", + "-nowarn", + "-encoding", + "UTF-8"); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak"}; + } +} diff --git a/checker/tests/nolightweightownership/BorrowOnReturn.java b/checker/tests/mustcall-nolightweightownership/BorrowOnReturn.java similarity index 100% rename from checker/tests/nolightweightownership/BorrowOnReturn.java rename to checker/tests/mustcall-nolightweightownership/BorrowOnReturn.java diff --git a/checker/tests/nolightweightownership/NonOwningPolyInteraction.java b/checker/tests/mustcall-nolightweightownership/NonOwningPolyInteraction.java similarity index 100% rename from checker/tests/nolightweightownership/NonOwningPolyInteraction.java rename to checker/tests/mustcall-nolightweightownership/NonOwningPolyInteraction.java diff --git a/checker/tests/nolightweightownership/OwningParams.java b/checker/tests/mustcall-nolightweightownership/OwningParams.java similarity index 100% rename from checker/tests/nolightweightownership/OwningParams.java rename to checker/tests/mustcall-nolightweightownership/OwningParams.java diff --git a/checker/tests/mustcall/SimpleSocketField.java b/checker/tests/mustcall/SimpleSocketField.java new file mode 100644 index 00000000000..8d721e732fd --- /dev/null +++ b/checker/tests/mustcall/SimpleSocketField.java @@ -0,0 +1,21 @@ +// a test that sockets in fields are considered @MustCall("close") + +import java.net.Socket; +import org.checkerframework.checker.mustcall.qual.MustCall; + +class SimpleSocketField { + Socket mySock = new Socket(); + + SimpleSocketField() throws Exception { + @MustCall("close") Socket s = mySock; + // This assignment is safe, because the only possible value of mySock here is the unconnected + // socket in the field initializer. + @MustCall({}) Socket s1 = mySock; + } + + void test() { + @MustCall("close") Socket s = mySock; + // :: error: assignment + @MustCall({}) Socket s1 = mySock; + } +} diff --git a/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingServerSockets.java b/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingServerSockets.java new file mode 100644 index 00000000000..45fc6b5bc11 --- /dev/null +++ b/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingServerSockets.java @@ -0,0 +1,50 @@ +// a set of test cases that demonstrate that errors are actually insued in appropriate +// places when ServerSockets are connected + +// This version of the test expects that the obligation creation support (i.e. the +// @CreatesMustCallFor annotation +// and its accompanying logic) has been disabled. + +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class ConnectingServerSockets { + + static void simple_ss_test(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + ServerSocket s = new ServerSocket(); + s.bind(sa); + } + + static void simple_ss_test2(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + ServerSocket s = new ServerSocket(); + // s.bind(sa); + } + + static void simple_ss_test4(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + ServerSocket s = new ServerSocket(); + s.bind(sa, to); + } + + static @MustCall({}) ServerSocket makeUnconnected() throws Exception { + // :: error: return + return new ServerSocket(); + } + + static void simple_ss_test5(SocketAddress sa) throws Exception { + ServerSocket s = makeUnconnected(); + s.bind(sa); + } + + static void simple_ss_test6(SocketAddress sa) throws Exception { + ServerSocket s = makeUnconnected(); + // s.bind(sa); + } + + static void simple_ss_test8(SocketAddress sa, int to) throws Exception { + ServerSocket s = makeUnconnected(); + s.bind(sa, to); + } +} diff --git a/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingSockets.java b/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingSockets.java new file mode 100644 index 00000000000..4e210dc03e9 --- /dev/null +++ b/checker/tests/resourceleak-nocreatesmustcallfor/ConnectingSockets.java @@ -0,0 +1,57 @@ +// a set of test cases that demonstrate that errors are actually issued in appropriate +// places when Sockets are connected + +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class ConnectingSockets { + + static void simple_ns_test(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.bind(sa); + } + + static void simple_ns_test2(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + // s.bind(sa); + } + + static void simple_ns_test3(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.connect(sa); + } + + static void simple_ns_test4(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.connect(sa, to); + } + + static @MustCall({}) Socket makeUnconnected() throws Exception { + // :: error: return + return new Socket(); + } + + static void simple_ns_test5(SocketAddress sa) throws Exception { + Socket s = makeUnconnected(); + s.bind(sa); + } + + static void simple_ns_test6(SocketAddress sa) throws Exception { + Socket s = makeUnconnected(); + // s.bind(sa); + } + + static void simple_ns_test7(SocketAddress sa) throws Exception { + Socket s = makeUnconnected(); + s.connect(sa); + } + + static void simple_ns_test8(SocketAddress sa, int to) throws Exception { + Socket s = makeUnconnected(); + s.connect(sa, to); + } +} diff --git a/checker/tests/resourceleak-nocreatesmustcallfor/CreatesMustCallForSimpler.java b/checker/tests/resourceleak-nocreatesmustcallfor/CreatesMustCallForSimpler.java new file mode 100644 index 00000000000..f5f8cdb3f78 --- /dev/null +++ b/checker/tests/resourceleak-nocreatesmustcallfor/CreatesMustCallForSimpler.java @@ -0,0 +1,30 @@ +// A simpler test that @CreatesMustCallFor works as intended wrt the Object Construction Checker. + +// This test has been modified to expect that CreatesMustCallFor is feature-flagged to off. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForSimpler { + + @CreatesMustCallFor + void reset() {} + + @CreatesMustCallFor("this") + void resetThis() {} + + void a() {} + + static @MustCall({}) CreatesMustCallForSimpler makeNoMC() { + // :: error: return + return new CreatesMustCallForSimpler(); + } + + static void test1() { + CreatesMustCallForSimpler cos = makeNoMC(); + @MustCall({}) CreatesMustCallForSimpler a = cos; + cos.reset(); + @CalledMethods({"reset"}) CreatesMustCallForSimpler b = cos; + @CalledMethods({}) CreatesMustCallForSimpler c = cos; + } +} diff --git a/checker/tests/resourceleak-nocreatesmustcallfor/SocketContainer.java b/checker/tests/resourceleak-nocreatesmustcallfor/SocketContainer.java new file mode 100644 index 00000000000..de092942148 --- /dev/null +++ b/checker/tests/resourceleak-nocreatesmustcallfor/SocketContainer.java @@ -0,0 +1,33 @@ +// A simple class that has a Socket as an owning field. +// This is a modified version of tests/socket/SocketContainer.java +// for checking that without CO support we can't assign to non-final owning fields at all. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("close") class SocketContainer { + @Owning Socket sock; + + public SocketContainer(String host, int port) throws Exception { + // Assignments to owning fields should not be permitted. + // :: error: required.method.not.called + sock = new Socket(host, port); + } + + // No missing create obligation error is issued, since CO is disabled... + public void reassign(String host, int port) throws Exception { + sock.close(); + // For the RHS, because the field can't take ownership + // :: error: required.method.not.called + Socket sr = new Socket(host, port); + // No warning for overwriting the field, since it can't take ownership! + sock = sr; + } + + @EnsuresCalledMethods(value = "this.sock", methods = "close") + public void close() throws IOException { + sock.close(); + } +} diff --git a/checker/tests/resourceleak-nolightweightownership/ACOwning.java b/checker/tests/resourceleak-nolightweightownership/ACOwning.java new file mode 100644 index 00000000000..1af042f9cf8 --- /dev/null +++ b/checker/tests/resourceleak-nolightweightownership/ACOwning.java @@ -0,0 +1,59 @@ +// This copy of the ACOwning.java test from the mustcall tests +// has expected errors as if ownership transfer can only happen based +// on defaults - that is, that @Owning and @NotOwning annotations are +// ignored. + +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ACOwning { + + @MustCall("a") static class Foo { + void a() {} + } + + Foo makeFoo() { + return new Foo(); + } + + static void takeOwnership(@Owning Foo foo) { + foo.a(); + } + + static void noOwnership(Foo foo) {} + + static void takeOwnershipWrong(@Owning Foo foo) {} + + static @NotOwning Foo getNonOwningFoo() { + return new Foo(); + } + + static void callGetNonOwningFoo() { + // :: error: required.method.not.called + getNonOwningFoo(); + } + + static void ownershipInCallee() { + // :: error: required.method.not.called + Foo f = new Foo(); + takeOwnership(f); + // :: error: required.method.not.called + Foo g = new Foo(); + noOwnership(g); + } + + @Owning + public Foo owningAtReturn() { + return new Foo(); + } + + void owningAtReturnTest() { + // :: error: required.method.not.called + Foo f = owningAtReturn(); + } + + void ownershipTest() { + // :: error: required.method.not.called + takeOwnership(new Foo()); + } +} diff --git a/checker/tests/resourceleak-noresourcealiases/MustCallAliasExamples.java b/checker/tests/resourceleak-noresourcealiases/MustCallAliasExamples.java new file mode 100644 index 00000000000..e7f65924150 --- /dev/null +++ b/checker/tests/resourceleak-noresourcealiases/MustCallAliasExamples.java @@ -0,0 +1,62 @@ +// Simple tests of @MustCallAlias functionality on wrapper streams. +// This version has been modified to expect that @MustCallAlias annotations +// are always ignored, as if running with -AnoResourceAliases. + +import java.io.*; +import java.io.IOException; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasExamples { + + void test_two_locals(String address) { + Socket socket = null; + try { + socket = new Socket(address, 8000); + // :: error: required.method.not.called + DataInputStream d = new DataInputStream(socket.getInputStream()); + } catch (IOException e) { + + } finally { + closeSocket(socket); + } + } + + // :: error: required.method.not.called + void test_close_wrapper(@Owning InputStream b) throws IOException { + DataInputStream d = new DataInputStream(b); + d.close(); + } + + void test_close_nonwrapper(@Owning InputStream b) throws IOException { + // :: error: required.method.not.called + DataInputStream d = new DataInputStream(b); + b.close(); + } + + // :: error: required.method.not.called + void test_no_close(@Owning InputStream b) { + // :: error: required.method.not.called + DataInputStream d = new DataInputStream(b); + } + + // :: error: required.method.not.called + void test_no_assign(@Owning InputStream b) { + // :: error: required.method.not.called + new DataInputStream( + // :: error: required.method.not.called + new BufferedInputStream(b)); + } + + @EnsuresCalledMethods(value = "#1", methods = "close") + void closeSocket(Socket sock) { + try { + if (sock != null) { + sock.close(); + } + } catch (IOException e) { + + } + } +} diff --git a/checker/tests/resourceleak-noresourcealiases/MustCallAliasPassthroughLocal.java b/checker/tests/resourceleak-noresourcealiases/MustCallAliasPassthroughLocal.java new file mode 100644 index 00000000000..fdba664ba89 --- /dev/null +++ b/checker/tests/resourceleak-noresourcealiases/MustCallAliasPassthroughLocal.java @@ -0,0 +1,25 @@ +// A test that passing a local to an MCA super constructor is allowed. +// This version has been modified to expect errors, as if running under +// -AnoResourceAliases - so @MustCallAlias annotations are ignored. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughLocal extends FilterInputStream { + MustCallAliasPassthroughLocal(File f) throws Exception { + // This is safe - this MCA constructor of FilterInputStream means that the result of this + // constructor - i.e. the caller - is taking ownership of this newly-created output stream. + // :: error: required.method.not.called + super(new FileInputStream(f)); + } + + static void test(File f) throws Exception { + // :: error: required.method.not.called + new MustCallAliasPassthroughLocal(f); + } + + static void test_ok(File f) throws Exception { + new MustCallAliasPassthroughLocal(f).close(); + } +} diff --git a/checker/tests/resourceleak/ACExceptionalExitPointTest.java b/checker/tests/resourceleak/ACExceptionalExitPointTest.java new file mode 100644 index 00000000000..190c505e7f0 --- /dev/null +++ b/checker/tests/resourceleak/ACExceptionalExitPointTest.java @@ -0,0 +1,38 @@ +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ACExceptionalExitPointTest { + + @MustCall("a") class Foo { + void a() {} + + @This Foo b() { + return this; + } + + void c() {} + } + + Foo makeFoo() { + return new Foo(); + } + + @CalledMethods({"a"}) Foo makeFoo2() { + Foo f = new Foo(); + f.a(); + return f; + } + + void exceptionalExitWrong() throws Exception { + // :: error: required.method.not.called + Foo fw = makeFoo(); + throw new Exception(); + } + + void exceptionalExitCorrect() throws Exception { + Foo fw = new Foo(); + fw.a(); + throw new Exception(); + } +} diff --git a/checker/tests/resourceleak/ACMethodInvocationTest.java b/checker/tests/resourceleak/ACMethodInvocationTest.java new file mode 100644 index 00000000000..9ca6929a0b5 --- /dev/null +++ b/checker/tests/resourceleak/ACMethodInvocationTest.java @@ -0,0 +1,97 @@ +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ACMethodInvocationTest { + + @MustCall("a") class Foo { + void a() {} + + @This Foo b() { + return this; + } + + void c() {} + } + + @Owning + Foo makeFoo() { + return new Foo(); + } + + @CalledMethods({"a"}) Foo makeFooFinalize() { + Foo f = new Foo(); + f.a(); + return f; + } + + @Owning + @CalledMethods({"b"}) Foo makeFooFinalize2() { + Foo f = new Foo(); + f.b(); + return f; + } + + void CallMethodsInSequence() { + makeFoo().a(); + } + + void CallMethodsInSequence2() { + makeFoo().b().a(); + } + + void testFluentAPIWrong() { + // :: error: required.method.not.called + makeFoo().b(); + } + + void testFluentAPIWrong2() { + // :: error: required.method.not.called + makeFoo(); + } + + void invokeMethodWithCallA() { + makeFooFinalize(); + } + + void invokeMethodWithCallBWrong() { + // :: error: required.method.not.called + makeFooFinalize2(); + } + + void invokeMethodAndCallCWrong() { + // :: error: required.method.not.called + makeFoo().c(); + } + + Foo returnMakeFoo() { + return makeFoo(); + } + + Foo testField1; + Foo testField2; + Foo testField3; + + void testStoringInField() { + // :: error: required.method.not.called + testField1 = makeFoo(); + // :: error: required.method.not.called + testField2 = new Foo(); + + testField3 = makeFooFinalize(); + } + + void tryCatchFinally() { + Foo f = null; + try { + f = new Foo(); + try { + throw new RuntimeException(); + } catch (Exception e) { + + } + } finally { + f.a(); + } + } +} diff --git a/checker/tests/resourceleak/ACOwning.java b/checker/tests/resourceleak/ACOwning.java new file mode 100644 index 00000000000..848be301a2a --- /dev/null +++ b/checker/tests/resourceleak/ACOwning.java @@ -0,0 +1,80 @@ +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ACOwning { + + @MustCall("a") static class Foo { + void a() {} + } + + Foo makeFoo() { + return new Foo(); + } + + static void takeOwnership(@Owning Foo foo, Foo f) { + foo.a(); + } + + static void noOwnership(Foo foo) {} + + // :: error: required.method.not.called + static void takeOwnershipWrong(@Owning Foo foo) {} + + static @NotOwning Foo getNonOwningFoo() { + // :: error: required.method.not.called + return new Foo(); + } + + static void callGetNonOwningFoo() { + getNonOwningFoo(); + } + + static void ownershipInCallee() { + Foo f = new Foo(); + // :: error: required.method.not.called + takeOwnership(f, new Foo()); + // :: error: required.method.not.called + Foo g = new Foo(); + noOwnership(g); + } + + // make sure enum doesn't crash things + static enum TestEnum { + CASE1, + CASE2, + CASE3 + } + + @Owning + public Foo owningAtReturn() { + return new Foo(); + } + + void owningAtReturnTest() { + // :: error: required.method.not.called + Foo f = owningAtReturn(); + } + + void ownershipTest() { + // :: error: required.method.not.called + takeOwnership(new Foo(), makeFoo()); + } + + @MustCall({}) + // :: error: super.invocation + private class SubFoo extends Foo { + + void test() { + SubFoo f = new SubFoo(); + } + + void test2() { + // :: error: required.method.not.called + Foo f = new Foo(); + } + + void test3() { + Foo f = new SubFoo(); + } + } +} diff --git a/checker/tests/resourceleak/ACRegularExitPointTest.java b/checker/tests/resourceleak/ACRegularExitPointTest.java new file mode 100644 index 00000000000..a7bb586312f --- /dev/null +++ b/checker/tests/resourceleak/ACRegularExitPointTest.java @@ -0,0 +1,400 @@ +import java.io.IOException; +import java.util.function.Function; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ACRegularExitPointTest { + + @MustCall("a") class Foo { + void a() {} + + @This Foo b() { + return this; + } + + void c(@CalledMethods("a") Foo this) {} + } + + @MustCall("a") class SubFoo extends Foo {} + + Foo makeFoo() { + return new Foo(); + } + + @CalledMethods("a") Foo makeFooCallA() { + Foo f = new Foo(); + f.a(); + return f; + } + + @EnsuresCalledMethods(value = "#1", methods = "a") + void callA(Foo f) { + f.a(); + } + + void makeFooFinalize() { + Foo f = new Foo(); + f.a(); + } + + void makeFooFinalizeWrong() { + Foo m; + // :: error: required.method.not.called + m = new Foo(); + // :: error: required.method.not.called + Foo f = new Foo(); + f.b(); + } + + void testStoringInLocalWrong() { + // :: error: required.method.not.called + Foo foo = makeFoo(); + } + + void testStoringInLocalWrong2() { + Foo f; + // :: error: required.method.not.called + f = makeFoo(); + } + + void testStoringInLocal() { + Foo foo = makeFooCallA(); + } + + void testStoringInLocalWrong3() { + // :: error: required.method.not.called + Foo foo = new Foo(); + } + + void emptyFuncWithFormalPram(Foo f) {} + + void innerFunc(Foo f) { + Runnable r = + new Runnable() { + public void run() { + Foo f; + } + ; + }; + r.run(); + } + + void innerFuncWrong(Foo f) { + Runnable r = + new Runnable() { + public void run() { + // :: error: required.method.not.called + Foo g = new Foo(); + } + ; + }; + r.run(); + } + + void innerFunc2(Foo f) { + Runnable r = + new Runnable() { + public void run() { + Foo g = makeFoo(); + g.a(); + } + ; + }; + r.run(); + } + + void innerfunc3() { + + Foo f = makeFoo(); + f.a(); + Function innerfunc = + st -> { + // :: error: required.method.not.called + Foo fn1 = new Foo(); + Foo fn2 = makeFoo(); + fn2.a(); + return fn2; + }; + + innerfunc.apply(f); + } + + void ifElse(boolean b) { + if (b) { + Foo f1 = new Foo(); + f1.a(); + } else { + // :: error: required.method.not.called + Foo f2 = new Foo(); + } + } + + Foo ifElseWithReturnExit(boolean b, boolean c) { + // :: error: required.method.not.called + Foo f1 = makeFoo(); + // :: error: required.method.not.called + Foo f3 = new Foo(); + // :: error: required.method.not.called + Foo f4 = new Foo(); + + if (b) { + // :: error: required.method.not.called + Foo f2 = new Foo(); + if (c) { + f4.a(); + } else { + f4.b(); + } + return f1; + } else { + // :: error: required.method.not.called + Foo f2 = new Foo(); + f2 = new Foo(); + f2.a(); + } + return f3; + } + + void ifElseWithDeclaration(boolean b) { + Foo f1; + Foo f2; + if (b) { + f1 = new Foo(); + f1.a(); + } else { + // :: error: required.method.not.called + f2 = new Foo(); + } + } + + void ifElseWithInitialization(boolean b) { + // :: error: required.method.not.called + Foo f2 = new Foo(); + Foo f11 = null; + if (b) { + f11 = makeFoo(); + f11.a(); + } else { + // :: error: required.method.not.called + f2 = new Foo(); + } + } + + void ifWithInitialization(boolean b) { + // :: error: required.method.not.called + Foo f1 = new Foo(); + // :: error: required.method.not.called + Foo f2 = new Foo(); + if (b) { + f1.a(); + } + } + + void variableGoesOutOfScope(boolean b) { + if (b) { + Foo f1 = new Foo(); + f1.a(); + } + } + + void ifWithNullInitialization(boolean b) { + Foo f1 = null; + Foo f2 = null; + if (b) { + f1 = new Foo(); + f1.a(); + } else { + // :: error: required.method.not.called + f2 = new Foo(); + } + } + + void variableInitializedWithNull() { + Foo f = null; + } + + void testLoop() { + Foo f = null; + while (true) { + // :: error: required.method.not.called + f = new Foo(); + } + } + + void overWrittingVarInLoop() { + // :: error: required.method.not.called + Foo f = new Foo(); + while (true) { + // :: error: required.method.not.called + f = new Foo(); + } + } + + void loopWithNestedBranches(boolean b) { + Foo frodo = null; + while (true) { + if (b) { + // :: error: required.method.not.called + frodo = new Foo(); + } else { + // this is a known false positive, due to lack of path sensitivity in the + // Called Methods Checker + // :: error: required.method.not.called + frodo = new Foo(); + frodo.a(); + } + } + } + + void replaceVarWithNull(boolean b, boolean c) { + // :: error: required.method.not.called + Foo f = new Foo(); + if (b) { + f = null; + } else if (c) { + f = null; + } else { + + } + } + + void ownershipTransfer() { + Foo f1 = new Foo(); + Foo f2 = f1; + Foo f3 = f2.b(); + f3.a(); + } + + void ownershipTransfer2() { + Foo f1 = null; + Foo f2 = f1; + } + + void testECM() { + Foo f = new Foo(); + callA(f); + } + + void testFinallyBlock(boolean b) { + Foo f = null; + try { + f = new Foo(); + if (true) { + throw new IOException(); + } + } catch (IOException e) { + + } finally { + f.a(); + } + } + + void testSubFoo() { + // :: error: required.method.not.called + Foo f = new SubFoo(); + } + + void testSubFoo2() { + // :: error: required.method.not.called + SubFoo f = new SubFoo(); + } + + static void takeOwnership(@Owning Foo foo) { + foo.a(); + } + + /** cases where ternary expressions are assigned to a variable */ + void testTernaryAssigned(boolean b) { + Foo ternary1 = b ? new Foo() : makeFoo(); + ternary1.a(); + + // :: error: required.method.not.called + Foo ternary2 = b ? new Foo() : makeFoo(); + + // :: error: required.method.not.called + Foo x = new Foo(); + Foo ternary3 = b ? new Foo() : x; + ternary3.a(); + + Foo y = new Foo(); + Foo ternary4 = b ? y : y; + ternary4.a(); + + takeOwnership(b ? new Foo() : makeFoo()); + + // :: error: required.method.not.called + Foo x2 = new Foo(); + takeOwnership(b ? x2 : null); + + int i = 10; + Foo ternaryInLoop = null; + while (i > 0) { + // :: error: required.method.not.called + ternaryInLoop = b ? null : new Foo(); + i--; + } + ternaryInLoop.a(); + + (b ? new Foo() : makeFoo()).a(); + } + + /** + * tests where ternary and cast expressions (possibly nested) may or may not be assigned to a + * variable + */ + void testTernaryCastUnassigned(boolean b) { + // :: error: required.method.not.called + if ((b ? new Foo() : null) != null) { + b = !b; + } + + // :: error: required.method.not.called + if ((b ? makeFoo() : null) != null) { + b = !b; + } + + Foo x = new Foo(); + if ((b ? x : null) != null) { + b = !b; + } + x.a(); + + // :: error: required.method.not.called + if (((Foo) new Foo()) != null) { + b = !b; + } + + // double cast; no error + Foo doubleCast = (Foo) ((Foo) makeFoo()); + doubleCast.a(); + + // nesting casts and ternary expressions; no error + Foo deepNesting = (b ? (!b ? makeFoo() : (Foo) makeFoo()) : ((Foo) new Foo())); + deepNesting.a(); + } + + @Owning + Foo testTernaryReturnOk(boolean b) { + return b ? new Foo() : makeFoo(); + } + + @Owning + Foo testTernaryReturnBad(boolean b) { + // :: error: required.method.not.called + Foo x = new Foo(); + return b ? x : makeFoo(); + } + + @MustCall("toString") static class Sub1 extends Object {} + + @MustCall("clone") static class Sub2 extends Object {} + + static void test_ternary(boolean b) { + // :: error: required.method.not.called + Object toStringAndClone = b ? new Sub1() : new Sub2(); + // at this point, for soundness, we should be responsible for calling both toString and clone on + // obj... + toStringAndClone.toString(); + } +} diff --git a/checker/tests/resourceleak/ACSocketTest.java b/checker/tests/resourceleak/ACSocketTest.java new file mode 100644 index 00000000000..c541341b7ac --- /dev/null +++ b/checker/tests/resourceleak/ACSocketTest.java @@ -0,0 +1,438 @@ +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.PrintStream; +import java.net.*; +import java.nio.channels.*; +import java.util.*; +import java.util.concurrent.atomic.AtomicReference; +import javax.net.ssl.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +public class ACSocketTest { + + @Owning + Socket makeSocket(String address, int port) { + + try { + Socket socket = new Socket(address, port); + return socket; + } catch (IOException i) { + return null; + } + } + + void basicTest(String address, int port) { + try { + // :: error: required.method.not.called + Socket socket2 = new Socket(address, port); + Socket specialSocket = new Socket(address, port); + specialSocket.close(); + } catch (IOException i) { + } + } + + void tryWithResourcesTest(String address, int port) throws IOException { + try (Socket s = new Socket(address, port)) {} + } + + void callMakeSocketAndClose(String address, int port) { + Socket socket = makeSocket(address, port); + try { + socket.close(); + } catch (IOException i) { + } + } + + void callMakeSocket(String address, int port) { + // :: error: required.method.not.called + Socket socket = makeSocket(address, port); + } + + void ifElseWithDeclaration(String address, int port, boolean b) { + Socket s1; + Socket s2; + try { + if (b) { + s1 = new Socket(address, port); + s1.close(); + } else { + // :: error: required.method.not.called + s2 = new Socket(address, port + 1); + } + } catch (IOException i) { + + } + } + + void testLoop(String address, int port) { + Socket s = null; + while (true) { + try { + s = new Socket(address, port); + s.close(); + } catch (IOException e) { + + } + } + } + + void overWrittingVarInLoop(String address, int port) { + // :: error: required.method.not.called + Socket s = makeSocket(address, port); + while (true) { + try { + // :: error: required.method.not.called + s = new Socket(address, port); + } catch (IOException e) { + + } + } + } + + void loopWithNestedBranches(String address, int port, boolean b) { + Socket s = null; + while (true) { + if (b) { + // :: error: required.method.not.called + s = makeSocket(address, port); + } else { + // :: error: required.method.not.called + s = makeSocket(address, port); + } + } + } + + void replaceVarWithNull(String address, int port, boolean b, boolean c) { + Socket s; + try { + // :: error: required.method.not.called + s = new Socket(address, port); + } catch (IOException e) { + + } + if (b) { + s = null; + } else if (c) { + s = null; + } else { + + } + } + + void ownershipTransfer(String address, int port) { + Socket s1 = null; + try { + // :: error: required.method.not.called + s1 = new Socket(address, port); + } catch (IOException e) { + + } + // It is equally correct to report an error here. + Socket s2 = s1; + if (true) { + closeSocket(s2); + } + } + + void test(String address, int port) { + try { + // :: error: required.method.not.called + Socket socket = new Socket(address, 80); + PrintStream out = new PrintStream(socket.getOutputStream()); + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + in.close(); + } catch (Exception e) { + e.printStackTrace(); + } + } + + protected Socket sock; + + void connectToLeader(AtomicReference socket) throws IOException { + // :: error: required.method.not.called + if (socket.get() == null) { + throw new IOException("Failed connect to "); + } else { + // :: error: required.method.not.called + sock = socket.get(); + } + } + + Socket createSocket(boolean b, String address, int port) throws IOException { + Socket sock; + if (b) { + // :: error: required.method.not.called + sock = new Socket(address, port); + } else { + // :: error: required.method.not.called + sock = new Socket(address, port); + } + + sock.setSoTimeout(10000); + closeSocket(sock); + return sock; + } + + // @EnsuresCalledMethodsIf(expression = "#1", methods = {"close"}, result = true) + // void closeSocket(Socket sock) { + //// if (sock == null) { + //// return; + //// } + // + // try { + // sock.close(); + // } catch (IOException ie) { + // + // } + // } + + public static void ruok(String host, int port) { + Socket s = null; + try { + s = new Socket(host, port); + } catch (IOException e) { + + } finally { + + try { + s.close(); + } catch (IOException e) { + + } + } + } + + @EnsuresCalledMethods(value = "#1", methods = "close") + void closeSocket(Socket sock) { + try { + if (sock != null) { + sock.close(); + } + } catch (IOException e) { + + } + } + + @EnsuresCalledMethods(value = "#1", methods = "close") + void closeServerSocket(ServerSocket sock) { + try { + if (sock != null) { + sock.close(); + } + } catch (IOException e) { + + } + } + + void useCloseSocket(String address, int port) throws IOException { + Socket sock = new Socket(address, port); + Socket s = getSocket(sock); + closeSocket(sock); + } + + void setSockOpts(Socket sock) throws SocketException { + sock.setTcpNoDelay(true); + sock.setKeepAlive(true); + sock.setSoTimeout(1000); + } + + void initiateConnection(SocketAddress endpoint, int timeout, SSLContext context, final Long sid) { + Socket sock = null; + try { + sock = context.getSocketFactory().createSocket(); + setSockOpts(sock); + sock.connect(endpoint, timeout); + if (sock instanceof SSLSocket) { + SSLSocket sslSock = (SSLSocket) sock; + sslSock.startHandshake(); + } + } catch (ClassCastException e) { + closeSocket(sock); + return; + } catch (IOException e) { + closeSocket(sock); + return; + } + + try { + startConnection(sock); + } catch (IOException e) { + closeSocket(sock); + } + } + + private boolean startConnection(@Owning Socket s) throws IOException { + closeSocket(s); + return true; + } + + private boolean startConnection(@Owning SSLSocket s) throws IOException { + closeSocket(s); + return true; + } + + @MustCall({"close"}) class PrependableSocket extends Socket { + + public PrependableSocket(SocketImpl base) throws IOException { + super(base); + } + } + + void makePrependableSocket() throws IOException { + // :: error: required.method.not.called + final PrependableSocket prependableSocket = new PrependableSocket(null); + } + + // private void acceptConnections() { + // int numRetries = 0; + // Socket client = null; + // + // while ((!shutdown) && (portBindMaxRetry == 0 || numRetries < portBindMaxRetry)) { + // try { + // serverSocket = createNewServerSocket(); + // LOG.info("{} is accepting connections now, my election bind port: {}", + // QuorumCnxManager.this.mySid, address.toString()); + // while (!shutdown) { + // try { + // client = serverSocket.accept(); + // setSockOpts(client); + // LOG.info("Received connection request from {}", + // client.getRemoteSocketAddress()); + // // Receive and handle the connection request + // // asynchronously if the quorum sasl authentication is + // // enabled. This is required because sasl server + // // authentication process may take few seconds to finish, + // // this may delay next peer connection requests. + // if (quorumSaslAuthEnabled) { + // receiveConnectionAsync(client); + // } else { + // receiveConnection(client); + // } + // numRetries = 0; + // } catch (SocketTimeoutException e) { + // LOG.warn("The socket is listening for the election accepted " + // + "and it timed out unexpectedly, but will retry." + // + "see ZOOKEEPER-2836"); + // } + // } + // } catch (IOException e) { + // if (shutdown) { + // break; + // } + // + // LOG.error("Exception while listening", e); + // + // if (e instanceof SocketException) { + // socketException.set(true); + // } + // + // numRetries++; + // try { + // close(); + // Thread.sleep(1000); + // } catch (IOException ie) { + // LOG.error("Error closing server socket", ie); + // } catch (InterruptedException ie) { + // LOG.error("Interrupted while sleeping. Ignoring exception", ie); + // } + // closeSocket(client); + // } + // } + // if (!shutdown) { + // LOG.error( + // "Leaving listener thread for address {} after {} errors. Use {} property to + // increase retry count.", + // formatInetAddr(address), + // numRetries, + // ELECTION_PORT_BIND_RETRY); + // } + // } + + void createNewServerSocket(InetSocketAddress address, boolean b, boolean c) throws IOException { + ServerSocket socket; + + if (b) { + socket = new ServerSocket(); + } else if (c) { + socket = new ServerSocket(); + } else { + socket = new ServerSocket(); + } + + socket.setReuseAddress(true); + socket.bind(address); + closeServerSocket(socket); + } + + @Owning + public SSLServerSocket createSSLServerSocket(SSLContext sslContext) throws IOException { + SSLServerSocket sslServerSocket = + (SSLServerSocket) sslContext.getServerSocketFactory().createServerSocket(); + return configureSSLServerSocket(sslServerSocket); + } + + private SSLServerSocket nonOwningSSField; + + void assignToNonOwningViaCast(SSLContext sslContext) throws IOException { + nonOwningSSField = (SSLServerSocket) sslContext.getServerSocketFactory().createServerSocket(); + } + + private SSLServerSocket configureSSLServerSocket(@Owning SSLServerSocket socket) { + return socket; + } + + public SSLSocket createSSLSocket( + @Owning Socket socket, byte[] pushbackBytes, SSLContext sslContext) throws IOException { + SSLSocket sslSocket; + if (pushbackBytes != null && pushbackBytes.length > 0) { + sslSocket = + (SSLSocket) + sslContext.getSocketFactory().createSocket(socket, null, socket.getPort(), true); + } else { + sslSocket = + (SSLSocket) + sslContext.getSocketFactory().createSocket(socket, null, socket.getPort(), true); + } + return configureSSLSocket(sslSocket, false); + } + + private SSLSocket configureSSLSocket(@Owning SSLSocket socket, boolean isClientSocket) { + SSLParameters sslParameters = socket.getSSLParameters(); + // configureSslParameters(sslParameters, isClientSocket); + socket.setSSLParameters(sslParameters); + socket.setUseClientMode(isClientSocket); + return socket; + } + + private void updateSocketAddresses(SelectionKey sockKey) { + // no error here as SelectionKey.channel()'s return is @NotOwning + Socket socket = ((SocketChannel) sockKey.channel()).socket(); + SocketAddress localSocketAddress = socket.getLocalSocketAddress(); + SocketAddress remoteSocketAddress = socket.getRemoteSocketAddress(); + } + + private void recieverParameterWithCasting(@Owning SelectableChannel channel1) throws IOException { + try { + ((SocketChannel) channel1).socket(); + } finally { + channel1.close(); + } + } + + @NotOwning + Socket getSocket(Socket s) { + return s; + } + + private ServerSocket testMCAParamInReturn() throws IOException { + ServerSocketChannel chan = ServerSocketChannel.open(); + return chan.socket(); + } + + private void testMCAParamInReturn2() throws IOException { + ServerSocket chan = ServerSocketChannel.open().socket(); + } +} diff --git a/checker/tests/resourceleak/BindChannel.java b/checker/tests/resourceleak/BindChannel.java new file mode 100644 index 00000000000..a43e1cb008c --- /dev/null +++ b/checker/tests/resourceleak/BindChannel.java @@ -0,0 +1,40 @@ +// A test for code encountered by Narges. + +import java.io.*; +import java.net.*; +import java.nio.channels.*; + +class BindChannel { + static void test(InetSocketAddress addr, boolean b) { + try { + // This channel is bound - so even with unconnected socket support, we need to + // treat either this channel or the .socket() expression as must-close. + // + // Even though there's now a temporary in the Must Call Checker for the value that + // has the reset method (bind) called on it below, we can't successfully translate + // the reset expression to that temporary, since all we have is a string (from the + // reset annotation) and so we have to go through the type factory's parsing facility, + // which doesn't know about the temporaries and so doesn't return them. We're therefore + // limited to issuing the reset.not.owning error below, + // instead of the preferable required.method.not.called error on this line - as in + // the method below, which extracts the socket into a local variable, which can be + // parsed as an CO target. + ServerSocketChannel httpChannel = ServerSocketChannel.open(); + // :: error: reset.not.owning + httpChannel.socket().bind(addr); + } catch (IOException io) { + + } + } + + static void test_lv(InetSocketAddress addr, boolean b) { + try { + ServerSocketChannel httpChannel = ServerSocketChannel.open(); + // :: error: required.method.not.called + ServerSocket httpSock = httpChannel.socket(); + httpSock.bind(addr); + } catch (IOException io) { + + } + } +} diff --git a/checker/tests/resourceleak/COAnonymousClass.java b/checker/tests/resourceleak/COAnonymousClass.java new file mode 100644 index 00000000000..70de3b47bfa --- /dev/null +++ b/checker/tests/resourceleak/COAnonymousClass.java @@ -0,0 +1,53 @@ +// Test case for https://github.com/kelloggm/object-construction-checker/issues/368 + +import org.checkerframework.checker.mustcall.qual.*; + +class COAnonymousClass { + static class Foo { + + @CreatesMustCallFor("this") + void resetFoo() {} + + void other() { + + Runnable r = + new Runnable() { + @Override + @CreatesMustCallFor("Foo.this") + // :: error: creates.mustcall.for.override.invalid + public void run() { + // Ideally, we would not issue the following error. However, the Checker Framework's + // JavaExpression support + // (https://checkerframework.org/manual/#java-expressions-as-arguments) + // treats all versions of "this" (including "Foo.this") as referring to the object + // that directly contains the annotation, so we treat this call to resetFoo as not + // permitted. + // :: error: reset.not.owning + resetFoo(); + } + }; + call_run(r); + } + + void other2() { + + Runnable r = + new Runnable() { + @Override + @CreatesMustCallFor("this") + // :: error: creates.mustcall.for.override.invalid + public void run() { + // This error definitely must be issued, since Foo.this != this. + // :: error: reset.not.owning + resetFoo(); + } + }; + call_run(r); + } + + // If this call to run() were permitted with no errors, this would be unsound. + void call_run(Runnable r) { + r.run(); + } + } +} diff --git a/checker/tests/resourceleak/COInSubtype.java b/checker/tests/resourceleak/COInSubtype.java new file mode 100644 index 00000000000..0a238eefd34 --- /dev/null +++ b/checker/tests/resourceleak/COInSubtype.java @@ -0,0 +1,23 @@ +// A test for a bad interaction between CO and subtyping +// that could happen if CO was unsound. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class COInSubtype { + static class Foo { + + @CreatesMustCallFor("this") + void resetFoo() {} + } + + @MustCall("a") static class Bar extends Foo { + void a() {} + } + + static void test() { + // :: error: required.method.not.called + @MustCall("a") Foo f = new Bar(); + f.resetFoo(); + } +} diff --git a/checker/tests/resourceleak/CheckFields.java b/checker/tests/resourceleak/CheckFields.java new file mode 100644 index 00000000000..995f4d99737 --- /dev/null +++ b/checker/tests/resourceleak/CheckFields.java @@ -0,0 +1,166 @@ +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class CheckFields { + + @MustCall("a") static class Foo { + void a() {} + + void c() {} + } + + Foo makeFoo() { + return new Foo(); + } + + @MustCall("b") static class FooField { + private final @Owning Foo finalOwningFoo; + // :: error: required.method.not.called + private final @Owning Foo finalOwningFooWrong; + private final Foo finalNotOwningFoo; + private @Owning Foo owningFoo; + private @Owning @MustCall({}) Foo owningEmptyMustCallFoo; + private Foo notOwningFoo; + + public FooField() { + this.finalOwningFoo = new Foo(); + this.finalOwningFooWrong = new Foo(); + // :: error: required.method.not.called + this.finalNotOwningFoo = new Foo(); + } + + @CreatesMustCallFor + void assingToOwningFieldWrong() { + Foo f = new Foo(); + // :: error: required.method.not.called + this.owningFoo = f; + } + + @CreatesMustCallFor + void assignToOwningFieldWrong2() { + // :: error: required.method.not.called + this.owningFoo = new Foo(); + } + + @CreatesMustCallFor + void assingToOwningField() { + // this is a safe re-assignment. + if (this.owningFoo == null) { + Foo f = new Foo(); + this.owningFoo = f; + } + } + + void assingToFinalNotOwningField() { + // :: error: required.method.not.called + Foo f = new Foo(); + this.notOwningFoo = f; + } + + Foo getOwningFoo() { + return this.owningFoo; + } + + @EnsuresCalledMethods( + value = {"this.finalOwningFoo", "this.owningFoo"}, + methods = {"a"}) + void b() { + this.finalOwningFoo.a(); + this.finalOwningFoo.c(); + this.owningFoo.a(); + } + } + + void testField() { + FooField fooField = new FooField(); + fooField.b(); + } + + void testAccessField() { + FooField fooField = new FooField(); + // :: error: required.method.not.called + fooField.owningFoo = new Foo(); + fooField.b(); + } + + void testAccessField2() { + FooField fooField = new FooField(); + if (fooField.owningFoo == null) { + fooField.owningFoo = new Foo(); + } + fooField.b(); + } + + void testAccessFieldWrong() { + // :: error: required.method.not.called + FooField fooField = new FooField(); + // :: error: required.method.not.called + fooField.owningFoo = new Foo(); + // :: error: required.method.not.called + fooField.notOwningFoo = new Foo(); + } + + @CreatesMustCallFor("#1") + void testAccessField_param(FooField fooField) { + // :: error: required.method.not.called + fooField.owningFoo = new Foo(); + fooField.b(); + } + + // :: error: missing.creates.mustcall.for + void testAccessField_param_no_co(FooField fooField) { + // :: error: required.method.not.called + fooField.owningFoo = new Foo(); + fooField.b(); + } + + static class NestedWrong { + + // Non-final owning fields also require the surrounding class to have an appropriate MC + // annotation. + // :: error: required.method.not.called + @Owning Foo foo; + + @CreatesMustCallFor("this") + void initFoo() { + if (this.foo == null) { + this.foo = new Foo(); + } + } + } + + @MustCall("f") static class NestedWrong2 { + // Non-final owning fields also require the surrounding class to have an appropriate MC + // annotation. + // :: error: required.method.not.called + @Owning Foo foo; + + @CreatesMustCallFor("this") + void initFoo() { + if (this.foo == null) { + this.foo = new Foo(); + } + } + + void f() {} + } + + @MustCall("f") static class NestedRight { + // Non-final owning fields also require the surrounding class to have an appropriate MC + // annotation. + @Owning Foo foo; + + @CreatesMustCallFor("this") + void initFoo() { + if (this.foo == null) { + this.foo = new Foo(); + } + } + + @EnsuresCalledMethods(value = "this.foo", methods = "a") + void f() { + this.foo.a(); + } + } +} diff --git a/checker/tests/resourceleak/CloseableAndMore.java b/checker/tests/resourceleak/CloseableAndMore.java new file mode 100644 index 00000000000..5bd1addd248 --- /dev/null +++ b/checker/tests/resourceleak/CloseableAndMore.java @@ -0,0 +1,28 @@ +// A test that when a class implements autocloseable and has another must-call obligation, +// errors are still issued about the other obligation even when it used as a resource variable. + +import java.io.IOException; +import org.checkerframework.checker.mustcall.qual.InheritableMustCall; + +@InheritableMustCall({"close", "foo"}) +public class CloseableAndMore implements AutoCloseable { + void foo() {} + + @Override + public void close() throws IOException {} + + public static void test_bad() { + // :: error: required.method.not.called + try (CloseableAndMore c = new CloseableAndMore()) { + + } catch (Exception e) { + } + } + + public static void test_good() { + try (CloseableAndMore c = new CloseableAndMore()) { + c.foo(); + } catch (Exception e) { + } + } +} diff --git a/checker/tests/resourceleak/CommonModuleCrash.java b/checker/tests/resourceleak/CommonModuleCrash.java new file mode 100644 index 00000000000..87b3a92bb87 --- /dev/null +++ b/checker/tests/resourceleak/CommonModuleCrash.java @@ -0,0 +1,11 @@ +import java.net.*; + +class CommonModuleCrash { + Socket bar = new Socket(); + + static void baz(Socket s) {} + + static { + baz(new Socket()); + } +} diff --git a/checker/tests/resourceleak/ConnectingServerSockets.java b/checker/tests/resourceleak/ConnectingServerSockets.java new file mode 100644 index 00000000000..c09b8a9845e --- /dev/null +++ b/checker/tests/resourceleak/ConnectingServerSockets.java @@ -0,0 +1,46 @@ +// a set of test cases that demonstrate that errors are actually insued in appropriate +// places when ServerSockets are connected + +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class ConnectingServerSockets { + + static void simple_ss_test(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + ServerSocket s = new ServerSocket(); + s.bind(sa); + } + + static void simple_ss_test2(SocketAddress sa) throws Exception { + ServerSocket s = new ServerSocket(); + // s.bind(sa); + } + + static void simple_ss_test4(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + ServerSocket s = new ServerSocket(); + s.bind(sa, to); + } + + static @MustCall({}) ServerSocket makeUnconnected() throws Exception { + return new ServerSocket(); + } + + static void simple_ss_test5(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + ServerSocket s = makeUnconnected(); + s.bind(sa); + } + + static void simple_ss_test6(SocketAddress sa) throws Exception { + ServerSocket s = makeUnconnected(); + // s.bind(sa); + } + + static void simple_ss_test8(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + ServerSocket s = makeUnconnected(); + s.bind(sa, to); + } +} diff --git a/checker/tests/resourceleak/ConnectingSockets.java b/checker/tests/resourceleak/ConnectingSockets.java new file mode 100644 index 00000000000..b433015a3b1 --- /dev/null +++ b/checker/tests/resourceleak/ConnectingSockets.java @@ -0,0 +1,58 @@ +// a set of test cases that demonstrate that errors are actually insued in appropriate +// places when Sockets are connected + +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class ConnectingSockets { + + static void simple_ns_test(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.bind(sa); + } + + static void simple_ns_test2(SocketAddress sa) throws Exception { + Socket s = new Socket(); + // s.bind(sa); + } + + static void simple_ns_test3(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.connect(sa); + } + + static void simple_ns_test4(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + s.connect(sa, to); + } + + static @MustCall({}) Socket makeUnconnected() throws Exception { + return new Socket(); + } + + static void simple_ns_test5(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = makeUnconnected(); + s.bind(sa); + } + + static void simple_ns_test6(SocketAddress sa) throws Exception { + Socket s = makeUnconnected(); + // s.bind(sa); + } + + static void simple_ns_test7(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = makeUnconnected(); + s.connect(sa); + } + + static void simple_ns_test8(SocketAddress sa, int to) throws Exception { + // :: error: required.method.not.called + Socket s = makeUnconnected(); + s.connect(sa, to); + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForIndirect.java b/checker/tests/resourceleak/CreatesMustCallForIndirect.java new file mode 100644 index 00000000000..3050a40710e --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForIndirect.java @@ -0,0 +1,63 @@ +// A test that methods containing calls to other @CreatesMustCallFor methods work as intended. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForIndirect { + + @CreatesMustCallFor + void reset() {} + + void a() {} + + static @MustCall({}) CreatesMustCallForSimple makeNoMC() { + return null; + } + + public static void resetIndirect_no_anno(CreatesMustCallForIndirect r) { + // :: error: reset.not.owning + r.reset(); + } + + @CreatesMustCallFor("#1") + public static void resetIndirect_anno(CreatesMustCallForIndirect r) { + r.reset(); + } + + public static void reset_local() { + // :: error: required.method.not.called + CreatesMustCallForIndirect r = new CreatesMustCallForIndirect(); + r.reset(); + } + + public static void reset_local2() { + CreatesMustCallForIndirect r = new CreatesMustCallForIndirect(); + r.reset(); + r.a(); + } + + public static void reset_local3() { + // :: error: required.method.not.called + CreatesMustCallForIndirect r = new CreatesMustCallForIndirect(); + // Ideally, we'd issue a reset.not.owning error on the next line instead, but not being able to + // parse + // the case and requiring it to be in a local var is okay too. + // :: error: createsmustcallfor.target.unparseable + ((CreatesMustCallForIndirect) r).reset(); + } + + // :: error: required.method.not.called + public static void test(@Owning CreatesMustCallForIndirect r) { + resetIndirect_anno(r); + } + + public static void test2(CreatesMustCallForIndirect r) { + // :: error: reset.not.owning + resetIndirect_anno(r); + } + + public static void test3(@Owning CreatesMustCallForIndirect r) { + resetIndirect_anno(r); + r.a(); + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForInnerClass.java b/checker/tests/resourceleak/CreatesMustCallForInnerClass.java new file mode 100644 index 00000000000..04f148939c0 --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForInnerClass.java @@ -0,0 +1,26 @@ +// Test case for https://github.com/kelloggm/object-construction-checker/issues/368 + +import org.checkerframework.checker.mustcall.qual.*; + +class CreatesMustCallForInnerClass { + static class Foo { + + @CreatesMustCallFor("this") + void resetFoo() {} + + /** non-static inner class */ + class Bar { + @CreatesMustCallFor + void bar() { + // :: error: reset.not.owning + resetFoo(); + } + } + + void callBar() { + Bar b = new Bar(); + // If this call to bar() were permitted with no errors, this would be unsound. + b.bar(); + } + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForOverride.java b/checker/tests/resourceleak/CreatesMustCallForOverride.java new file mode 100644 index 00000000000..ced5f357995 --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForOverride.java @@ -0,0 +1,31 @@ +// A test case that a create-obligation method cannot be called via dynamic dispatch +// without resetting the obligation. + +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForOverride { + @CreatesMustCallFor + @Override + // :: error: creates.mustcall.for.override.invalid + public String toString() { + return "this method could re-assign a field or do something else it shouldn't"; + } + + public void a() {} + + public static void test_no_cast() { + // :: error: required.method.not.called + CreatesMustCallForOverride co = new CreatesMustCallForOverride(); + co.a(); + co.toString(); + } + + public static void test_cast() { + // it would be ideal if the checker issued an error directly here, but the best we can do is + // issue the error above when the offending version of toString() is defined + CreatesMustCallForOverride co = new CreatesMustCallForOverride(); + co.a(); + Object o = co; + o.toString(); + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForOverride2.java b/checker/tests/resourceleak/CreatesMustCallForOverride2.java new file mode 100644 index 00000000000..8992932f011 --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForOverride2.java @@ -0,0 +1,177 @@ +// This test checks that 1) writing a CO annotation on an overridden method that's also +// CO is permitted, and 2) that all overridden versions of a CO method are always also CO. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class CreatesMustCallForOverride2 { + + @InheritableMustCall("a") + static class Foo { + + @CreatesMustCallFor + public void b() {} + + public void a() {} + } + + static class Bar extends Foo { + + @Override + @CreatesMustCallFor + public void b() {} + } + + static class Baz extends Foo {} + + static class Qux extends Foo { + @Override + public void b() {} + } + + static class Razz extends Foo { + + public @Owning Foo myFoo; + + @Override + @EnsuresCalledMethods(value = "this.myFoo", methods = "a") + public void a() { + super.a(); + myFoo.a(); + } + + // this version isn't permitted, since it adds a new obligation + @Override + @CreatesMustCallFor("this.myFoo") + // :: error: creates.mustcall.for.override.invalid + public void b() {} + } + + static class Thud extends Foo { + + public @Owning Foo myFoo; + + @Override + @EnsuresCalledMethods(value = "this.myFoo", methods = "a") + public void a() { + super.a(); + myFoo.a(); + } + + // this method isn't permitted, since it's also adding a new obligation + @Override + @CreatesMustCallFor("this.myFoo") + @CreatesMustCallFor("this") + // :: error: creates.mustcall.for.override.invalid + public void b() {} + } + + static class Thudless extends Thud { + // this method override is also NOT permitted, because the @CreatesMustCallFor("this.myFoo") + // annotation + // from Thud is inherited! + @Override + @CreatesMustCallFor("this") + // :: error: creates.mustcall.for.override.invalid + public void b() {} + } + + static void test1() { + // :: error: required.method.not.called + Foo foo = new Foo(); + foo.a(); + foo.b(); + } + + static void test2() { + // :: error: required.method.not.called + Foo foo = new Bar(); + foo.a(); + foo.b(); + } + + static void test3() { + // :: error: required.method.not.called + Foo foo = new Baz(); + foo.a(); + foo.b(); + } + + static void test4() { + // :: error: required.method.not.called + Foo foo = new Qux(); + foo.a(); + foo.b(); + } + + static void test5() { + // :: error: required.method.not.called + Bar foo = new Bar(); + foo.a(); + foo.b(); + } + + static void test6() { + // :: error: required.method.not.called + Baz foo = new Baz(); + foo.a(); + foo.b(); + } + + static void test7() { + // :: error: required.method.not.called + Qux foo = new Qux(); + foo.a(); + foo.b(); + } + + static void test8() { + // :: error: required.method.not.called + Foo foo = new Razz(); + foo.a(); + foo.b(); + } + + static void test9() { + // No error is issued here, because Razz#b is *only* @CreatesMustCallFor("this.myFoo"), not + // @CreatesMustCallFor("this"). An error is issued at the declaration of Razz#b instead. + Razz foo = new Razz(); + foo.a(); + foo.b(); + } + + static void test10() { + // :: error: required.method.not.called + Foo foo = new Thud(); + foo.a(); + foo.b(); + } + + static void test11() { + // :: error: required.method.not.called + Thud foo = new Thud(); + foo.a(); + foo.b(); + } + + static void test12() { + // :: error: required.method.not.called + Foo foo = new Thudless(); + foo.a(); + foo.b(); + } + + static void test13() { + // :: error: required.method.not.called + Thud foo = new Thudless(); + foo.a(); + foo.b(); + } + + static void test14() { + // :: error: required.method.not.called + Thudless foo = new Thudless(); + foo.a(); + foo.b(); + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForRepeat.java b/checker/tests/resourceleak/CreatesMustCallForRepeat.java new file mode 100644 index 00000000000..d38f243a965 --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForRepeat.java @@ -0,0 +1,70 @@ +// A simple test that @CreatesMustCallFor is repeatable and works as intended. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForRepeat { + + @CreatesMustCallFor("this") + @CreatesMustCallFor("#1") + void reset(CreatesMustCallForRepeat r) {} + + void a() {} + + static @MustCall({}) CreatesMustCallForRepeat makeNoMC() { + return null; + } + + static void test1() { + // :: error: required.method.not.called + CreatesMustCallForRepeat cos1 = makeNoMC(); + // :: error: required.method.not.called + CreatesMustCallForRepeat cos2 = makeNoMC(); + @MustCall({}) CreatesMustCallForRepeat a = cos2; + @MustCall({}) CreatesMustCallForRepeat a2 = cos2; + cos2.a(); + cos1.reset(cos2); + // :: error: assignment + @CalledMethods({"reset"}) CreatesMustCallForRepeat b = cos1; + @CalledMethods({}) CreatesMustCallForRepeat c = cos1; + @CalledMethods({}) CreatesMustCallForRepeat d = cos2; + // :: error: assignment + @CalledMethods({"a"}) CreatesMustCallForRepeat e = cos2; + } + + static void test3() { + // :: error: required.method.not.called + CreatesMustCallForRepeat cos = new CreatesMustCallForRepeat(); + // :: error: required.method.not.called + CreatesMustCallForRepeat cos2 = new CreatesMustCallForRepeat(); + cos.a(); + cos.reset(cos2); + } + + static void test4() { + CreatesMustCallForRepeat cos = new CreatesMustCallForRepeat(); + // :: error: required.method.not.called + CreatesMustCallForRepeat cos2 = new CreatesMustCallForRepeat(); + cos.a(); + cos.reset(cos2); + cos.a(); + } + + static void test5() { + // :: error: required.method.not.called + CreatesMustCallForRepeat cos = new CreatesMustCallForRepeat(); + CreatesMustCallForRepeat cos2 = new CreatesMustCallForRepeat(); + cos.a(); + cos.reset(cos2); + cos2.a(); + } + + static void test6() { + CreatesMustCallForRepeat cos = new CreatesMustCallForRepeat(); + CreatesMustCallForRepeat cos2 = new CreatesMustCallForRepeat(); + cos.a(); + cos.reset(cos2); + cos2.a(); + cos.a(); + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForSimple.java b/checker/tests/resourceleak/CreatesMustCallForSimple.java new file mode 100644 index 00000000000..abe2775ed9c --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForSimple.java @@ -0,0 +1,76 @@ +// A simple test that @CreatesMustCallFor works as intended wrt the Object Construction Checker. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForSimple { + + @CreatesMustCallFor + void reset() {} + + @CreatesMustCallFor("this") + void resetThis() {} + + void a() {} + + static @MustCall({}) CreatesMustCallForSimple makeNoMC() { + return null; + } + + static void test1() { + // :: error: required.method.not.called + CreatesMustCallForSimple cos = makeNoMC(); + @MustCall({}) CreatesMustCallForSimple a = cos; + cos.reset(); + // :: error: assignment + @CalledMethods({"reset"}) CreatesMustCallForSimple b = cos; + @CalledMethods({}) CreatesMustCallForSimple c = cos; + } + + static void test2() { + // :: error: required.method.not.called + CreatesMustCallForSimple cos = makeNoMC(); + @MustCall({}) CreatesMustCallForSimple a = cos; + cos.resetThis(); + // :: error: assignment + @CalledMethods({"resetThis"}) CreatesMustCallForSimple b = cos; + @CalledMethods({}) CreatesMustCallForSimple c = cos; + } + + static void test3() { + // :: error: required.method.not.called + CreatesMustCallForSimple cos = new CreatesMustCallForSimple(); + cos.a(); + cos.resetThis(); + } + + static void test4() { + CreatesMustCallForSimple cos = new CreatesMustCallForSimple(); + cos.a(); + cos.resetThis(); + cos.a(); + } + + static void test5() { + CreatesMustCallForSimple cos = new CreatesMustCallForSimple(); + cos.resetThis(); + cos.a(); + } + + static void test6(boolean b) { + CreatesMustCallForSimple cos = new CreatesMustCallForSimple(); + if (b) { + cos.resetThis(); + } + cos.a(); + } + + static void test7(boolean b) { + // :: error: required.method.not.called + CreatesMustCallForSimple cos = new CreatesMustCallForSimple(); + cos.a(); + if (b) { + cos.resetThis(); + } + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForSimpler.java b/checker/tests/resourceleak/CreatesMustCallForSimpler.java new file mode 100644 index 00000000000..a3c99423b86 --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForSimpler.java @@ -0,0 +1,29 @@ +// A simpler test that @CreatesMustCallFor works as intended wrt the Object Construction Checker. + +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForSimpler { + + @CreatesMustCallFor + void reset() {} + + @CreatesMustCallFor("this") + void resetThis() {} + + void a() {} + + static @MustCall({}) CreatesMustCallForSimpler makeNoMC() { + return null; + } + + static void test1() { + // :: error: required.method.not.called + CreatesMustCallForSimpler cos = makeNoMC(); + @MustCall({}) CreatesMustCallForSimpler a = cos; + cos.reset(); + // :: error: assignment + @CalledMethods({"reset"}) CreatesMustCallForSimpler b = cos; + @CalledMethods({}) CreatesMustCallForSimpler c = cos; + } +} diff --git a/checker/tests/resourceleak/CreatesMustCallForTargets.java b/checker/tests/resourceleak/CreatesMustCallForTargets.java new file mode 100644 index 00000000000..be9ea157f3d --- /dev/null +++ b/checker/tests/resourceleak/CreatesMustCallForTargets.java @@ -0,0 +1,70 @@ +// A test that errors are correctly issued when re-assignments don't match the +// create obligation annotation on a method. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("a") class CreatesMustCallForTargets { + @Owning InputStream is1; + + @CreatesMustCallFor + // :: error: incompatible.creates.mustcall.for + static void resetObj1(CreatesMustCallForTargets r) throws Exception { + if (r.is1 == null) { + r.is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor("#2") + // :: error: incompatible.creates.mustcall.for + static void resetObj2(CreatesMustCallForTargets r, CreatesMustCallForTargets other) + throws Exception { + if (r.is1 == null) { + r.is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor("#1") + static void resetObj3(CreatesMustCallForTargets r, CreatesMustCallForTargets other) + throws Exception { + if (r.is1 == null) { + r.is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor + void resetObj4(CreatesMustCallForTargets this, CreatesMustCallForTargets other) throws Exception { + if (is1 == null) { + is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor + // :: error: incompatible.creates.mustcall.for + void resetObj5(CreatesMustCallForTargets this, CreatesMustCallForTargets other) throws Exception { + if (other.is1 == null) { + other.is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor("#2") + // :: error: incompatible.creates.mustcall.for + void resetObj6(CreatesMustCallForTargets this, CreatesMustCallForTargets other) throws Exception { + if (other.is1 == null) { + other.is1 = new FileInputStream("foo.txt"); + } + } + + @CreatesMustCallFor("#1") + void resetObj7(CreatesMustCallForTargets this, CreatesMustCallForTargets other) throws Exception { + if (other.is1 == null) { + other.is1 = new FileInputStream("foo.txt"); + } + } + + @EnsuresCalledMethods(value = "this.is1", methods = "close") + void a() throws Exception { + is1.close(); + } +} diff --git a/checker/tests/resourceleak/DoubleIf.java b/checker/tests/resourceleak/DoubleIf.java new file mode 100644 index 00000000000..a0c41d30e24 --- /dev/null +++ b/checker/tests/resourceleak/DoubleIf.java @@ -0,0 +1,54 @@ +// Based on an FP in ZK. Only #parse threw an error; +// removing either if statement made the code verifiable. +// Adding an @CalledMethods annotation, like in parse4, also +// makes this code verifiable... + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.CalledMethods; + +class DoubleIf { + + String fn; + + public void parse(boolean b, boolean c) throws Exception { + if (c) { + FileInputStream fis1 = new FileInputStream(fn); + try { + } finally { + fis1.close(); + } + if (b) {} + } + } + + public void parse2(boolean c) throws Exception { + if (c) { + FileInputStream fis2 = new FileInputStream(fn); + try { + } finally { + fis2.close(); + } + } + } + + public void parse3(boolean b) throws Exception { + FileInputStream fis3 = new FileInputStream(fn); + try { + } finally { + fis3.close(); + } + if (b) {} + } + + public void parse4(boolean b, boolean c) throws Exception { + if (c) { + FileInputStream fis4 = new FileInputStream(fn); + try { + } finally { + fis4.close(); + } + if (b) {} + @CalledMethods("close") FileInputStream fis24 = fis4; + } + } +} diff --git a/checker/tests/resourceleak/Enclosing.java b/checker/tests/resourceleak/Enclosing.java new file mode 100644 index 00000000000..94aedb41151 --- /dev/null +++ b/checker/tests/resourceleak/Enclosing.java @@ -0,0 +1,21 @@ +// test case for https://github.com/kelloggm/object-construction-checker/issues/366 + +import org.checkerframework.checker.mustcall.qual.*; + +class Enclosing { + @MustCall("a") static class Foo { + void a() {} + } + + static class Nested { + // :: error: required.method.not.called + @Owning Foo foo; + + @CreatesMustCallFor("this") + void initFoo() { + if (this.foo == null) { + this.foo = new Foo(); + } + } + } +} diff --git a/checker/tests/resourceleak/EnhancedFor.java b/checker/tests/resourceleak/EnhancedFor.java new file mode 100644 index 00000000000..3144e969225 --- /dev/null +++ b/checker/tests/resourceleak/EnhancedFor.java @@ -0,0 +1,35 @@ +// Based on some false positives I found in ZK. + +import java.io.IOException; +import java.net.Socket; +import java.util.List; + +class EnhancedFor { + void test(List list) { + for (Socket s : list) { + try { + s.close(); + } catch (IOException i) { + } + } + } + + void test2(List list) { + for (int i = 0; i < list.size(); i++) { + Socket s = list.get(i); + try { + s.close(); + } catch (IOException io) { + } + } + } + + void test3(List list) { + // This error is issued because `s` is a local variable, and + // the foreach loop under the hood assigns the result of a call + // to Iterator#next into it (which is owning by default, because it's + // a method return type). + // :: error: required.method.not.called + for (Socket s : list) {} + } +} diff --git a/checker/tests/resourceleak/FileDescriptorTest.java b/checker/tests/resourceleak/FileDescriptorTest.java new file mode 100644 index 00000000000..c1abb083421 --- /dev/null +++ b/checker/tests/resourceleak/FileDescriptorTest.java @@ -0,0 +1,53 @@ +import java.io.*; +import java.io.IOException; +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class FileDescriptorTest { + // This is the original test case. It fails because `in.close()` might throw an exception, which + // is + // not caught; therefore, file might still be open. + public static void readPropertiesFile(File from) throws IOException { + // This is a false positive. + // :: error: required.method.not.called + RandomAccessFile file = new RandomAccessFile(from, "rws"); + FileInputStream in = null; + try { + in = new FileInputStream(file.getFD()); + file.seek(0); + } finally { + if (in != null) { + in.close(); + } + file.close(); + } + } + + // This is a similar test to the above, but without using the indirection through getFD(). + // This test case demonstrates that the problem is not related to getFD(). + // This warning is a false positive, and should be resolved at the same time as the warning above. + // :: error: required.method.not.called + public static void sameScenario_noFD(@Owning Socket sock) throws IOException { + InputStream in = null; + try { + in = sock.getInputStream(); + } finally { + if (in != null) { + in.close(); + } + sock.close(); + } + } + + // This version, written by Narges, does not issue a false positive. + public static void readPropertiesFile_noFP(File from) throws IOException { + RandomAccessFile file = new RandomAccessFile(from, "rws"); + FileInputStream in = null; + try { + in = new FileInputStream(file.getFD()); + in.close(); + } catch (IOException e) { + file.close(); + } + } +} diff --git a/checker/tests/resourceleak/GetChannelOnLocks.java b/checker/tests/resourceleak/GetChannelOnLocks.java new file mode 100644 index 00000000000..50141c09bbb --- /dev/null +++ b/checker/tests/resourceleak/GetChannelOnLocks.java @@ -0,0 +1,39 @@ +// Based on a false positive in hdfs +// A test that shows we are handling the sequence of method invocations correctly + +import java.io.IOException; +import java.nio.channels.FileLock; + +class GetChannelOnLocks { + public boolean isLockSupported(FileLock lock, FileLock lock1, FileLock lock2) throws IOException { + FileLock firstLock = null; + FileLock secondLock = null; + try { + firstLock = lock1; + if (firstLock == null) { + return true; + } + secondLock = lock2; + if (secondLock == null) { + return true; + } + } finally { + if (firstLock != null && firstLock != lock) { + firstLock.release(); + firstLock.channel().close(); + } + if (secondLock != null) { + secondLock.release(); + secondLock.channel().close(); + } + } + return false; + } + + public void isLockSupported2(FileLock lock, FileLock firstLock) throws IOException { + if (firstLock != null && firstLock != lock) { + firstLock.release(); + firstLock.channel().close(); + } + } +} diff --git a/checker/tests/resourceleak/HBaseReport1.java b/checker/tests/resourceleak/HBaseReport1.java new file mode 100644 index 00000000000..2206658f64c --- /dev/null +++ b/checker/tests/resourceleak/HBaseReport1.java @@ -0,0 +1,36 @@ +// Based on a HBase false positive +// In this example fstream is @MCA with out and close is called on all +// exit paths but we randomly report a warning for this test case +// @skip-test + +import java.io.*; +import java.util.*; + +class HBaseReport1 { + + public static void test(String fileName) { + FileWriter fstream; + try { + // :: error: required.method.not.called + fstream = new FileWriter(fileName); + } catch (IOException e) { + return; + } + + BufferedWriter out = new BufferedWriter(fstream); + + try { + try { + out.write(fileName + "\n"); + } finally { + try { + out.close(); + } finally { + fstream.close(); + } + } + } catch (IOException e) { + + } + } +} diff --git a/checker/tests/resourceleak/HdfsReport3.java b/checker/tests/resourceleak/HdfsReport3.java new file mode 100644 index 00000000000..dbb13767e9c --- /dev/null +++ b/checker/tests/resourceleak/HdfsReport3.java @@ -0,0 +1,26 @@ +// Based on a false positive in hdfs + +import java.io.*; +import java.net.*; +import java.nio.*; +import java.nio.file.*; +import java.security.*; +import java.util.*; +import javax.net.ssl.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class HdfsReport3 { + private StringBuffer nonObligationTest(int id) { + final StringWriter out = new StringWriter(); + dumpTreeRecursively(new PrintWriter(out, true), new StringBuilder(), id); + return out.getBuffer(); + } + + public void dumpTreeRecursively(PrintWriter out, StringBuilder prefix, int snapshotId) {} + + // StringBuilder doesn't implement closeable + private final StringBuilder sb = new StringBuilder(); + private final Formatter formatter = new Formatter(sb); +} diff --git a/checker/tests/resourceleak/InheritanceStream.java b/checker/tests/resourceleak/InheritanceStream.java new file mode 100644 index 00000000000..36f85881836 --- /dev/null +++ b/checker/tests/resourceleak/InheritanceStream.java @@ -0,0 +1,22 @@ +// A test that checks that an empty @MustCall annotation in a stub on a subclass overrides +// a non-empty one on a superclass (that is being inherited). + +import java.io.*; + +class InheritanceStream { + void testBAIS(byte[] buf) { + new ByteArrayInputStream(buf); + } + + void testBAOS() { + new ByteArrayOutputStream(); + } + + void testSR(String buf) { + new StringReader(buf); + } + + void testSW() { + new StringWriter(); + } +} diff --git a/checker/tests/resourceleak/InputOutputStreams.java b/checker/tests/resourceleak/InputOutputStreams.java new file mode 100644 index 00000000000..1331a4f4df7 --- /dev/null +++ b/checker/tests/resourceleak/InputOutputStreams.java @@ -0,0 +1,134 @@ +// A MustCallAlias test wrt sockets. Also coincidentally tests that MCA sets can be larger than two. + +import java.io.*; +import java.io.IOException; +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class InputOutputStreams { + void test_close_sock(@Owning Socket sock) throws IOException { + try { + InputStream is = sock.getInputStream(); + OutputStream os = sock.getOutputStream(); + } catch (IOException e) { + + } finally { + sock.close(); + } + } + + // getInputStream()/getOutputStream() can throw IOException in three different scenarios: + // 1) The underlying socket is already closed + // 2) The underlying socket is not connected + // 3) The underlysing socket input is shutdown + // In the first case our checker always reports a false positive but for the second case + // and third case our checker has to verify that close is called on the underlying resource. + // So, because sock.getInputStream() can throw IOException, "is" can be null, then sock will + // remain open. So, it's a true positive warning. + // :: error: required.method.not.called + void test_close_is(@Owning Socket sock) throws IOException { + InputStream is = null; + OutputStream os = null; + try { + is = sock.getInputStream(); + os = sock.getOutputStream(); + } catch (IOException e) { + + } finally { + is.close(); + } + } + + // :: error: required.method.not.called + void test_close_os(@Owning Socket sock) throws IOException { + InputStream is = sock.getInputStream(); + OutputStream os = sock.getOutputStream(); + os.close(); + } + + // :: error: required.method.not.called + void test_close_os2(@Owning Socket sock) throws IOException { + OutputStream os = null; + InputStream is = null; + try { + is = sock.getInputStream(); + } catch (IOException e) { + try { + os = sock.getOutputStream(); + } catch (IOException ee) { + } + } finally { + os.close(); + } + } + + // :: error: required.method.not.called + void test_close_os3(@Owning Socket sock) throws IOException { + OutputStream os = null; + try { + InputStream is = sock.getInputStream(); + } catch (IOException e) { + } + try { + os = sock.getOutputStream(); + } finally { + os.close(); + } + } + + // TODO this case requires more general tracking of additional boolean conditions + // If getOutputStream() throws an IOException, then the os variable remains definitely null. + // When our worklist analysis gets to the finally block along the corresponding CFG edge, + // it does not know that os is definitely null, and it is only tracking sock as a name for + // the resource. So, it analyzes a path through the "then" branch of the conditional, + // where sock.close() is not invoked, and reports an error. + // :: error: required.method.not.called + void test_close_os4(@Owning Socket sock) throws IOException { + OutputStream os = null; + try { + InputStream is = sock.getInputStream(); + } catch (IOException e) { + } + try { + os = sock.getOutputStream(); + } finally { + if (os != null) { + os.close(); + } else { + sock.close(); + } + } + } + + // :: error: required.method.not.called + void test_close_buff(@Owning Socket sock) throws IOException { + BufferedOutputStream buff = null; + try { + InputStream is = sock.getInputStream(); + OutputStream os = sock.getOutputStream(); + buff = new BufferedOutputStream(os); + } catch (IOException e) { + + } finally { + buff.close(); + } + } + + void test_write(String host, int port) { + Socket sock = null; + try { + sock = new Socket(host, port); + sock.getOutputStream().write("isro".getBytes()); + } catch (Exception e) { + System.err.println("write failed: " + e); + } finally { + if (sock != null) { + try { + sock.close(); + } catch (IOException e) { + System.err.println("couldn't close socket!"); + } + } + } + } +} diff --git a/checker/tests/resourceleak/IsClosed.java b/checker/tests/resourceleak/IsClosed.java new file mode 100644 index 00000000000..44150381f30 --- /dev/null +++ b/checker/tests/resourceleak/IsClosed.java @@ -0,0 +1,28 @@ +// A test that the EnsuresCalledMethodsIf annotations in +// the Socket stub files are respected. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.mustcall.qual.Owning; + +class IsClosed { + void test_socket(@Owning Socket sock) { + if (!sock.isClosed()) { + try { + sock.close(); + } catch (IOException io) { + + } + } + } + + void test_server_socket(@Owning ServerSocket sock) { + if (!sock.isClosed()) { + try { + sock.close(); + } catch (IOException io) { + + } + } + } +} diff --git a/checker/tests/resourceleak/MCANotOwningField.java b/checker/tests/resourceleak/MCANotOwningField.java new file mode 100644 index 00000000000..25aa4087117 --- /dev/null +++ b/checker/tests/resourceleak/MCANotOwningField.java @@ -0,0 +1,17 @@ +// A test case that a MustCallAlias value with a non-owning field with a must-call obligation +// does not lead to a false positive. + +import java.net.Socket; + +class MCANotOwningField { + + final Socket s; + + MCANotOwningField(Socket s) throws Exception { + this.s = s; + } + + void simple() throws Exception { + s.getInputStream(); + } +} diff --git a/checker/tests/resourceleak/MCAOwningField.java b/checker/tests/resourceleak/MCAOwningField.java new file mode 100644 index 00000000000..e4e2f849ef1 --- /dev/null +++ b/checker/tests/resourceleak/MCAOwningField.java @@ -0,0 +1,24 @@ +// A test case for a common pattern in Zookeeper: something is must-call-alias +// with an owning field, and therefore a false positive was issued. + +import java.net.Socket; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("stop") class MCAOwningField { + + @Owning final Socket s; + + MCAOwningField() throws Exception { + s = new Socket(); + } + + void simple() throws Exception { + s.getInputStream(); + } + + @EnsuresCalledMethods(value = "s", methods = "close") + void stop() throws Exception { + s.close(); + } +} diff --git a/checker/tests/resourceleak/MCAWithThis.java b/checker/tests/resourceleak/MCAWithThis.java new file mode 100644 index 00000000000..d9451bb2700 --- /dev/null +++ b/checker/tests/resourceleak/MCAWithThis.java @@ -0,0 +1,18 @@ +// A test that when a must-call class with MustCallAlias methods +// is extended, those methods can be used without false positives. + +import java.net.Socket; + +class MCAWithThis extends Socket { + public MCAWithThis() { + super(); + } + + public void test() throws Exception { + this.getInputStream(); + } + + public void test2() throws Exception { + getInputStream(); + } +} diff --git a/checker/tests/resourceleak/ManualMustCallEmptyOnConstructor.java b/checker/tests/resourceleak/ManualMustCallEmptyOnConstructor.java new file mode 100644 index 00000000000..9c2239c39c8 --- /dev/null +++ b/checker/tests/resourceleak/ManualMustCallEmptyOnConstructor.java @@ -0,0 +1,24 @@ +// test for https://github.com/kelloggm/object-construction-checker/issues/326 + +import java.io.InputStream; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class ManualMustCallEmptyOnConstructor { + + // Test that writing @MustCall({}) on a constructor results in an error + @MustCall("a") static class Foo { + final @Owning InputStream is; + + // :: error: inconsistent.constructor.type + @MustCall({}) Foo(@Owning InputStream is) { + this.is = is; + } + + @EnsuresCalledMethods(value = "this.is", methods = "close") + void a() throws Exception { + is.close(); + } + } +} diff --git a/checker/tests/resourceleak/MustCallAliasExamples.java b/checker/tests/resourceleak/MustCallAliasExamples.java new file mode 100644 index 00000000000..e45513be55d --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasExamples.java @@ -0,0 +1,53 @@ +// Simple tests of @MustCallAlias functionality on wrapper streams. + +import java.io.*; +import java.io.IOException; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasExamples { + + void test_two_locals(String address) { + Socket socket = null; + try { + socket = new Socket(address, 8000); + DataInputStream d = new DataInputStream(socket.getInputStream()); + } catch (IOException e) { + + } finally { + closeSocket(socket); + } + } + + void test_close_wrapper(@Owning InputStream b) throws IOException { + DataInputStream d = new DataInputStream(b); + d.close(); + } + + void test_close_nonwrapper(@Owning InputStream b) throws IOException { + DataInputStream d = new DataInputStream(b); + b.close(); + } + + // :: error: required.method.not.called + void test_no_close(@Owning InputStream b) { + DataInputStream d = new DataInputStream(b); + } + + // :: error: required.method.not.called + void test_no_assign(@Owning InputStream b) { + new DataInputStream(new BufferedInputStream(b)); + } + + @EnsuresCalledMethods(value = "#1", methods = "close") + void closeSocket(Socket sock) { + try { + if (sock != null) { + sock.close(); + } + } catch (IOException e) { + + } + } +} diff --git a/checker/tests/resourceleak/MustCallAliasImpl.java b/checker/tests/resourceleak/MustCallAliasImpl.java new file mode 100644 index 00000000000..66b6794c089 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasImpl.java @@ -0,0 +1,23 @@ +// A simple test that the extra obligations that MustCallAlias imposes are +// respected. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class MustCallAliasImpl implements Closeable { + + final @Owning Closeable foo; + + public @MustCallAlias MustCallAliasImpl(@MustCallAlias Closeable foo) { + this.foo = foo; + } + + @Override + @EnsuresCalledMethods( + value = {"this.foo"}, + methods = {"close"}) + public void close() throws IOException { + this.foo.close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasImplWrong1.java b/checker/tests/resourceleak/MustCallAliasImplWrong1.java new file mode 100644 index 00000000000..8cb88dd09e1 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasImplWrong1.java @@ -0,0 +1,25 @@ +// A simple test that the extra obligations that MustCallAlias imposes are +// respected. This version gets it wrong by not assigning the MCA param +// to a field. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class MustCallAliasImplWrong1 implements Closeable { + + final @Owning Closeable foo; + + // :: error: required.method.not.called + public @MustCallAlias MustCallAliasImplWrong1(@MustCallAlias Closeable foo) { + this.foo = null; + } + + @Override + @EnsuresCalledMethods( + value = {"this.foo"}, + methods = {"close"}) + public void close() throws IOException { + this.foo.close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasImplWrong2.java b/checker/tests/resourceleak/MustCallAliasImplWrong2.java new file mode 100644 index 00000000000..a302fbfa43d --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasImplWrong2.java @@ -0,0 +1,20 @@ +// A simple test that the extra obligations that MustCallAlias imposes are +// respected. This version gets it wrong by assigning the MCA param to a non-owning +// field. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class MustCallAliasImplWrong2 implements Closeable { + + final /*@Owning*/ Closeable foo; + + // :: error: required.method.not.called + public @MustCallAlias MustCallAliasImplWrong2(@MustCallAlias Closeable foo) { + this.foo = foo; + } + + @Override + public void close() {} +} diff --git a/checker/tests/resourceleak/MustCallAliasLayeredStreams.java b/checker/tests/resourceleak/MustCallAliasLayeredStreams.java new file mode 100644 index 00000000000..6b8f6a7ac8d --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasLayeredStreams.java @@ -0,0 +1,20 @@ +// Test case based on an MCA situation in Zookeeper. +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasLayeredStreams { + InputStream cache; + + public InputStream createInputStream(String filename) throws FileNotFoundException { + if (cache == null) { + // The real version of this uses a mix of JDK and custom streams, so it makes more sense... + // TODO we shouldn't report a warning here and the code is okay because the cache is + // non-owning, + // and the caller of createInputStream is the owner of all of these streams. + // :: error: required.method.not.called + cache = new DataInputStream(new BufferedInputStream(new FileInputStream(new File(filename)))); + } + return cache; + } +} diff --git a/checker/tests/resourceleak/MustCallAliasOwningField.java b/checker/tests/resourceleak/MustCallAliasOwningField.java new file mode 100644 index 00000000000..458c7b886a0 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasOwningField.java @@ -0,0 +1,29 @@ +// Based on a MustCallAlias scenario in Zookeeper. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public @MustCall("shutdown") class MustCallAliasOwningField { + + private final @Owning BufferedInputStream input; + + public MustCallAliasOwningField(@Owning BufferedInputStream input, boolean b) { + this.input = input; + if (b) { + DataInputStream d = new DataInputStream(input); + authenticate(d); + } + } + + @EnsuresCalledMethods(value = "this.input", methods = "close") + public void shutdown() throws IOException { + input.close(); + } + + public static void authenticate(InputStream is) {} + + public void wrapField() { + DataInputStream dis = new DataInputStream(input); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthrough.java b/checker/tests/resourceleak/MustCallAliasPassthrough.java new file mode 100644 index 00000000000..86a58ca7ccc --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthrough.java @@ -0,0 +1,12 @@ +// A test that a class can extend another class with an MCA constructor, +// and have its own constructor be MCA as well. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthrough extends FilterInputStream { + @MustCallAlias MustCallAliasPassthrough(@MustCallAlias InputStream is) { + super(is); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughChain.java b/checker/tests/resourceleak/MustCallAliasPassthroughChain.java new file mode 100644 index 00000000000..099e7abba9a --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughChain.java @@ -0,0 +1,53 @@ +// This test checks that a chain of several MCA methods can be verified, and that messing up the +// chain +// leads to errors. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughChain { + + static @MustCallAlias InputStream withMCA(@MustCallAlias InputStream is) { + return is; + } + + static @MustCallAlias InputStream chain1(@MustCallAlias InputStream is) { + return withMCA(is); + } + + static @MustCallAlias InputStream chain2(@MustCallAlias InputStream is) { + InputStream s = withMCA(is); + return s; + } + + static @MustCallAlias InputStream chain3(@MustCallAlias InputStream is) { + return withMCA(chain1(is)); + } + + static @MustCallAlias InputStream chain4(@MustCallAlias InputStream is) { + return withMCA(chain1(chain3(is))); + } + + static @MustCallAlias InputStream chain5(@MustCallAlias InputStream is) { + InputStream s = withMCA(chain1(is)); + return s; + } + + // :: error: required.method.not.called + static @MustCallAlias InputStream chain_bad1(@MustCallAlias InputStream is) { + InputStream s = withMCA(chain1(is)); + return null; + } + + // :: error: required.method.not.called + static @MustCallAlias InputStream chain_bad2(@MustCallAlias InputStream is) { + withMCA(chain1(is)); + return null; + } + + // :: error: required.method.not.called + static @MustCallAlias InputStream chain_bad3(@MustCallAlias InputStream is, boolean b) { + return b ? null : withMCA(chain1(is)); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughLocal.java b/checker/tests/resourceleak/MustCallAliasPassthroughLocal.java new file mode 100644 index 00000000000..67424865bea --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughLocal.java @@ -0,0 +1,22 @@ +// A test that passing a local to an MCA super constructor is allowed. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughLocal extends FilterInputStream { + MustCallAliasPassthroughLocal(File f) throws Exception { + // This is safe - this MCA constructor of FilterInputStream means that the result of this + // constructor - i.e. the caller - is taking ownership of this newly-created output stream. + super(new FileInputStream(f)); + } + + static void test(File f) throws Exception { + // :: error: required.method.not.called + new MustCallAliasPassthroughLocal(f); + } + + static void test_ok(File f) throws Exception { + new MustCallAliasPassthroughLocal(f).close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughThis.java b/checker/tests/resourceleak/MustCallAliasPassthroughThis.java new file mode 100644 index 00000000000..decc01adf57 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughThis.java @@ -0,0 +1,15 @@ +// A test that a class can have multiple MCA constructors. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughThis extends FilterInputStream { + @MustCallAlias MustCallAliasPassthroughThis(@MustCallAlias InputStream is) { + super(is); + } + + @MustCallAlias MustCallAliasPassthroughThis(@MustCallAlias InputStream is, int x) { + this(is); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughWrong1.java b/checker/tests/resourceleak/MustCallAliasPassthroughWrong1.java new file mode 100644 index 00000000000..8d75da29236 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughWrong1.java @@ -0,0 +1,14 @@ +// A test that a class can extend another class with an MCA constructor, +// and have its own constructor be MCA as well. +// This version just throws away the input rather than passing it to the super constructor. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughWrong1 extends FilterInputStream { + // :: error: required.method.not.called + @MustCallAlias MustCallAliasPassthroughWrong1(@MustCallAlias InputStream is) { + super(null); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughWrong2.java b/checker/tests/resourceleak/MustCallAliasPassthroughWrong2.java new file mode 100644 index 00000000000..19a24e9c982 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughWrong2.java @@ -0,0 +1,21 @@ +// A test that a class can extend another class with an MCA constructor, +// and have its own constructor be MCA as well. +// This version passes the MCA param to another method instead of the passthrough constructor. +// This is actually okay - the stream does get closed, if it needs to be closed - though the +// MCA annotation on the return type is super misleading and will lead to FPs. It would be better +// to annotate code like this with @Owning on the constructor. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughWrong2 extends FilterInputStream { + @MustCallAlias MustCallAliasPassthroughWrong2(@MustCallAlias InputStream is) throws Exception { + super(null); + closeIS(is); + } + + void closeIS(@Owning InputStream is) throws Exception { + is.close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughWrong3.java b/checker/tests/resourceleak/MustCallAliasPassthroughWrong3.java new file mode 100644 index 00000000000..85b4ca4daf4 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughWrong3.java @@ -0,0 +1,28 @@ +// This is a test for what happens when there's a missing MCA return type. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughWrong3 { + // Both of these verify - it's okay to leave off the MCA param, because the return type is + // owning - but the first one leads to imprecision at call sites. + static InputStream missingMCA(@MustCallAlias InputStream is) { + return is; + } + + static @MustCallAlias InputStream withMCA(@MustCallAlias InputStream is) { + return is; + } + + // :: error: required.method.not.called + void use_bad(@Owning InputStream is) throws Exception { + InputStream is2 = missingMCA(is); + is2.close(); + } + + void use_good(@Owning InputStream is) throws Exception { + InputStream is2 = withMCA(is); + is2.close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasPassthroughWrong4.java b/checker/tests/resourceleak/MustCallAliasPassthroughWrong4.java new file mode 100644 index 00000000000..af2d0604548 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasPassthroughWrong4.java @@ -0,0 +1,17 @@ +// A test that a class can extend another class with an MCA constructor, +// and have its own constructor be MCA as well. +// This version just closes the MCA parameter, which isn't wrong so much as weird but I wanted a +// test for it. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class MustCallAliasPassthroughWrong4 extends FilterInputStream { + // I mean I guess this return type is technically okay - it's too conservative (@Owning on the + // param would be better) but I see no reason not to verify it. + @MustCallAlias MustCallAliasPassthroughWrong4(@MustCallAlias InputStream is) throws Exception { + super(null); + is.close(); + } +} diff --git a/checker/tests/resourceleak/MustCallAliasSocketException.java b/checker/tests/resourceleak/MustCallAliasSocketException.java new file mode 100644 index 00000000000..56db01e82c5 --- /dev/null +++ b/checker/tests/resourceleak/MustCallAliasSocketException.java @@ -0,0 +1,28 @@ +// Based on a false positive in Zookeeper's SaslQuorumAuthLearner. + +import java.io.*; +import java.net.*; + +class MustCallAliasSocketException { + + public boolean quorumRequireSasl; + + // This socket isn't owning, so we shouldn't warn on it. + public void authenticate(Socket sock, String hostName) throws IOException { + if (!quorumRequireSasl) { + // I kept this block in the test case because it demonstrates + // that sock is definitely non-owning. + System.out.println("Skipping SASL authentication as"); + return; + } + try { + DataOutputStream dout = new DataOutputStream(sock.getOutputStream()); + // Before MCA was implemented, the call to getInputStream() below triggered + // a false positive warning that dout had not been closed. + DataInputStream din = new DataInputStream(sock.getInputStream()); + // ~30 lines omitted... + } finally { + // do some other things that are definitely not closing sock + } + } +} diff --git a/checker/tests/resourceleak/MustCallNullStore.java b/checker/tests/resourceleak/MustCallNullStore.java new file mode 100644 index 00000000000..0b88c11696d --- /dev/null +++ b/checker/tests/resourceleak/MustCallNullStore.java @@ -0,0 +1,20 @@ +import java.nio.ByteBuffer; + +// input exposing a bug where store after the return is null +class MustCallNullStore { + + int inflateDirect(ByteBuffer src, ByteBuffer dst) throws java.io.IOException { + + ByteBuffer presliced = dst; + if (dst.position() > 0) { + dst = dst.slice(); + } + + int n = 0; + try { + presliced.position(presliced.position() + n); + } finally { + } + return n; + } +} diff --git a/checker/tests/resourceleak/MustCloseIntoObject.java b/checker/tests/resourceleak/MustCloseIntoObject.java new file mode 100644 index 00000000000..b82c5143c15 --- /dev/null +++ b/checker/tests/resourceleak/MustCloseIntoObject.java @@ -0,0 +1,11 @@ +// Test that assigning a "must close" value into a class without a mustCall annotation +// still results in an error. + +import java.net.Socket; + +class MustCloseIntoObject { + void test() throws Exception { + // :: error: required.method.not.called + Object o = new Socket("", 0); + } +} diff --git a/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull.java b/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull.java new file mode 100644 index 00000000000..55547a0c39c --- /dev/null +++ b/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull.java @@ -0,0 +1,87 @@ +// A test that must-call close errors are not issued when overwriting a field +// if the field is definitely null. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods; +import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; +import org.checkerframework.checker.mustcall.qual.MustCall; +import org.checkerframework.checker.mustcall.qual.Owning; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; + +@MustCall("close") class NonFinalFieldOnlyOverwrittenIfNull { + @Owning @MonotonicNonNull InputStream is; + + @CreatesMustCallFor + void set(String fn) throws FileNotFoundException { + if (is == null) { + is = new FileInputStream(fn); + } + } + + @CreatesMustCallFor + void set_after_close(String fn, boolean b) throws IOException { + if (b) { + is.close(); + is = new FileInputStream(fn); + } + } + + @CreatesMustCallFor + void set_error(String fn, boolean b) throws FileNotFoundException { + if (b) { + // :: error: required.method.not.called + is = new FileInputStream(fn); + } + } + + // These three methods are copies of the three above, without the appropriate annotation. + // :: error: missing.creates.mustcall.for + void set2(String fn) throws FileNotFoundException { + if (is == null) { + is = new FileInputStream(fn); + } + } + + // :: error: missing.creates.mustcall.for + void set_after_close2(String fn, boolean b) throws IOException { + if (b) { + is.close(); + is = new FileInputStream(fn); + } + } + + // :: error: missing.creates.mustcall.for + void set_error2(String fn, boolean b) throws FileNotFoundException { + if (b) { + // :: error: required.method.not.called + is = new FileInputStream(fn); + } + } + + /* This version of close() doesn't verify, because in the `catch` block + `is` isn't @CalledMethods("close"). TODO: investigate that in the CM checker + @EnsuresCalledMethods(value="this.is", methods="close") + void close_real() { + if (is != null) { + try { + is.close(); + } catch (Exception ie) { + + } + } + } */ + + @EnsuresCalledMethods(value = "this.is", methods = "close") + void close() throws Exception { + if (is != null) { + is.close(); + } + } + + public static void test_leak() throws Exception { + // :: error: required.method.not.called + NonFinalFieldOnlyOverwrittenIfNull n = new NonFinalFieldOnlyOverwrittenIfNull(); + n.close(); + n.set_after_close("bar.txt", true); + } +} diff --git a/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull2.java b/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull2.java new file mode 100644 index 00000000000..ac8d2045b99 --- /dev/null +++ b/checker/tests/resourceleak/NonFinalFieldOnlyOverwrittenIfNull2.java @@ -0,0 +1,66 @@ +// Another test that must-call close errors are not issued when overwriting a field +// if the field is definitely null. + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods; +import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; +import org.checkerframework.checker.mustcall.qual.MustCall; +import org.checkerframework.checker.mustcall.qual.Owning; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; + +@MustCall("close") class NonFinalFieldOnlyOverwrittenIfNull2 { + @Owning @MonotonicNonNull InputStream is; + + @CreatesMustCallFor + void set(String fn) throws FileNotFoundException { + if (is == null) { + is = new FileInputStream(fn); + } + } + + @CreatesMustCallFor + void set_after_close(String fn, boolean b) throws IOException { + if (b) { + is.close(); + is = new FileInputStream(fn); + } + } + + @CreatesMustCallFor + void set_error(String fn, boolean b) throws FileNotFoundException { + if (b) { + // :: error: required.method.not.called + is = new FileInputStream(fn); + } + } + + /* This version of close() doesn't verify, because in the `catch` block + `is` isn't @CalledMethods("close"). TODO: investigate that in the CM checker + @EnsuresCalledMethods(value="this.is", methods="close") + void close_real() { + if (is != null) { + try { + is.close(); + } catch (Exception ie) { + + } + } + } */ + + @EnsuresCalledMethods(value = "this.is", methods = "close") + @CreatesMustCallFor + void close() throws Exception { + if (is != null) { + is.close(); + is = null; + } + } + + public static void test_leak() throws Exception { + // :: error: required.method.not.called + NonFinalFieldOnlyOverwrittenIfNull2 n = new NonFinalFieldOnlyOverwrittenIfNull2(); + n.set("foo.txt"); + n.close(); + n.set("bar.txt"); + } +} diff --git a/checker/tests/resourceleak/OptionalSocket.java b/checker/tests/resourceleak/OptionalSocket.java new file mode 100644 index 00000000000..f1153eeca00 --- /dev/null +++ b/checker/tests/resourceleak/OptionalSocket.java @@ -0,0 +1,25 @@ +// A test case for some false positives that came up in Zookeeper. +// These are sort-of a must call alias situation, but it's not as clear. +// I suspect our MCA implementation won't actually handle these cases, in +// which case it's fine with me to skip this test for now. - Martin +// @skip-test + +import java.io.*; +import java.net.*; +import java.util.*; +import org.checkerframework.checker.mustcall.qual.*; + +class OptionalSocket { + void test_close_get_null(@Owning Optional sock) throws IOException { + // TODO can't pass this + if (sock.get() != null) { + // TODO can't pass this + sock.get().close(); + } + } + + void test_close_get(@Owning Optional sock) throws IOException { + // TODO can't pass this + sock.get().close(); + } +} diff --git a/checker/tests/resourceleak/PaperExample.java b/checker/tests/resourceleak/PaperExample.java new file mode 100644 index 00000000000..2d337ee3646 --- /dev/null +++ b/checker/tests/resourceleak/PaperExample.java @@ -0,0 +1,16 @@ +import java.net.Socket; + +class PaperExample { + void test(String myHost, int myPort) throws Exception { + Socket s = null; + try { + s = new Socket(myHost, myPort); /* 1 */ + } catch (Exception e) { + } finally { + if (s != null) { + /* 2 */ + s.close(); + } + } + } +} diff --git a/checker/tests/resourceleak/ReassignmentWithMCA.java b/checker/tests/resourceleak/ReassignmentWithMCA.java new file mode 100644 index 00000000000..5d0c4dfb50b --- /dev/null +++ b/checker/tests/resourceleak/ReassignmentWithMCA.java @@ -0,0 +1,49 @@ +import java.io.*; +import java.security.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class ReassignmentWithMCA { + void testReassignment(File newFile, MessageDigest digester) throws IOException { + FileOutputStream fout = new FileOutputStream(newFile); + DigestOutputStream fos = new DigestOutputStream(fout, digester); + DataOutputStream out = new DataOutputStream(fos); + try { + out = new DataOutputStream(new BufferedOutputStream(fos)); + fout.getChannel(); + } finally { + out.close(); + } + } + + void testReassignmentWithoutMCA( + @Owning FileOutputStream fout1, @Owning FileOutputStream fout2, MessageDigest digester) + throws IOException { + DigestOutputStream fos1 = new DigestOutputStream(fout1, digester); + DataOutputStream out = new DataOutputStream(fos1); + try { + DigestOutputStream fos2 = new DigestOutputStream(fout2, digester); + out = new DataOutputStream(new BufferedOutputStream(fos2)); + fout1.getChannel(); + } finally { + callClose(fout1); + callClose(fout2); + } + } + + void testReassignmentSetSizeOne(@Owning FilterOutputStream out) throws IOException { + out = new DataOutputStream(out); + out.close(); + } + + @EnsuresCalledMethods(value = "#1", methods = "close") + void callClose(Closeable c) { + try { + if (c != null) { + c.close(); + } + } catch (IOException e) { + + } + } +} diff --git a/checker/tests/resourceleak/RequiresCalledMethodsTest.java b/checker/tests/resourceleak/RequiresCalledMethodsTest.java new file mode 100644 index 00000000000..9a0250f6c57 --- /dev/null +++ b/checker/tests/resourceleak/RequiresCalledMethodsTest.java @@ -0,0 +1,51 @@ +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +public class RequiresCalledMethodsTest { + + @MustCall("a") static class Foo { + void a() {} + + void c() {} + } + + @MustCall("releaseFoo") static class FooField { + private @Owning Foo foo = null; + + @RequiresCalledMethods( + value = {"this.foo"}, + methods = {"a"}) + @CreatesMustCallFor("this") + void overwriteFooCorrect() { + this.foo = new Foo(); + } + + @CreatesMustCallFor("this") + void overwriteFooWrong() { + // :: error: required.method.not.called + this.foo = new Foo(); + } + + @CreatesMustCallFor("this") + void overwriteFooWithoutReleasing() { + // :: error: contracts.precondition + overwriteFooCorrect(); + } + + void releaseThenOverwriteFoo() { + releaseFoo(); + // :: error: reset.not.owning + overwriteFooCorrect(); + } + + @EnsuresCalledMethods( + value = {"this.foo"}, + methods = {"a"}) + void releaseFoo() { + if (this.foo != null) { + foo.a(); + } + } + } +} diff --git a/checker/tests/resourceleak/SSLSocketFactoryTest.java b/checker/tests/resourceleak/SSLSocketFactoryTest.java new file mode 100644 index 00000000000..40570ada578 --- /dev/null +++ b/checker/tests/resourceleak/SSLSocketFactoryTest.java @@ -0,0 +1,17 @@ +// A test for a bug that came up while porting the Resource Leak Checker into the +// Checker Framework proper. + +import java.io.IOException; +import java.net.Socket; +import javax.net.ssl.*; +import org.checkerframework.checker.mustcall.qual.*; + +class SSLSocketFactoryTest { + public SSLSocket createSSLSocket(@Owning Socket socket, SSLContext sslContext) + throws IOException { + SSLSocket sslSocket = + (SSLSocket) + sslContext.getSocketFactory().createSocket(socket, null, socket.getPort(), true); + return sslSocket; + } +} diff --git a/checker/tests/resourceleak/SelfAssign.java b/checker/tests/resourceleak/SelfAssign.java new file mode 100644 index 00000000000..d658faf67c7 --- /dev/null +++ b/checker/tests/resourceleak/SelfAssign.java @@ -0,0 +1,36 @@ +// test assignments of the same variable to itself + +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class SelfAssign { + + static void test0() throws IOException { + InputStream selfAssignIn0 = new FileInputStream("file.txt"); + try { + selfAssignIn0 = selfAssignIn0; + } finally { + selfAssignIn0.close(); + } + } + + static void test1(boolean b) throws IOException { + InputStream selfAssignIn = new FileInputStream("file.txt"); + try { + selfAssignIn = + selfAssignIn.markSupported() ? selfAssignIn : new BufferedInputStream(selfAssignIn); + } finally { + selfAssignIn.close(); + } + } + + static void test2(boolean b) throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + in = new BufferedInputStream(in); + } finally { + in.close(); + } + } +} diff --git a/checker/tests/resourceleak/SimpleSocketExample.java b/checker/tests/resourceleak/SimpleSocketExample.java new file mode 100644 index 00000000000..e3e09484305 --- /dev/null +++ b/checker/tests/resourceleak/SimpleSocketExample.java @@ -0,0 +1,17 @@ +// A simple socket example for debugging. + +import java.io.IOException; +import java.net.Socket; + +class SimpleSocketExample { + void basicTest(String address, int port) { + try { + // :: error: required.method.not.called + Socket socket2 = new Socket(address, port); + Socket specialSocket = new Socket(address, port); + specialSocket.close(); + } catch (IOException i) { + + } + } +} diff --git a/checker/tests/resourceleak/SocketContainer.java b/checker/tests/resourceleak/SocketContainer.java new file mode 100644 index 00000000000..e9845f22f24 --- /dev/null +++ b/checker/tests/resourceleak/SocketContainer.java @@ -0,0 +1,30 @@ +// A simple class that has a Socket as an owning field. +// This test exists to check that we gracefully handle assignments 1) +// in the constructor and 2) to null. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("close") class SocketContainer { + @Owning Socket sock; + + public SocketContainer(String host, int port) throws Exception { + // It should be okay to assign to uninitialized owning fields in the constructor. + // But it isn't! Why? + // :: error: required.method.not.called + sock = new Socket(host, port); + // It's definitely not okay to do it twice! + // :: error: required.method.not.called + sock = new Socket(host, port); + } + + @EnsuresCalledMethods(value = "this.sock", methods = "close") + public void close() throws IOException { + sock.close(); + // It's okay to assign a field to null after its obligations have been fulfilled, + // without inducing a reset. + sock = null; + } +} diff --git a/checker/tests/resourceleak/SocketContainer2.java b/checker/tests/resourceleak/SocketContainer2.java new file mode 100644 index 00000000000..bcfc4046af0 --- /dev/null +++ b/checker/tests/resourceleak/SocketContainer2.java @@ -0,0 +1,23 @@ +// A simple class that has a Socket as an owning field. +// This test exists to check that we gracefully handle assignments to it. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("close") class SocketContainer2 { + + @Owning Socket sock = new Socket(); + + public SocketContainer2(String host, int port) throws Exception { + // This assignment is safe, because the only possible value of sock here is the unconnected + // socket in the field initializer. + sock = new Socket(host, port); + } + + @EnsuresCalledMethods(value = "this.sock", methods = "close") + public void close() throws IOException { + sock.close(); + } +} diff --git a/checker/tests/resourceleak/SocketContainer3.java b/checker/tests/resourceleak/SocketContainer3.java new file mode 100644 index 00000000000..93d839bb29e --- /dev/null +++ b/checker/tests/resourceleak/SocketContainer3.java @@ -0,0 +1,20 @@ +// A simple class that has a Socket as an owning field. +// This test exists to check that we gracefully handle assignments. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +@MustCall("close") class SocketContainer3 { + @Owning Socket sock = null; + + public SocketContainer3(String host, int port) throws Exception { + sock = new Socket(host, port); + } + + @EnsuresCalledMethods(value = "this.sock", methods = "close") + public void close() throws IOException { + sock.close(); + } +} diff --git a/checker/tests/resourceleak/SocketField.java b/checker/tests/resourceleak/SocketField.java new file mode 100644 index 00000000000..b9bda24426e --- /dev/null +++ b/checker/tests/resourceleak/SocketField.java @@ -0,0 +1,65 @@ +// test case for https://github.com/kelloggm/object-construction-checker/issues/381 + +import java.io.IOException; +import java.net.Socket; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.dataflow.qual.Pure; + +@MustCall("closeSocket") class SocketField { + protected @Owning Socket socket = null; + + @CreatesMustCallFor("this") + protected void setupConnection(javax.net.SocketFactory socketFactory) throws IOException { + // This is the original test case. Before this issue was fixed, an error was issued on the + // second line. + this.socket.close(); + this.socket = socketFactory.createSocket(); + } + + @CreatesMustCallFor("this") + protected void setupConnectionWithLocal(javax.net.SocketFactory socketFactory) + throws IOException { + // This is the original test case, modified to include an assignment to a local that + // demonstrates that + // the correct value was in the store at some point. + this.socket.close(); + @CalledMethods("close") Socket s = this.socket; + this.socket = socketFactory.createSocket(); + } + + @CreatesMustCallFor("this") + protected void setupConnectionWithConstructor(javax.net.SocketFactory socketFactory) + throws IOException { + // This is the original test case, modified to replace the call to createSocket() with a new + // Socket() call. + // This version succeeded, even before the bug was fixed. + this.socket.close(); + this.socket = new Socket(); + } + + @CreatesMustCallFor("this") + protected void setupConnection2(javax.net.SocketFactory socketFactory) throws IOException { + this.socket.close(); + // This version succeeds, because getSocket is @Pure, so no side-effects can occur. + this.socket = getSocket(socketFactory); + } + + @CreatesMustCallFor("this") + protected void setupConnection3(javax.net.SocketFactory socketFactory) throws IOException { + // This version demonstrates a work-around. + Socket s = socketFactory.createSocket(); + this.socket.close(); + this.socket = s; + } + + @Pure + private Socket getSocket(javax.net.SocketFactory socketFactory) throws IOException { + return socketFactory.createSocket(); + } + + @EnsuresCalledMethods(value = "this.socket", methods = "close") + private void closeSocket() throws IOException { + this.socket.close(); + } +} diff --git a/checker/tests/resourceleak/SocketNullOverwrite.java b/checker/tests/resourceleak/SocketNullOverwrite.java new file mode 100644 index 00000000000..2b6a3af93ea --- /dev/null +++ b/checker/tests/resourceleak/SocketNullOverwrite.java @@ -0,0 +1,16 @@ +// Taken from ACSocketTest and put here to ease debugging. + +import java.io.IOException; +import java.net.Socket; + +class SocketNullOverwrite { + void replaceVarWithNull(String address, int port) { + try { + // :: error: required.method.not.called + Socket s = new Socket(address, port); + s = null; + } catch (IOException e) { + + } + } +} diff --git a/checker/tests/resourceleak/StringFromObject.java b/checker/tests/resourceleak/StringFromObject.java new file mode 100644 index 00000000000..bccdc042c19 --- /dev/null +++ b/checker/tests/resourceleak/StringFromObject.java @@ -0,0 +1,20 @@ +// Tests that converting to a String from an object doesn't create +// phantom must call obligations. Taken from some code in Zookeeper that +// caused a false positive. + +import java.util.Map; +import java.util.Map.Entry; + +class StringFromObject { + boolean test(Map map) { + boolean isHierarchical = false; + for (Entry entry : map.entrySet()) { + String key = entry.getKey().toString().trim(); + if (key.startsWith("group") || key.startsWith("weight")) { + isHierarchical = true; + break; + } + } + return isHierarchical; + } +} diff --git a/checker/tests/resourceleak/TryWithResourcesSimple.class b/checker/tests/resourceleak/TryWithResourcesSimple.class new file mode 100644 index 0000000000000000000000000000000000000000..3944ca782dcd2a82f42fd68d10319e000589e4ce GIT binary patch literal 1776 zcmaJ>T~`xV6x~B6xnvkYJ}m+&Ra*jtX{|yFR-{x~Yyy@bUDyXZOm4|YG81PeAb+Jl zpuG6vOIWL{C2xJ`f3kMp42hxDCM$F9-DjV3&)Mf@{`&im*8t|QZeS3T0#gP$Fs&Y0 zff)lgVF`TJiO&rP+)|G(4BW;Yfms74CKWQL9$y-`fvj3F@GAp-m={=&hmo}BB+wOVCI2AlRynTQ_AFw!tIk$YzE+g923cCA7bxZdu+j~XtPvtfHhzp~^wG7RHG6Tv~KvMihU z77qkgOnis$H8f7OJjOUB+w){OwAML@vR$omLyxrsNAZjCkjG z#h+-n{;%=;_d=`tZ0yQtIc{CU_(UdVofAZzeMaGJ7<+%ctsEi>4wYsau6!_?@-vkl zN=cqR@laJF0%;dlJxap*SNTt~i^W2%S`9cvS>$xv|B@}m7cyJ8h5^1f)F;6oZxG1l zYK&H+O->#|d&Px@VLnq0uoIoQj1j(M6!j7GMsWWLI<_ZAj*-Yu*P$Ol&%bKWyU~jz z$tuARSAfBWSH8_D_gx9Ty4o4klmD>je50a0GKcIjj#S>*ycWPuCRT2wfx$>x8nW>}Qnk=M{FD zqf`aj0W9Jv?FygD{(>TQX=8bPDCHif;2BCYLosesi8;=0nRbO7 zrQROW={LA4AOvo_!JwwSWy?C#2;7WOXaaq2nLpW%5^Fd*-?D;4%X3B+=TgK>flqj# wPK7^h%<3u91FacxRF~*gBO};8rRKWfZf?kFPE1X>xo&Wp8y+-4Uk5V(0LyxT5&!@I literal 0 HcmV?d00001 diff --git a/checker/tests/resourceleak/TryWithResourcesSimple.java b/checker/tests/resourceleak/TryWithResourcesSimple.java new file mode 100644 index 00000000000..c53ccdaab94 --- /dev/null +++ b/checker/tests/resourceleak/TryWithResourcesSimple.java @@ -0,0 +1,34 @@ +// A simple test that a try-with-resources socket doesn't issue a false positive. + +import java.io.*; +import java.net.Socket; +import java.nio.channels.*; +import java.util.*; + +class TryWithResourcesSimple { + static void test(String address, int port) { + try (Socket socket = new Socket(address, port)) { + + } catch (Exception e) { + + } + } + + public boolean isPreUpgradableLayout(File oldF) throws IOException { + + if (!oldF.exists()) { + return false; + } + // check the layout version inside the storage file + // Lock and Read old storage file + try (RandomAccessFile oldFile = new RandomAccessFile(oldF, "rws"); + FileLock oldLock = oldFile.getChannel().tryLock()) { + if (null == oldLock) { + throw new OverlappingFileLockException(); + } + oldFile.seek(0); + int oldVersion = oldFile.readInt(); + return false; + } + } +} diff --git a/checker/tests/resourceleak/TwoConstructorsCloseable.java b/checker/tests/resourceleak/TwoConstructorsCloseable.java new file mode 100644 index 00000000000..282ce8d1a1e --- /dev/null +++ b/checker/tests/resourceleak/TwoConstructorsCloseable.java @@ -0,0 +1,19 @@ +// A test case for false positives that I encountered in Zookeeper. + +import java.io.Closeable; + +public class TwoConstructorsCloseable implements Closeable { + public TwoConstructorsCloseable(Object obj) {} + + public TwoConstructorsCloseable() { + this(null); + } + + public void close() {} + + class Derivative extends TwoConstructorsCloseable { + Derivative() { + super(null); + } + } +} diff --git a/checker/tests/resourceleak/UnconnectedSocketAlias.java b/checker/tests/resourceleak/UnconnectedSocketAlias.java new file mode 100644 index 00000000000..55bd1e96897 --- /dev/null +++ b/checker/tests/resourceleak/UnconnectedSocketAlias.java @@ -0,0 +1,14 @@ +// A test case for an interaction between CO and aliasing that could +// lead to a soundness bug if handled wrong. + +import java.net.*; + +class UnconnectedSocketAlias { + void test(SocketAddress sa) throws Exception { + // :: error: required.method.not.called + Socket s = new Socket(); + Socket t = s; + t.close(); + s.connect(sa); + } +} diff --git a/checker/tests/resourceleak/WrapperStream.java b/checker/tests/resourceleak/WrapperStream.java new file mode 100644 index 00000000000..79f187c5193 --- /dev/null +++ b/checker/tests/resourceleak/WrapperStream.java @@ -0,0 +1,10 @@ +// A simple test that must-call as a type annotation fixes the simplest version +// of the wrapper stream problem. + +import java.io.*; + +class WrapperStream { + void test(byte[] buf) { + InputStream is = new ByteArrayInputStream(buf); + } +} diff --git a/checker/tests/resourceleak/WrapperStreamPoly.java b/checker/tests/resourceleak/WrapperStreamPoly.java new file mode 100644 index 00000000000..8a50fdf0917 --- /dev/null +++ b/checker/tests/resourceleak/WrapperStreamPoly.java @@ -0,0 +1,18 @@ +// A simple test that must-call as a type annotation makes it so that so-called +// "polymorphic" streams like DataInputStream and DataOutputStream are treated as +// their constituent stream. + +import java.io.*; +import org.checkerframework.checker.mustcall.qual.Owning; + +class WrapperStreamPoly { + void test_no_close_needed(@Owning ByteArrayInputStream b) { + // b doesn't need to be closed, so neither does this stream. + DataInputStream d = new DataInputStream(b); + } + + // :: error: required.method.not.called + void test_close_needed(@Owning InputStream b) { + DataInputStream d = new DataInputStream(b); + } +} diff --git a/checker/tests/resourceleak/ZookeeperByteBufferInputStream.java b/checker/tests/resourceleak/ZookeeperByteBufferInputStream.java new file mode 100644 index 00000000000..c746a6119d3 --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperByteBufferInputStream.java @@ -0,0 +1,62 @@ +// This is a test that the correct errors are thrown on this class, which is a copy of one defined +// by Zookeeper. Earlier versions of our code to handle this issued many duplicate +// inconsistent.mustcall.subtype +// errors. This test case doesn't actually test for that, but it's useful for debugging and as a +// regression +// test that at least one error is still issued. + +import java.io.*; +import java.nio.ByteBuffer; +import org.checkerframework.checker.mustcall.qual.MustCall; + +@MustCall({}) +// :: error: inconsistent.mustcall.subtype +public class ZookeeperByteBufferInputStream extends InputStream { + + ByteBuffer bb; + + // :: error: super.invocation + public ZookeeperByteBufferInputStream(ByteBuffer bb) { + this.bb = bb; + } + + @Override + public int read() throws IOException { + if (bb.remaining() == 0) { + return -1; + } + return bb.get() & 0xff; + } + + @Override + public int available() throws IOException { + return bb.remaining(); + } + + @Override + public int read(byte[] b, int off, int len) throws IOException { + if (bb.remaining() == 0) { + return -1; + } + if (len > bb.remaining()) { + len = bb.remaining(); + } + bb.get(b, off, len); + return len; + } + + @Override + public int read(byte[] b) throws IOException { + return read(b, 0, b.length); + } + + @Override + public long skip(long n) throws IOException { + if (n < 0L) { + return 0; + } + n = Math.min(n, bb.remaining()); + bb.position(bb.position() + (int) n); + return n; + } +} diff --git a/checker/tests/resourceleak/ZookeeperReport1.java b/checker/tests/resourceleak/ZookeeperReport1.java new file mode 100644 index 00000000000..f00cad7ff77 --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperReport1.java @@ -0,0 +1,71 @@ +// Based on a Zookeeper false positive that requires unconnected socket support. + +import java.io.IOException; +import java.net.Socket; +import java.net.SocketAddress; +import org.checkerframework.checker.mustcall.qual.*; + +class ZookeeperReport1 { + + static int tickTime, initLimit; + + protected static @MustCall({}) Socket createSocket() throws IOException { + Socket sock; + sock = new Socket(); + sock.setSoTimeout(tickTime * initLimit); + return sock; + } + + protected static @MustCall({}) Socket createSocket2() throws IOException { + Socket sock; + sock = createCustomSocket(); + sock.setSoTimeout(tickTime * initLimit); + return sock; + } + + // This is the full version of case 1. + protected static @MustCall({}) Socket createSocket3(boolean b) throws IOException { + Socket sock; + if (b) { + sock = createCustomSocket(); + } else { + sock = new Socket(); + } + sock.setSoTimeout(tickTime * initLimit); + return sock; + } + + private static @MustCall({}) Socket createCustomSocket() { + return new Socket(); + } + + static void use1() throws IOException { + Socket s = createSocket(); + } + + static void use2(SocketAddress endpoint) throws IOException { + // :: error: required.method.not.called + Socket s = createSocket(); + s.connect(endpoint); + } + + static void use3() throws IOException { + Socket s = createSocket2(); + } + + static void use4(SocketAddress endpoint) throws IOException { + // :: error: required.method.not.called + Socket s = createSocket2(); + s.connect(endpoint); + } + + static void use5(boolean b) throws IOException { + Socket s = createSocket3(b); + } + + static void use6(SocketAddress endpoint, boolean b) throws IOException { + // :: error: required.method.not.called + Socket s = createSocket3(b); + s.connect(endpoint); + } +} diff --git a/checker/tests/resourceleak/ZookeeperReport3.java b/checker/tests/resourceleak/ZookeeperReport3.java new file mode 100644 index 00000000000..43907dcc88f --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperReport3.java @@ -0,0 +1,72 @@ +// Based on a Zookeeper false positive that requires unconnected socket support. +// This example is functionally equivalent to example #4, so I included it in this file +// ("createNewServerSocket"). This version of the test is incomplete: the real version +// of this test also requires support for Optional. See ZookeeperTest3WithOptional.java. + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.mustcall.qual.*; + +class ZookeeperReport3 { + + // This is a simpler version of case 3. + ServerSocket createServerSocket_easy( + InetSocketAddress address, boolean portUnification, boolean sslQuorum) { + ServerSocket serverSocket; + try { + serverSocket = new ServerSocket(); + serverSocket.setReuseAddress(true); + serverSocket.bind(address); + return serverSocket; + } catch (IOException e) { + System.err.println("Couldn't bind to " + address.toString() + e); + } + return null; + } + + ServerSocket createServerSocket( + InetSocketAddress address, boolean portUnification, boolean sslQuorum) { + ServerSocket serverSocket; + try { + if (portUnification || sslQuorum) { + serverSocket = new UnifiedServerSocket(portUnification); + } else { + serverSocket = new ServerSocket(); + } + serverSocket.setReuseAddress(true); + serverSocket.bind(address); + return serverSocket; + } catch (IOException e) { + System.err.println("Couldn't bind to " + address.toString() + e); + } + return null; + } + + private ServerSocket createNewServerSocket( + SocketAddress address, boolean portUnification, boolean sslQuorum) throws IOException { + ServerSocket socket; + + if (portUnification) { + System.out.println("Creating TLS-enabled quorum server socket"); + socket = new UnifiedServerSocket(true); + } else if (sslQuorum) { + System.out.println("Creating TLS-only quorum server socket"); + socket = new UnifiedServerSocket(false); + } else { + socket = new ServerSocket(); + } + + socket.setReuseAddress(true); + socket.bind(address); + + return socket; + } + + class UnifiedServerSocket extends ServerSocket { + // A human has to verify that this constructor actually does produce an unconnected socket. + @SuppressWarnings("inconsistent.constructor.type") + public @MustCall({}) UnifiedServerSocket(boolean b) throws IOException { + super(); + } + } +} diff --git a/checker/tests/resourceleak/ZookeeperReport3WithOptional.java b/checker/tests/resourceleak/ZookeeperReport3WithOptional.java new file mode 100644 index 00000000000..d21f746cce0 --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperReport3WithOptional.java @@ -0,0 +1,52 @@ +// Based on a Zookeeper false positive that requires unconnected socket support +// and support for java.util.Optional. + +// @skip-test until Optional is supported. For now, users should use null instead. + +import java.io.*; +import java.net.*; +import java.util.Optional; +import org.checkerframework.checker.mustcall.qual.*; + +class ZookeeperReport3WithOptional { + + // This is a simpler version of case 3. + Optional createServerSocket_easy( + InetSocketAddress address, boolean portUnification, boolean sslQuorum) { + ServerSocket serverSocket; + try { + serverSocket = new ServerSocket(); + serverSocket.setReuseAddress(true); + serverSocket.bind(address); + return Optional.of(serverSocket); + } catch (IOException e) { + System.err.println("Couldn't bind to " + address.toString() + e); + } + return Optional.empty(); + } + + Optional createServerSocket( + InetSocketAddress address, boolean portUnification, boolean sslQuorum) { + ServerSocket serverSocket; + try { + if (portUnification || sslQuorum) { + serverSocket = new UnifiedServerSocket(portUnification); + } else { + serverSocket = new ServerSocket(); + } + serverSocket.setReuseAddress(true); + serverSocket.bind(address); + return Optional.of(serverSocket); + } catch (IOException e) { + System.err.println("Couldn't bind to " + address.toString() + e); + } + return Optional.empty(); + } + + class UnifiedServerSocket extends ServerSocket { + // A human has to verify that this constructor actually does produce an unconnected socket. + public @MustCall({}) UnifiedServerSocket(boolean b) throws IOException { + super(); + } + } +} diff --git a/checker/tests/resourceleak/ZookeeperReport6.java b/checker/tests/resourceleak/ZookeeperReport6.java new file mode 100644 index 00000000000..1f0a0d0c0dd --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperReport6.java @@ -0,0 +1,15 @@ +// Based on a Zookeeper false positive that requires unconnected socket support. + +import java.io.IOException; +import java.nio.channels.SocketChannel; + +class ZookeeperReport6 { + SocketChannel createSock() throws IOException { + SocketChannel sock; + sock = SocketChannel.open(); + sock.configureBlocking(false); + sock.socket().setSoLinger(false, -1); + sock.socket().setTcpNoDelay(true); + return sock; + } +} diff --git a/checker/tests/resourceleak/ZookeeperTernaryCrash.java b/checker/tests/resourceleak/ZookeeperTernaryCrash.java new file mode 100644 index 00000000000..13c07a10328 --- /dev/null +++ b/checker/tests/resourceleak/ZookeeperTernaryCrash.java @@ -0,0 +1,74 @@ +import java.security.cert.CertificateParsingException; +import java.security.cert.X509Certificate; +import java.util.*; +import org.checkerframework.checker.mustcall.qual.MustCall; + +final class ZookeeperTernaryCrash { + + private static List getSubjectAltNames(final X509Certificate cert) { + try { + final Collection> entries = cert.getSubjectAlternativeNames(); + if (entries == null) { + return Collections.emptyList(); + } + final List result = new ArrayList(); + for (List entry : entries) { + // the need to add this annotation is annoying, but it's better than the alternative, + // which would be to prevent boxed primitives from having must-call types at all + // :: warning: cast.unsafe + final Integer type = entry.size() >= 2 ? (@MustCall({}) Integer) entry.get(0) : null; + if (type != null) { + if (type == SubjectName.DNS || type == SubjectName.IP) { + final Object o = entry.get(1); + if (o instanceof String) { + result.add(new SubjectName((String) o, type)); + } else if (o instanceof byte[]) { + // TODO ASN.1 DER encoded form + } + } + } + } + return result; + } catch (final CertificateParsingException ignore) { + return Collections.emptyList(); + } + } + + private static final class SubjectName { + + static final int DNS = 2; + static final int IP = 7; + + private final String value; + private final int type; + + static SubjectName IP(final String value) { + return new SubjectName(value, IP); + } + + static SubjectName DNS(final String value) { + return new SubjectName(value, DNS); + } + + SubjectName(final String value, final int type) { + if (type != DNS && type != IP) { + throw new IllegalArgumentException("Invalid type: " + type); + } + this.value = Objects.requireNonNull(value); + this.type = type; + } + + public int getType() { + return type; + } + + public String getValue() { + return value; + } + + @Override + public String toString() { + return value; + } + } +} diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index be71733f2d5..3679cb4308e 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -3,6 +3,10 @@ Version 3.X.X (? ?, 2021) **User-visible changes:** +The Resource Leak Checker ensures that certain methods are called on an +object before it is de-allocated. By default, it enforces that `close()` is +called on any expression whose compile-time type implements `java.io.Closeable`. + **Implementation details:** Method renamings (the old methods remain but are deprecated): diff --git a/docs/manual/advanced-features.tex b/docs/manual/advanced-features.tex index d4bdc383baa..08f02f01085 100644 --- a/docs/manual/advanced-features.tex +++ b/docs/manual/advanced-features.tex @@ -915,6 +915,9 @@ \item \ahrefloc{index-checker}{Index Checker} for array accesses (see \chapterpageref{index-checker}) +\item + \ahrefloc{resource-leak-checker}{Resource Leak Checker} for ensuring that resources are disposed of properly + (see \chapterpageref{resource-leak-checker}) \item \ahrefloc{regex-checker}{Regex Checker} to prevent use of syntactically invalid regular expressions (see \chapterpageref{regex-checker}) diff --git a/docs/manual/called-methods-checker.tex b/docs/manual/called-methods-checker.tex index d386385a8f5..4f8f1945aa4 100644 --- a/docs/manual/called-methods-checker.tex +++ b/docs/manual/called-methods-checker.tex @@ -6,6 +6,7 @@ of the form ``call method A before method B''. For the purpose of this checker, a method has ``definitely been called'' if it is \emph{invoked}: a method that might never return or that might throw an exception has +a method that might never return or that might throw an exception has definitely been called on every path after the call, including exceptional paths. The Called Methods Checker provides built-in support for one such property: diff --git a/docs/manual/introduction.tex b/docs/manual/introduction.tex index 1e1f9bcb370..21544939a4b 100644 --- a/docs/manual/introduction.tex +++ b/docs/manual/introduction.tex @@ -39,6 +39,9 @@ \item \ahrefloc{called-methods-checker}{Called Methods Checker} for the builder pattern (see \chapterpageref{called-methods-checker}) +\item + \ahrefloc{resource-leak-checker}{Resource Leak Checker} for ensuring that resources are disposed of properly + (see \chapterpageref{resource-leak-checker}) \item \ahrefloc{lock-checker}{Lock Checker} for concurrency and lock errors (see \chapterpageref{lock-checker}) diff --git a/docs/manual/manual.tex b/docs/manual/manual.tex index af4ee2d3342..18ede14896d 100644 --- a/docs/manual/manual.tex +++ b/docs/manual/manual.tex @@ -53,6 +53,7 @@ \input{optional-checker.tex} \input{interning-checker.tex} \input{called-methods-checker} +\input{resource-leak-checker} \input{fenum-checker.tex} \input{tainting-checker.tex} \input{lock-checker.tex} diff --git a/docs/manual/must-call-checker.tex b/docs/manual/must-call-checker.tex index 28505aaecdd..04a0db383f9 100644 --- a/docs/manual/must-call-checker.tex +++ b/docs/manual/must-call-checker.tex @@ -7,6 +7,10 @@ in particular, it does not enforce that the methods are called before objects are de-allocated. The Must Call Checker is intended to be run as a subchecker of another checker. +The primary client +of the Must Call Checker is the Resource Leak Checker (Section~\ref{resource-leak-checker}), +which enforces that every method in a must-call obligation for an expression is called +before that expression is de-allocated. For example, consider a \. This \ might have an obligation to call the \ method, to release an diff --git a/docs/manual/resource-leak-checker.tex b/docs/manual/resource-leak-checker.tex new file mode 100644 index 00000000000..e6bf81e40cf --- /dev/null +++ b/docs/manual/resource-leak-checker.tex @@ -0,0 +1,381 @@ +\htmlhr +\chapterAndLabel{Resource Leak Checker for must-call obligations}{resource-leak-checker} + +The Resource Leak Checker guarantees that the program fulfills every object's +must-call obligations before the object is de-allocated. + +A resource leak occurs when a program does not explicitly dispose of some finite +underlying resource, such as a socket, file descriptor, or database connection. To dispose +of the resource, the program should call some method on an object. +(De-allocating or garbage-collecting the object is not sufficient.) For +example, the program must call \ on every object that implements the +interface \. + +The Resource Leak Checker can check any property of the form ``the programmer +must call each method in a set of methods \emph{M} at least once +on object \emph{O} before \emph{O} is de-allocated''. For resource leaks, +by default \emph{M} is the set containing +\ and \emph{O} is any object that implements the interface +\. You can extend this guarantee to other types and methods +by writing \<@MustCall> or \<@InheritableMustCall> annotations, as described in +Section~\ref{must-call-annotations}. + +The Resource Leak Checker works in three stages: +\begin{enumerate} +\item The Must Call Checker (\chapterpageref{must-call-checker}) + over-approximates each expression's must-call methods as a + \refqualclass{checker/mustcall/qual}{MustCall} type. +\item The Called Methods Checker (\chapterpageref{called-methods-checker}) + under-approximates each expression's definitely-called methods as a + \refqualclass{checker/calledmethods/qual}{CalledMethods} type. +\item When any program element goes out of scope (i.e., it is ready to be + de-allocated), the Resource Leak Checker compares the types + \<@MustCall(\emph{MC})> and \<@CalledMethods(\emph{CM})>. It reports an error + if there exists some method in \emph{MC} that is not in \emph{CM}. +\end{enumerate} + + +\sectionAndLabel{How to run the Resource Leak Checker}{resource-leak-run-checker} + +Run one of these lines: + +\begin{Verbatim} +javac -processor resourceleak MyFile.java ... +javac -processor org.checkerframework.checker.resourceleak.ResourceLeakChecker MyFile.java ... +\end{Verbatim} + +The Resource Leak Checker supports all the command-line arguments + listed in Section~\ref{called-methods-run-checker} for +the Called Methods Checker. + +\sectionAndLabel{Resource Leak Checker annotations}{resource-leak-annotations} + +The Resource Leak Checker relies on the type qualifiers of two other checkers: +the Must Call Checker (Section~\ref{must-call-annotations}) and +the Called Methods Checker (Section~\ref{called-methods-spec}). You might need +to write qualifiers from either type hierarchy. The most common annotations from +these checkers that you might need to write are: + +\begin{description} + +\item[\refqualclasswithparams{checker/mustcall/qual}{MustCall}{String[] value}] +for example on an element with compile-time type \ that might contain a \. +See Section~\ref{must-call-annotations}. + +\item[\refqualclasswithparams{checker/mustcall/qual}{InheritableMustCall}{String[] value}] +on any classes defined in your program that have must-call obligations. See Section~\ref{must-call-on-class}. + +\item[\refqualclass{checker/calledmethods/qual}{EnsuresCalledMethods}] on a method in +your code that fulfills a must-call obligation of one of its parameters or of a field. +See Section~\ref{called-methods-ensurescalledmethods}. + +\end{description} + +The Resource Leak Checker supports annotations that express +aliasing patterns related to resource leaks: + +\begin{description} + +\item[\refqualclass{checker/mustcall/qual}{Owning}] +\item[\refqualclass{checker/mustcall/qual}{NotOwning}] + expresses ownership. When two aliases exist to the same Java object, + \<@Owning> and \<@NotOwning> indicate which of the two is responsible for + fulfilling must-call obligations. For details, see + Section~\ref{resource-leak-ownership}. + +\item[\refqualclass{checker/mustcall/qual}{MustCallAlias}] + represents a ``resource-aliasing'' relationship. Resource aliases are + distinct Java objects that control the same resource(s): + fulfilling the must-call obligations of one object also + fulfills the obligations of the other object. For details, + see Section~\ref{resource-leak-resource-alias}. + +\end{description} + +The Resource Leak Checker also supports an annotation to permit re-assigning +fields or re-opening resources: + +\begin{description} + +\item[\refqualclasswithparams{checker/mustcall/qual}{CreatesMustCallFor}{String value}] + is a declaration annotation that indicates that after a call to a method + with this annotation none of the must-call obligations of the in-scope, owning expression + listed in \ have been met. + In other words, the annotated method ``resets'' the must-call obligations of the expression. + Multiple \<@CreatesMustCallFor> + annotations can be written on the same method. For more details + on how to use this annotation to permit re-assignment of owning + fields or the re-opening of resources, see Section~\ref{resource-leak-createsmustcallfor}. + +\end{description} + + +\sectionAndLabel{Example of how safe resource usage is verified}{resource-leak-example} + +Consider the following example of safe use of a \, in which the comments indicate the +inferred Must Call and Called Methods type qualifiers for \: +\begin{verbatim} +{ + Socket s = null; + // 1. @MustCall({}) @CalledMethodsBottom + try { + s = new Socket(myHost, myPort); + // 2. @MustCall("close") @CalledMethods({}) + } catch (Exception e) { + // do nothing + } finally { + if (s != null) { + s.close(); + // 3. @MustCall("close") @CalledMethods("close") + } else { + // do nothing + // 4. @MustCall("close") @CalledMethodsBottom + } + // 5. @MustCall("close") @CalledMethods("close") + } + // 6. @MustCall("close") @CalledMethods("close") +} +\end{verbatim} + +At point 1, \'s type qualifiers are the type qualifiers of \: +\ has no must-call obligations (\<@MustCall(\{\})>), +and methods cannot be called on it (\<@CalledMethodsBottom>). + +At point 2, \ is a new \ object, which +has a must-call obligation (\<@MustCall("close")>) +and has had no methods called on it (\<@CalledMethods(\{\})>). + +At point 3, \ has definitely been called on \, so +\'s Called Methods type is updated. Note that the Must Call type +does not change. + +At point 4, \ is definitely \ and its type is adjusted accordingly. + +At point 5, \'s type is the least upper bound of the types at points 3 +and 4. + +At point 6, \ goes out of scope. The Resource Leak Checker reports a +\ error if the Must Call set contains any +element that the Called Methods set does not. + + +\sectionAndLabel{Aliased references and ownership transfer}{resource-leak-ownership} + +Resource leak checking is complicated by aliasing. Multiple expressions +may evaluate to the same Java object, but each object only needs to be +closed once. (Section~\ref{resource-leak-resource-alias} describes a +related situation called ``resource aliasing'', when multiple Java objects +refer to the same underlying resource.) + +For example, consider the following code that safely closes a \: + +\begin{verbatim} + void example(String myHost, int myPort) { + Socket s = new Socket(myHost, myPort); + closeSocket(s); + } + void closeSocket(@Owning @MustCall("close") Socket t) { + t.close(); + } +\end{verbatim} + +There are two aliases for a socket object: \ in \ and \ in +\. Ordinarily, the Resource Leak Checker requires that +\ is called on every expression of type \, but that is not +necessary here. The Resource Leak Checker should not warn when +\ goes out of scope in \, because \ takes ownership +of the socket --- that is, \ takes responsibility for closing +it. The \<@Owning> annotation on \'s declaration expresses this fact; it +tells the Resource Leak Checker that \ is the reference that must be +closed, and its alias \ need not be closed. + +Constructor returns are always \<@Owning>. +Method returns default to \<@Owning>, +and parameters and fields default to \<@NotOwning>. This treatment of parameter and +return types ensures sound handling of unannotated third-party libraries: any +object returned from such a library will be tracked by default, and the checker +never assumes that passing an object to an unannotated library will satisfy its obligations. + +\<@Owning> and \<@NotOwning> always \emph{transfer} must-call obligations: must-call +obligations are conserved (i.e., neither created nor destroyed) by ownership annotations. +Writing \<@Owning> or \<@NotOwning> can never make the checker +unsound: a real warning can never be hidden by them. +As with any annotation, incorrect or missing annotations can lead to false positive warnings. + + +\subsectionAndLabel{Owning fields}{resource-leak-owning-fields} + +Unannotated fields are treated as non-owning. + +The Resource Leak Checker treats all \textbf{static fields} as non-owning, meaning that no assignment to one +can satisfy a must-call obligation. + +For \textbf{final, non-static owning fields}, +the Resource Leak Checker enforces the ``resource acquisition is +initialization (RAII)'' programming idiom. Some +destructor-like method \ must satisfy the field's must-call obligation +(and this fact must be expressed via a \<@EnsuresCalledMethods> annotation on \), +and the enclosing class must have a \<@MustCall("d")> obligation to +ensure the destructor is called. + +\textbf{Non-final, non-static owning fields} usually require one or more \<@CreatesMustCallFor> annotations +when they might be re-assigned. See Section~\ref{resource-leak-createsmustcallfor} for +more details on how to annotate a non-final, non-static owning field. + + +\sectionAndLabel{Resource aliasing}{resource-leak-resource-alias} + +A \emph{resource alias} set is a set of Java objects that +correspond to the same underlying system resource. +Calling a must-call method on any member of a resource-alias set +fulfills that obligation for all members of the set. +Members of the set may have different Java types. + +Programmers most often encounter resource aliasing when using \emph{wrapper types}. +For example, the Java \ wrapper adds buffering to a +delegate stream. +The wrapper's \ method invokes \ on the delegate. Calling +\ on either object has the same effect: it closes the underlying resource. + +A resource aliasing relationship is expressed in source code via a pair of \<@MustCallAlias> annotations: +one on a parameter of a method or constructor, and another on its return type. +For example, the annotated JDK contains this constructor of \: +\begin{Verbatim} +@MustCallAlias BufferedOutputStream(@MustCallAlias OutputStream out); +\end{Verbatim} + +When a pair of \<@MustCallAlias> annotations is written on a method or constructor \'s return type +and its parameter \

, the Resource Leak Checker requires one of the following: +\begin{enumerate} +\item \

is passed to another method or constructor (including \) in a + \<@MustCallAlias> position, and \ returns that method's result, or +\item \

is stored in an \<@Owning> field of the enclosing class. +\end{enumerate} + + +\sectionAndLabel{Creating obligations (how to re-assign a non-final owning field)}{resource-leak-createsmustcallfor} + +Consider a class that has must-call obligations; that is, the class +declaration is annotated with \<@MustCall(...)>. +Every constructor implicitly creates obligations for the newly-created object. +Non-constructor methods may also create obligations +when re-assigning non-final owning fields or allocating +new system-level resources. + +A post-condition annotation, +\<@CreatesMustCallFor>, +indicates for which expression an obligation is created. +If you write \<@CreatesMustCallFor(>\emph{T}\<)> on a method \emph{N} that +overrides a method \emph{M}, then \emph{M} must also be annotated as +\<@CreatesMustCallFor(>\emph{T}\<)>. (\emph{M} may also have other +\<@CreatesMustCallFor> annotations that \emph{N} does not.) + +\<@CreatesMustCallFor> allows the Resource Leak Checker to verify uses of non-final fields +that contain a resource, even if they are re-assigned. Consider +the following example: + +\begin{verbatim} + @MustCall("close") // default qualifier for uses of SocketContainer + class SocketContainer { + private @Owning Socket sock; + + public SocketContainer() { sock = ...; } + + void close() { sock.close() }; + + @CreatesMustCallFor("this") + void reconnect() { + if (!sock.isClosed()) { + sock.close(); + } + sock = ...; + } + } +\end{verbatim} + +In the lifetime of a \ object, \ +might be re-assigned arbitrarily many times: once at each +call to \. This code is safe, however: \ +ensures that \ is closed before re-assigning it. + +Sections~\ref{resource-leak-createsmustcallfor-callsite} +and~\ref{resource-leak-createsmustcallfor-declaration} +explain how the Resource Leak Checker verifies uses and declarations of +methods annotated with \<@CreatesMustCallFor>. + + +\subsectionAndLabel{Requirements at a call site of a \<@CreatesMustCallFor> method}{resource-leak-createsmustcallfor-callsite} + +At a call site to a method annotated as +\<@CreatesMustCallFor(>\emph{expr}\<)>, the Resource Leak Checker: +\begin{enumerate} +\item + Treats any existing \<@MustCall> obligations of \emph{expr} as \emph{satisfied}, +\item + Creates a fresh obligation to check, as if \emph{expr} was assigned to a newly-allocated + object (i.e. as if \emph{expr} were a constructor result). +\item + Un-refines the type in the Called Methods Checker's type hierarchy for \emph{expr} to + \<@CalledMethods(\{\})>, if it had any other Called Methods type. +\item + Requires that the expression corresponding to \emph{expr} (that is, \emph{expr} + viewpoint-adapted to the method call site) is owned; that is, it is + annotated or defaulted as \<@Owning>. Otherwise, the checker + will issue a \ error at the call-site. You can avoid this + error by extracting \emph{expr} into a new local variable (because + locals are \<@Owning> by default) and replacing all instances of \emph{expr} + in the call with references to the new local variable. +\end{enumerate} + +Treating the obligation before the call as satisfied is sound: the +checker creates a new obligation for calls to \<@CreatesMustCallFor> methods, +and the Must Call Checker (\chapterpageref{must-call-checker}) ensures the +\<@MustCall> type for the target will have a \emph{superset} of any methods +present before the call. Intuitively, calling an \<@CreatesMustCallFor> method +``resets'' the obligations of the target, so whether they were satisfied before +the call or not is irrelevant. + +If an \<@CreatesMustCallFor> +method \emph{n} is invoked within a method \emph{m} that has an \<@CreatesMustCallFor> annotation, +and the \<@CreatesMustCallFor> annotations on \emph{n} and \emph{m} have +the same target---imposing the obligation produced by calling \emph{n} on the caller of \emph{m}---then +the newly-created obligation is treated as satisfied immediately +at the call-site of \emph{n} in the body of \emph{m} (because it is imposed at call-sites of \emph{m} +instead). + + +\subsectionAndLabel{Requirements at a declaration of a \<@CreatesMustCallFor> method}{resource-leak-createsmustcallfor-declaration} + +Any method that re-assigns a non-final, owning field of some object \ +must be annotated \<@CreatesMustCallFor("obj")>. +Other methods may also be annotated with \<@CreatesMustCallFor>. + +The Resource Leak Checker enforces two rules to ensure that re-assignments +to non-final, owning fields (like \ in method \ above) are +sound: +\begin{itemize} +\item any method that re-assigns a non-final, owning field of an object + must be annotated with a \<@CreatesMustCallFor> annotation + that targets that object. +\item when a non-final, owning field $f$ is re-assigned at statement $s$, + at the program point before $s$, $f$'s must-call obligations must have been satisfied. +\end{itemize} +\noindent +The first rule ensures that \ is called after the last call +to \, and the second rule ensures that \ +safely closes \ before re-assigning it. Because the Called Methods Checker +treats calls to an \<@CreatesMustCallFor> method like \ as if the call might +cause arbitrary side-effects, after such a call the only method known to have been +definitely called is the \<@CreatesMustCallFor> method: previous called +methods (including \) do not appear in the \<@CalledMethods> type qualifier. + +% TODO: should this section also include text about unconnected sockets, or is what's here sufficient? + +% LocalWords: de subchecker OutputStream MustCall MustCallUnknown RAII +% LocalWords: PolyMustCall InheritableMustCall MultiRandSelector callsite +% LocalWords: Partitioner CalledMethods AnoLightweightOwnership NotOwning +% LocalWords: AnoResourceAliasing MustCallAlias AnoCreatesMustCallFor +% LocalWords: AcountMustCall EnsuresCalledMethods CreatesMustCallFor expr +% LocalWords: closeSocket destructor'' destructor BufferedOutputStream +% LocalWords: AnoResourceAliases createsmustcallfor SocketContainer +% LocalWords: CalledMethodsBottom