Bugzilla – Full Text Bug Listing
| Summary: | [compiler][resources] Incorrect Errors/Warnings check for potential resource leak when surrounding with try-catch | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Leemax Li <leemax.chronicle> |
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | jarthana, manoj.palat, max.gilead |
| Version: | 4.2.1 | ||
| Target Milestone: | 4.5 M7 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| See Also: |
https://git.eclipse.org/r/45263 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=0747d5e7c6159c917fde8175fc9c67a8bade55fc |
||
| Whiteboard: | |||
Stephan, please take a look. Observations: The null pointer warning is correct, saying just that bw can be null at the site of dereference. We never promised that the analysis included the flow of NPEs after they have been thrown. We can't give such promise because NPE is an uncheck exception. Ergo: catching an NPE after it was thrown does not help to avoid the warning that an NPE may occur. The code is buggy because in the good case, where no exception is thrown, we never see a call to close(). So the warnings remind you to add a finally block to your code where you call close(). The only thing that I think needs fixing is what warning/error we report: saying "... is never closed" is to harsh, the compiler should see that on one (however unlikely) path the resource can be closed, so it should say "... may not be closed". Another test case, showing that the warning is *not* issued if the FileOutputStream is constructed with a string argument, which is somewhat surprising in itself.
import java.io.*;
public class Test {
public static void a() throws IOException {
final OutputStream out = new FileOutputStream(new File("bar"));
try {
throw new RuntimeException(); // Potential resource leak: 'out' may not be closed at this location
} catch (final RuntimeException e) {
throw e; // Potential resource leak: 'out' may not be closed at this location
} finally {
out.close();
}
}
public static void b() throws IOException {
final OutputStream out = new FileOutputStream("bar");
try {
throw new RuntimeException();
} catch (final RuntimeException e) {
throw e;
} finally {
out.close();
}
}
}
(In reply to Leemax Li from comment #0) At current head I see "never closed" reported against both 'out' and 'bw', independent of where 'out' is declared. This particular behavior has changed between 4.5M1 (no warning re 'out' if declared ouside the try) and 4.5M2. (In reply to Max Gilead from comment #3) > Another test case, showing that the warning is *not* issued if the > FileOutputStream is constructed with a string argument, which is somewhat > surprising in itself. Indeed looks suspicious. While I'm not sure both issues are one and the same, I hope I'll be able to fix all within the 4.5 cycle. (In reply to Stephan Herrmann from comment #4) > (In reply to Max Gilead from comment #3) > > Another test case, showing that the warning is *not* issued if the > > FileOutputStream is constructed with a string argument, which is somewhat > > surprising in itself. > > Indeed looks suspicious. Fixed by the change for bug 421035. At 1.7 we now only report: ---------- 1. WARNING in /tmp/Test2.java (at line 4) final OutputStream out = new FileOutputStream(new File("bar")); ^^^ Resource 'out' should be managed by try-with-resource ---------- 2. WARNING in /tmp/Test2.java (at line 14) final OutputStream out = new FileOutputStream("bar"); ^^^ Resource 'out' should be managed by try-with-resource ---------- (In reply to Leemax Li from comment #0) > When set: > 1. "Errors/Warnings" --> "potential programming problems" --> "potential > resource leak" > 2. "Errors/Warnings" --> "Null analysis" --> "potential null pointer access" > both to: warning or error. > > The following code always produce 2 warning/error which are incorrect: > > snippet (1/3): > OutputStream out = null; > BufferedWriter bw = null; > try { > // code... > out = new FileOutputStream(myFile); > OutputStreamWriter writer = new OutputStreamWriter(out); > bw = new BufferedWriter(writer); > // more code... > } catch (Exception e) { > try { > bw.close(); // WARN: potential null pointer access > } catch (Exception ignored) {} > return; // WARN: resource leak - bw may not be closed > } In HEAD this reports: ---------- 1. WARNING in /tmp/Bug396575.java (at line 11) bw = new BufferedWriter(writer); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Resource 'bw' should be managed by try-with-resource ---------- 2. WARNING in /tmp/Bug396575.java (at line 11) bw = new BufferedWriter(writer); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Resource leak: 'bw' is never closed ---------- 3. WARNING in /tmp/Bug396575.java (at line 15) bw.close(); // WARN: potential null pointer access ^^ Potential null pointer access: The variable bw may be null at this location ---------- All this looks good to me ("never" is true for normal flows, not true for exceptional flows, but I hold that this is acceptable). > but seems the "Potential Null Pointer" is a bug to me since i have wrapped > it in a try-catch clause which will capture "Exception" that the null > pointer exception should be included. Null pointer analysis has nothing to do with catching NPE, but with preventing it from occurring in the first place. > And there's a variant for "resource leak": > if declare "OutputStream out" inside the try-catch, the warning will show > that the "out" is never closed. > > // OutputStream out = null; // not declare the out here > BufferedWriter bw = null; > try { > // code... > // declare "out" here > inside try-catch as a temp variable > OutputStream out = new FileOutputStream(myFile); // WARN: out is > never closed. > OutputStreamWriter writer = new OutputStreamWriter(out); > bw = new BufferedWriter(writer); > // more code... > } catch (Exception e) { > try { > bw.close(); // WARN: potential null pointer access > } catch (Exception ignored) {} > return; // WARN: resource leak - bw may not be closed > } > > no matter how code is organized in "catch" clause, the out will always > produce a "never closed" resource leak. In HEAD: similar to above, plus one more: ---------- 4. WARNING in /tmp/Bug396575.java (at line 26) OutputStream out = new FileOutputStream(myFile); // WARN: out is never closed. ^^^ Resource leak: 'out' is never closed ---------- This one still looks suspicious, because 'out' is wrapped in 'bw' and hence we don't want to complain against the inner, only the outer. I suspect that this is an artifact of how the analysis is implemented scope-based: we need to decide about 'out' when analysis comes to the end of the try block, but checks for 'bw' are still pending until the end of the method (because 'bw' is declared in the toplevel scope of the method). New Gerrit change created: https://git.eclipse.org/r/45263 It turned out the remaining problem could indeed be traced back to the order of analysing resources (in BlockScope.checkUnclosedCloseables()). It still happened that we performed the checks for an inner resource "out" before we had a chance to look at its (transitive) wrapper "bw". In particular it could happen that we removed "writer" from the set, given it is wrapped by "bw" and then "out" didn't have a matching wrapper within the set anymore. Resolved by rewriting the iteration logic into a proper Iterator, recognizing that the previous attempt was growing too complex, which was due to the idea of a single set iteration per request. The new iterator makes priorities in the search explicit as "Stage"s. Gerrit change https://git.eclipse.org/r/45263 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=0747d5e7c6159c917fde8175fc9c67a8bade55fc . Verified for Eclipse 4.5 Mars M7 Build id: I20150422-1000 |
When set: 1. "Errors/Warnings" --> "potential programming problems" --> "potential resource leak" 2. "Errors/Warnings" --> "Null analysis" --> "potential null pointer access" both to: warning or error. The following code always produce 2 warning/error which are incorrect: snippet (1/3): OutputStream out = null; BufferedWriter bw = null; try { // code... out = new FileOutputStream(myFile); OutputStreamWriter writer = new OutputStreamWriter(out); bw = new BufferedWriter(writer); // more code... } catch (Exception e) { try { bw.close(); // WARN: potential null pointer access } catch (Exception ignored) {} return; // WARN: resource leak - bw may not be closed } for the resource leak, seems have been verified fixed in 4.3M3?5? ??? but seems the "Potential Null Pointer" is a bug to me since i have wrapped it in a try-catch clause which will capture "Exception" that the null pointer exception should be included. And there's a variant for "resource leak": if declare "OutputStream out" inside the try-catch, the warning will show that the "out" is never closed. // OutputStream out = null; // not declare the out here BufferedWriter bw = null; try { // code... // declare "out" here inside try-catch as a temp variable OutputStream out = new FileOutputStream(myFile); // WARN: out is never closed. OutputStreamWriter writer = new OutputStreamWriter(out); bw = new BufferedWriter(writer); // more code... } catch (Exception e) { try { bw.close(); // WARN: potential null pointer access } catch (Exception ignored) {} return; // WARN: resource leak - bw may not be closed } no matter how code is organized in "catch" clause, the out will always produce a "never closed" resource leak.