Methods invoked from within a finally block can throw an exception. Failure to catch and handle such exceptions results in the abrupt termination of the entire try block. Abrupt termination causes any exception thrown in the try block to be lost, preventing any possible recovery method from handling that specific problem. Additionally, the transfer of control associated with the exception may prevent execution of any expressions or statements that occur after the point in the finally block from which the exception is thrown. Consequently, programs must appropriately handle checked exceptions that are thrown from within a finally block.
Allowing checked exceptions to escape a finally block also violates ERR04-J. Do not complete abruptly from a finally block.
Noncompliant Code Example
This noncompliant code example contains a finally block that closes the reader object. The programmer incorrectly assumes that the statements in the finally block cannot throw exceptions and consequently fails to appropriately handle any exception that may arise.
public class Operation {
public static void doOperation(String some_file) {
// ... Code to check or set character encoding ...
try {
BufferedReader reader =
new BufferedReader(new FileReader(some_file));
try {
// Do operations
} finally {
reader.close();
// ... Other cleanup code ...
}
} catch (IOException x) {
// Forward to handler
}
}
}
The close() method can throw an IOException, which, if thrown, would prevent execution of any subsequent cleanup statements. This problem will not be diagnosed by the compiler because any IOException would be caught by the outer catch block. Also, an exception thrown from the close() operation can mask any exception that gets thrown during execution of the Do operations block, preventing proper recovery.
Compliant Solution (Handle Exceptions in finally Block)
This compliant solution encloses the close() method invocation in a try-catch block of its own within the finally block. Consequently, the potential IOException can be handled without allowing it to propagate further.
public class Operation {
public static void doOperation(String some_file) {
// ... Code to check or set character encoding ...
try {
BufferedReader reader =
new BufferedReader(new FileReader(some_file));
try {
// Do operations
} finally {
try {
reader.close();
} catch (IOException ie) {
// Forward to handler
}
// ... Other cleanup code ...
}
} catch (IOException x) {
// Forward to handler
}
}
}
Compliant Solution (try-with-resources)
Java SE 7 introduced a feature called try-with-resources that can close certain resources automatically in the event of an error. This compliant solution uses try-with-resources to properly close the file.
public class Operation {
public static void doOperation(String some_file) {
// ... Code to check or set character encoding ...
try ( // try-with-resources
BufferedReader reader =
new BufferedReader(new FileReader(some_file))) {
// Do operations
} catch (IOException ex) {
System.err.println("thrown exception: " + ex.toString());
Throwable[] suppressed = ex.getSuppressed();
for (int i = 0; i < suppressed.length; i++) {
System.err.println("suppressed exception: "
+ suppressed[i].toString());
}
// Forward to handler
}
}
public static void main(String[] args) {
if (args.length < 1) {
System.out.println("Please supply a path as an argument");
return;
}
doOperation(args[0]);
}
}
When an IOException occurs in the try block of the doOperation() method, it is caught by the catch block and printed as the thrown exception. Exceptions that occur while creating the BufferedReader are included. When an IOException occurs while closing the reader, that exception is also caught by the catch block and printed as the thrown exception. If both the try block and closing the reader throw an IOException, the catch clause catches both exceptions and prints the try block exception as the thrown exception. The close exception is suppressed and printed as the suppressed exception. In all cases, the reader is safely closed.
Risk Assessment
Failure to handle an exception in a finally block may have unexpected results.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
|---|---|---|---|---|---|
ERR05-J | Low | Unlikely | Medium | P2 | L3 |
Automated Detection
Tool | Version | Checker | Description |
|---|---|---|---|
| Coverity | 7.5 | PW.ABNORMAL_TERMINATION_ OF_FINALLY_BLOCK | Implemented |
| Parasoft Jtest | 2024.1 | CERT.ERR05.ARCF CERT.ERR05.ATSF | Avoid using 'return's inside 'finally blocks if thare are other 'return's inside the try-catch block Do not exit "finally" blocks abruptly |
| SonarQube | 9.9 | S1163 | Exceptions should not be thrown in finally blocks |
Related Guidelines
CWE-248, Uncaught Exception CWE-460, Improper Cleanup on Thrown Exception CWE-584, Return inside CWE-705, Incorrect Control Flow Scoping CWE-754, Improper Check for Unusual or Exceptional Conditions |
Bibliography
Puzzle 41, "Field and Stream" | |
Section 8.3, "Preventing Resource Leaks (Java)" | |
The |
20 Comments
Fred Long
This needs a second compliant solution using closeIgnoringException().
Dhruv Mohindra
Addressed I guess.
David Svoboda
We should consider merging this rule with ERR04-J. Do not exit abruptly from a finally block
Robert Seacord (Manager)
rewrote the intro paragraph; please review for correctness.
Robert Seacord (Manager)
I'm a bit concerned that the compliant solution for Compliant Solution (Java 1.7: try-with-resources) is noncompliant with respect to ERR00-J. Do not suppress or ignore checked exceptions but still shown in blue. This violates are rule that all compliant examples must be compliant with all other rules.
Is there an easy way to make the compliant solution compliant with this other rule? For example, I don't see where
pathis defined even though all ofmainis shown. Can the CS loop back to get a new path name?David Svoboda
Addressed. The 'path' was a typo. The point of this code sample is to show how try-with-resources handles multiple exceptions, which is why the print statements are appropriate (in spite of ERR00-J).
Robert Seacord (Manager)
I think that argument is going in the wrong direction. That suggests also that if the point of your program is to demonstrate a buffer overflow, then a program with a buffer overflow is a compliant solution.
My concern is that compliant solutions should be exemplars of secure programming, so that they can be copy/pasted into real systems.
I made a small change that isn't ideal but perhaps is sufficient. This example would be helped if the printf statements were for debugging only (System.err?)
Thomas Hawtin
I don't think there is anything wrong with throwing an exception from a finally block, whether checked or not. You really need to avoid closing more than one resource in a finally block. The path of execution should be as an exception in the unlikely event of an error disposing a resource.
"try-with-resources" arranges that the innermost exception is thrown, but it will still throw checked exceptions from resource disposal.
Output streams are somewhere tricky. FilterOutputStream.close flushes first. It will ignore the exception from the flush, which you don't want but cant be changed. So, I would suggest flushing the stream at the end of the try block, so that any exception in the otherwise happy case is noted. Also, the underlying resource can be closed rather than the decorator.
The compliant code has a bit of a gap between the resource being allocated and the try block. Some people are fine with that, but I'd put the decorators within the try block. Also, FileReader implicitly picks up whatever character encoding happens to be configured, rather than the choice being explicit.
A complication is the likes of ZipInputStream (I think), which in the Sun/Oracle implementation has resources of its own (C heap allocations) which should be disposed of.
David Svoboda
The danger is having to deal with two exceptions being thrown from the same try/catch/finally statement. If an exception occurs in the try block, and another exception occurs in the finally block, the finally exception gets thrown, and the try exception gets forgotten. This occurs in both Java 1.6 and Java 1.7.
The one exception (pardon the pun) to this fact is the try-with-resources construct in Java 1.7, where one exception gets thrown, but it provides access to the other exception via the getSuppressed() method. This method doesn't exist in earlier Javas. Also it is not used in normal try/catch/finally blocks; it is only used in try-with-resources.
Also, we now permit exceptions that occur during a file close to be ignored, as per EX0 of ERR00-J. Do not suppress or ignore checked exceptions.
BTW I killed the first NCCE, it violated ERR00-J and provided nothing useful.
Agreed. That 'throws IOException' was masking the fact that the file open occurs outside the try block. I moved it in all the code samples (except the try-with-resources, obviously
Dean Sutherland
Added a comment to each code sample noting the (elided) presence of code to check or set the character encoding. This should be a sufficient bow in the direction of IDS17-J.
Dhruv Mohindra
Suggestions:
More about the last CS:
Dhruv Mohindra
I also think it would be better to clarify that any exception in try that has the same type as any exception in the finally block will result in "exception swallowing". Such as read and write methods for IO also throw IOException and so does the IO stream classes (Bloch puzzle 41). Bloch also suggests using the Closeable interface (instead of the dedicated method that we have listed as 2nd CS):
private static void closeIgnoringException(Closeable c) { if (c != null) { try { c.close(); } catch (IOException ex) { // There is nothing we can do if close fails } } }This rule is not about abrupt termination of finally block but a reference to that rule will be useful.
David Svoboda
removed
can, but why bother?
Yes it is, it illustrates how to catch suppressed exceptions.
s/error/exception/g in that paragraph
I'm not convinced that either suggestion will make the rule better. We could argue all day about how much or little info to include without reaching a conclusion.
The rule already does that, although s/swallow/ignore/g;
Modified 2nd CS as suggested
Added.
Thomas Hawtin
If an operation fails, the failure should generally be reported by throwing an exception. All this "// Forward to handler" stuff is not the right way to handle the situation (except, perhaps, for failing upcalls).
David Svoboda
Well, first of all, we recommend that you not catch exceptions you are unprepared to handle. So when you see 'forward to handler', it's inside a catch clause, which means that we do not wish to (or cannot) throw the exception further, for various reasons.
Second, the 'forward to handler' comments are actually referring to the Exception Reporter pattern promoted by ERR00-J. Do not suppress or ignore checked exceptions. I suppose that could be made more clear; if we want to standardize the wording we should agree on something and have it applied globally to every catch clause that has 'nonstandard' wording now.
Pamela Curtis
I changed "because IOExceptions are caught" to "because any IOException would be caught" in the first paragraph after the first noncompliant example because the s after IOException was causing the code close markup - }} - to fail.
Yozo TODA
I can't understand the meaning of the phrase "The compiler correctly fails to diagnose this problem because ..." in the paragraph after the NCCE.
can it read as "The compiler incorrectly fails to diagnose..." or, simply, "The compiler fails to diagnose..." ?
another point I want to clarify is "the problem".
does it mention about the compiler's failing to alert that the inner
tryblock does not catch the exception?With my current understanding, the phrase should be re-written as "The compiler fails to diagnose that the inner
tryblock does not catch theIOException, because anyIOExceptionwould be caught by the outer catch block."David Svoboda
Tweaked the wording; hopefully clearer now.
James Ahlborn
Curious as to why this section specifically calls out "checked" exceptions? This rule applies to unchecked exceptions as well. While i understand that most RuntimeExceptions in the jdk are often "programmer error" type exceptions, you have frameworks like Hibernate where the primary exception (HibernateException) is a RuntimeException. This rule should apply to all exceptions.
Also, i agree with David Svoboda's earlier comment that this rule is pretty much just another case of ERR04-J.
Yitzhak Mandelbaum
I can't see any reason for distinction between checked and unchecked and +1 on merging with ERR04-J. That said, in an earlier comment Dhruv says:
but doesn't expand on this point. In general, I think that both this rule and ERR04-J do not sufficiently motivate themselves to warrant inclusion in the standard. That's not to say there isn't good motivation, only that it doesn't appear in the text.