Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorStephan Herrmann2018-08-30 18:07:35 +0000
committerStephan Herrmann2019-12-23 11:34:33 +0000
commit13f7e07fece96abc7cb29cec2b89b953a4a2e096 (patch)
treed9a5498e9034ad265b047c84566a4133bffdd976
parent3393a221971fc81b11b8c717e2b4d8ec57ce80f0 (diff)
downloadeclipse.jdt.core-13f7e07fece96abc7cb29cec2b89b953a4a2e096.tar.gz
eclipse.jdt.core-13f7e07fece96abc7cb29cec2b89b953a4a2e096.tar.xz
eclipse.jdt.core-13f7e07fece96abc7cb29cec2b89b953a4a2e096.zip
Bug 463320 - [compiler][resource] potential "resource leak" problemI20191223-1800
disappears when local variable inlined Change-Id: Ie0c8c1d4bd9e7e733a5ce3963adbe6189f5aba98
-rw-r--r--org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java100
-rw-r--r--org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java76
-rw-r--r--org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java5
-rw-r--r--org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java3
4 files changed, 172 insertions, 12 deletions
diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
index 5b741c918c..74feba70eb 100644
--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
@@ -53,7 +53,7 @@ private static final String APACHE_DBUTILS_CONTENT = "package org.apache.commons
"}\n";
static {
-// TESTS_NAMES = new String[] { "testBug542707" };
+// TESTS_NAMES = new String[] { "testBug463320" };
// TESTS_NUMBERS = new int[] { 50 };
// TESTS_RANGE = new int[] { 11, -1 };
}
@@ -3088,7 +3088,12 @@ public void testBug368709a() {
"}\n"
},
"----------\n" +
- "1. ERROR in X.java (at line 18)\n" +
+ "1. ERROR in X.java (at line 15)\n" +
+ " return wc.open(getObjectId(), type).openStream();\n" +
+ " ^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
+ "Potential resource leak: \'<unassigned Closeable value>\' may not be closed\n" +
+ "----------\n" +
+ "2. ERROR in X.java (at line 18)\n" +
" return new ObjectStream.Filter(type, size, in);\n" +
" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"Potential resource leak: \'in\' may not be closed at this location\n" +
@@ -5056,7 +5061,7 @@ public void testBug397204() {
Map options = getCompilerOptions();
options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
- runConformTest(
+ runLeakTest(
new String[] {
"HostIdTest.java",
"import java.io.*;\n" +
@@ -5110,6 +5115,13 @@ public void testBug397204() {
" \n" +
"}\n"
},
+ // FIXME: should detect that format returns the same formatter
+ "----------\n" +
+ "1. ERROR in HostIdTest.java (at line 42)\n" +
+ " formatter.format(\"%08x\", hostid);\n" +
+ " ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
+ "Potential resource leak: \'<unassigned Closeable value>\' may not be closed\n" +
+ "----------\n",
options);
}
public void testBug397204_comment4() {
@@ -5703,4 +5715,86 @@ public void testBug486506() {
"----------\n",
options);
}
+public void testBug463320() {
+ Map options = getCompilerOptions();
+ options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+ options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+ runLeakTest(
+ new String[] {
+ "Try17.java",
+ "import java.util.zip.*;\n" +
+ "import java.io.*;\n" +
+ "public class Try17 {\n" +
+ " void potential() throws IOException {\n" +
+ " String name= getZipFile().getName();\n" +
+ " System.out.println(name);\n" +
+ " }\n" +
+ " void definite() throws IOException {\n" +
+ " String name= new ZipFile(\"bla.jar\").getName();\n" +
+ " System.out.println(name);\n" +
+ " }\n" +
+ " void withLocal() throws IOException {\n" +
+ " ZipFile zipFile = getZipFile();\n" +
+ " String name= zipFile.getName();\n" +
+ " System.out.println(name);\n" +
+ " }\n" +
+ "\n" +
+ " ZipFile getZipFile() throws IOException {\n" +
+ " return new ZipFile(\"bla.jar\");\n" +
+ " }\n" +
+ "}"
+ },
+ "----------\n" +
+ "1. ERROR in Try17.java (at line 5)\n" +
+ " String name= getZipFile().getName();\n" +
+ " ^^^^^^^^^^^^\n" +
+ "Potential resource leak: \'<unassigned Closeable value>\' may not be closed\n" +
+ "----------\n" +
+ "2. ERROR in Try17.java (at line 9)\n" +
+ " String name= new ZipFile(\"bla.jar\").getName();\n" +
+ " ^^^^^^^^^^^^^^^^^^^^^^\n" +
+ "Resource leak: \'<unassigned Closeable value>\' is never closed\n" +
+ "----------\n" +
+ "2. ERROR in Try17.java (at line 13)\n" +
+ " ZipFile zipFile = getZipFile();\n" +
+ " ^^^^^^^\n" +
+ "Potential resource leak: \'zipFile\' may not be closed\n" +
+ "----------\n",
+ options);
+}
+public void testBug463320_comment8() {
+ if (this.complianceLevel < ClassFileConstants.JDK1_5) return; // required version of java.nio.file.*
+ Map options = getCompilerOptions();
+ options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+ options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+ runLeakTest(
+ new String[] {
+ "Try17.java",
+ "import java.io.*;\n" +
+ "import java.nio.file.*;\n" +
+ "import java.net.*;\n" +
+ "public class Try17 {\n" +
+ " public InputStream openInputStream(URI uri) {\n" +
+ " try {\n" +
+ " System.out.println(FileSystems.getFileSystem(uri));\n" +
+ " return Files.newInputStream(Paths.get(uri));\n" +
+ " } catch (FileSystemNotFoundException e) {\n" +
+ " throw new IllegalArgumentException(e);\n" +
+ " } catch (IOException e) {\n" +
+ " throw new IllegalStateException(e);\n" +
+ " }\n" +
+ " }\n" +
+ " public InputStream delegateGet(URI uri) {\n" +
+ " return openInputStream(uri);\n" + // no problem here!
+ " }\n" +
+ "}"
+ },
+ "----------\n" +
+ "1. ERROR in Try17.java (at line 7)\n" +
+ " System.out.println(FileSystems.getFileSystem(uri));\n" +
+ " ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
+ "Potential resource leak: \'<unassigned Closeable value from line 7>\' may not be closed\n" +
+ "----------\n",
+ options);
+}
}
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
index 5736ac48a4..debe6b0423 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
@@ -81,6 +81,7 @@ public class FakedTrackingVariable extends LocalDeclaration {
private static final int REPORTED_DEFINITIVE_LEAK = 64;
// a local declarations that acts as the element variable of a foreach loop (should never suggest to use t-w-r):
private static final int FOREACH_ELEMENT_VAR = 128;
+ public MessageSend acquisition;
public static boolean TEST_372319 = false; // see https://bugs.eclipse.org/372319
@@ -215,7 +216,10 @@ public class FakedTrackingVariable extends LocalDeclaration {
} else if (expression instanceof AllocationExpression) {
// return any preliminary tracking variable from analyseCloseableAllocation
return ((AllocationExpression) expression).closeTracker;
- }
+ } else if (expression instanceof MessageSend) {
+ // return any preliminary tracking variable from analyseCloseableAcquisition
+ return ((MessageSend) expression).closeTracker;
+ }
return null;
}
@@ -247,6 +251,16 @@ public class FakedTrackingVariable extends LocalDeclaration {
closeTracker.currentAssignment = location;
preConnectTrackerAcrossAssignment(location, local, flowInfo, closeTracker, rhs);
}
+ } else if (rhs instanceof MessageSend) {
+ closeTracker = local.closeTracker;
+ if (closeTracker != null) {
+ handleReassignment(flowInfo, closeTracker, location);
+ }
+ if (rhs.resolvedType != TypeBinding.NULL) { // not NULL means valid closeable as per method precondition
+ closeTracker = new FakedTrackingVariable(local, location, flowInfo, null, FlowInfo.UNKNOWN);
+ closeTracker.currentAssignment = location;
+ ((MessageSend) rhs).closeTracker = closeTracker;
+ }
}
}
@@ -382,6 +396,38 @@ public class FakedTrackingVariable extends LocalDeclaration {
}
}
+ /**
+ * Check if a message send acquires a closeable from its receiver, see:
+ * Bug 463320 - [compiler][resource] potential "resource leak" problem disappears when local variable inlined
+ */
+ public static FlowInfo analyseCloseableAcquisition(BlockScope scope, FlowInfo flowInfo, MessageSend acquisition) {
+ // client has checked that the resolvedType is an AutoCloseable, hence the following cast is safe:
+ if (((ReferenceBinding)acquisition.resolvedType).hasTypeBit(TypeIds.BitResourceFreeCloseable)) {
+ // remove unnecessary attempts (closeable is not relevant)
+ if (acquisition.closeTracker != null) {
+ acquisition.closeTracker.withdraw();
+ acquisition.closeTracker = null;
+ }
+ return flowInfo;
+ } else { // regular resource
+ FakedTrackingVariable tracker = acquisition.closeTracker;
+ if (tracker != null) {
+ // pre-connected tracker means: directly assigning the acquisition to a local, forget special treatment:
+ tracker.withdraw();
+ acquisition.closeTracker = null;
+ return flowInfo;
+ } else {
+ tracker = new FakedTrackingVariable(scope, acquisition, flowInfo, FlowInfo.UNKNOWN); // no local available, closeable is unassigned
+ acquisition.closeTracker = tracker;
+ }
+ tracker.acquisition = acquisition;
+ FlowInfo outsideInfo = flowInfo.copy();
+ outsideInfo.markAsDefinitelyNonNull(tracker.binding);
+ flowInfo.markAsDefinitelyNull(tracker.binding);
+ return FlowInfo.conditional(outsideInfo, flowInfo);
+ }
+ }
+
private static FakedTrackingVariable pickMoreUnsafe(FakedTrackingVariable tracker1, FakedTrackingVariable tracker2, BlockScope scope, FlowInfo info) {
// whichever of the two trackers has stronger indication to be leaking will be returned,
// the other one will be removed from the scope (considered to be merged into the former).
@@ -404,18 +450,22 @@ public class FakedTrackingVariable extends LocalDeclaration {
if (presetTracker != null && presetTracker.originalBinding != null) {
// the current assignment forgets a previous resource in the LHS, may cause a leak
// report now because handleResourceAssignment can't distinguish this from a self-wrap situation
- int closeStatus = flowInfo.nullStatus(presetTracker.binding);
- if (closeStatus != FlowInfo.NON_NULL // old resource was not closed
- && closeStatus != FlowInfo.UNKNOWN // old resource had some flow information
- && !flowInfo.isDefinitelyNull(presetTracker.originalBinding) // old resource was not null
- && !(presetTracker.currentAssignment instanceof LocalDeclaration)) // forgetting old val in local decl is syntactically impossible
- allocation.closeTracker.recordErrorLocation(presetTracker.currentAssignment, closeStatus);
+ handleReassignment(flowInfo, presetTracker, presetTracker.currentAssignment);
} else {
allocation.closeTracker = new FakedTrackingVariable(scope, allocation, flowInfo, FlowInfo.UNKNOWN); // no local available, closeable is unassigned
}
flowInfo.markAsDefinitelyNull(allocation.closeTracker.binding);
}
+ private static void handleReassignment(FlowInfo flowInfo, FakedTrackingVariable existingTracker, ASTNode location) {
+ int closeStatus = flowInfo.nullStatus(existingTracker.binding);
+ if (closeStatus != FlowInfo.NON_NULL // old resource was not closed
+ && closeStatus != FlowInfo.UNKNOWN // old resource had some flow information
+ && !flowInfo.isDefinitelyNull(existingTracker.originalBinding) // old resource was not null
+ && !(location instanceof LocalDeclaration)) // forgetting old val in local decl is syntactically impossible
+ existingTracker.recordErrorLocation(location, closeStatus);
+ }
+
/** Find an existing tracking variable for the argument of an allocation for a resource wrapper. */
private static FakedTrackingVariable findCloseTracker(BlockScope scope, FlowInfo flowInfo, Expression arg)
{
@@ -482,7 +532,7 @@ public class FakedTrackingVariable extends LocalDeclaration {
rhsTrackVar.globalClosingState &= ~(SHARED_WITH_OUTSIDE|OWNED_BY_OUTSIDE|FOREACH_ELEMENT_VAR);
}
} else {
- if (rhs instanceof AllocationExpression || rhs instanceof ConditionalExpression || rhs instanceof SwitchExpression) {
+ if (rhs instanceof AllocationExpression || rhs instanceof ConditionalExpression || rhs instanceof SwitchExpression || rhs instanceof MessageSend) {
if (rhsTrackVar == disconnectedTracker)
return; // b.: self wrapper: res = new Wrap(res); -> done!
if (local.closeTracker == rhsTrackVar
@@ -645,6 +695,12 @@ public class FakedTrackingVariable extends LocalDeclaration {
tracker.withdraw();
((AllocationExpression) expression).closeTracker = null;
}
+ } else if (expression instanceof MessageSend) {
+ FakedTrackingVariable tracker = ((MessageSend) expression).closeTracker;
+ if (tracker != null && tracker.originalBinding == null) {
+ tracker.withdraw();
+ ((AllocationExpression) expression).closeTracker = null;
+ }
} else {
// assignment passing a local into a field?
LocalVariableBinding local = expression.localVariableBinding();
@@ -1003,6 +1059,10 @@ public class FakedTrackingVariable extends LocalDeclaration {
if (isPotentialProblem) {
if ((this.globalClosingState & (REPORTED_POTENTIAL_LEAK|REPORTED_DEFINITIVE_LEAK)) != 0)
return 0;
+// if ((this.globalClosingState & (ACQUIRED_FROM_OUTSIDE)) != 0
+// && location instanceof ReturnStatement
+// && ((ReturnStatement) location).expression == this.acquisition)
+// return 0; // directly returning a resource acquired from a message send: don't assume responsibility
problemReporter.potentiallyUnclosedCloseable(this, location);
} else {
if ((this.globalClosingState & (REPORTED_DEFINITIVE_LEAK)) != 0)
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
index 290f5cc15e..b7a2ae094d 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
@@ -139,6 +139,7 @@ public class MessageSend extends Expression implements IPolyExpression, Invocati
public TypeBinding[] argumentTypes = Binding.NO_PARAMETERS;
public boolean argumentsHaveErrors = false;
+ public FakedTrackingVariable closeTracker;
@Override
public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, FlowInfo flowInfo) {
@@ -238,6 +239,10 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
// checkExceptionHandlers; consider protecting there instead of here;
// NullReferenceTest#test0510
}
+ // after having analysed exceptions above start tracking newly allocated resource:
+ if (analyseResources && FakedTrackingVariable.isAnyCloseable(this.resolvedType))
+ flowInfo = FakedTrackingVariable.analyseCloseableAcquisition(currentScope, flowInfo, this);
+
manageSyntheticAccessIfNecessary(currentScope, flowInfo);
// account for pot. exceptions thrown by method execution
flowContext.recordAbruptExit();
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
index d0bb1be1ef..4f35cb3a01 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
@@ -9887,7 +9887,8 @@ public void redundantSpecificationOfTypeArguments(ASTNode location, TypeBinding[
}
public void potentiallyUnclosedCloseable(FakedTrackingVariable trackVar, ASTNode location) {
String[] args = { trackVar.nameForReporting(location, this.referenceContext) };
- if (location == null) {
+ if (location == null || trackVar.acquisition != null) {
+ // if acquisition is set, the problem is not location specific
this.handle(
IProblem.PotentiallyUnclosedCloseable,
args,

Back to the top