An exceptional condition can circumvent the release of a lock, leading to deadlock. According to the Java API [API 2014]:
A
ReentrantLock
is owned by the thread last successfully locking, but not yet unlocking it. A thread invokinglock
will return, successfully acquiring the lock, when the lock is not owned by another thread.
Consequently, an unreleased lock in any thread will prevent other threads from acquiring the same lock. Programs must release all actively held locks on exceptional conditions. Intrinsic locks of class objects used for method and block synchronization are automatically released on exceptional conditions (such as abnormal thread termination).
This guideline is an instance of FIO04-J. Release resources when they are no longer needed. However, most Java lock objects are not closeable, so they cannot be automatically released using Java 7's try
-with-resources feature.
Noncompliant Code Example (Checked Exception)
This noncompliant code example protects a resource, an open file, by using a ReentrantLock
. However, the method fails to release the lock when an exception occurs while performing operations on the open file. When an exception is thrown, control transfers to the catch
block and the call to unlock()
never executes.
public final class Client { private final Lock lock = new ReentrantLock(); public void doSomething(File file) { InputStream in = null; try { in = new FileInputStream(file); lock.lock(); // Perform operations on the open file lock.unlock(); } catch (FileNotFoundException x) { // Handle exception } finally { if (in != null) { try { in.close(); } catch (IOException x) { // Handle exception } } } } }
Noncompliant Code Example (finally
Block)
This noncompliant code example attempts to rectify the problem of the lock not being released by invoking Lock.unlock()
in the finally
block. This code ensures that the lock is released regardless of whether or not an exception occurs. However, it does not acquire the lock until after trying to open the file. If the file cannot be opened, the lock may be unlocked without ever being locked in the first place.
public final class Client { private final Lock lock = new ReentrantLock(); public void doSomething(File file) { InputStream in = null; try { in = new FileInputStream(file); lock.lock(); // Perform operations on the open file } catch (FileNotFoundException fnf) { // Forward to handler } finally { lock.unlock(); if (in != null) { try { in.close(); } catch (IOException e) { // Forward to handler } } } } }
Compliant Solution (finally
Block)
This compliant solution encapsulates operations that could throw an exception in a try
block immediately after acquiring the lock (which cannot throw). The lock is acquired just before the try
block, which guarantees that it is held when the finally
block executes.
public final class Client { private final Lock lock = new ReentrantLock(); public void doSomething(File file) { InputStream in = null; lock.lock(); try { in = new FileInputStream(file); // Perform operations on the open file } catch (FileNotFoundException fnf) { // Forward to handler } finally { lock.unlock(); if (in != null) { try { in.close(); } catch (IOException e) { // Forward to handler } } } } }
Compliant Solution (Execute-Around Idiom)
The execute-around idiom provides a generic mechanism to perform resource allocation and cleanup operations so that the client can focus on specifying only the required functionality. This idiom reduces clutter in client code and provides a secure mechanism for resource management.
In this compliant solution, the client's doSomething()
method provides only the required functionality by implementing the doSomethingWithFile()
method of the LockAction
interface without having to manage the acquisition and release of locks or the open and close operations of files. The ReentrantLockAction
class encapsulates all resource management actions.
public interface LockAction { void doSomethingWithFile(InputStream in); } public final class ReentrantLockAction { private static final Lock lock = new ReentrantLock(); public static void doSomething(File file, LockAction action) { InputStream in = null; lock.lock(); try { in = new FileInputStream(file); action.doSomethingWithFile(in); } catch (FileNotFoundException fnf) { // Forward to handler } finally { lock.unlock(); if (in != null) { try { in.close(); } catch (IOException e) { // Forward to handler } } } } } public final class Client { public void doSomething(File file) { ReentrantLockAction.doSomething(file, new LockAction() { public void doSomethingWithFile(InputStream in) { // Perform operations on the open file } }); } }
Noncompliant Code Example (Unchecked Exception)
This noncompliant code example uses a ReentrantLock
to protect a java.util.Date
instance—recall that java.util.Date
is thread-unsafe by design.
final class DateHandler { private final Date date = new Date(); private final Lock lock = new ReentrantLock(); // str could be null public void doSomething(String str) { lock.lock(); String dateString = date.toString(); if (str.equals(dateString)) { // ... } // ... lock.unlock(); } }
A runtime exception can occur because the doSomething()
method fails to check whether str
is a null reference, preventing the lock from being released.
Compliant Solution (finally
Block)
This compliant solution encapsulates all operations that can throw an exception in a try
block and releases the lock in the associated finally
block. Consequently, the lock is released even in the event of a runtime exception.
final class DateHandler { private final Date date = new Date(); private final Lock lock = new ReentrantLock(); // str could be null public void doSomething(String str) { lock.lock(); try { String dateString = date.toString(); if (str != null && str.equals(dateString)) { // ... } // ... } finally { lock.unlock(); } } }
The doSomething()
method also avoids throwing a NullPointerException
by ensuring that the string does not contain a null reference.
Risk Assessment
Failure to release locks on exceptional conditions could lead to thread starvation and deadlock.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
LCK08-J | Low | Likely | Low | P9 | L2 |
Automated Detection
Some static analysis tools are capable of detecting violations of this rule.
Tool | Version | Checker | Description |
---|---|---|---|
Parasoft Jtest | 2024.1 | CERT.LCK08.RLF CERT.LCK08.LOCK | Release Locks in a "finally" block Do not abandon unreleased locks |
ThreadSafe | 1.3 | CCE_LK_UNRELEASED_ON_EXN | Implemented |
Related Vulnerabilities
The GERONIMO-2234 issue report describes a vulnerability in the Geronimo application server. If the user single-clicks the keystore portlet, the user will lock the default keystore without warning. This causes a crash and stack trace to be produced. Furthermore, the server cannot be restarted because the lock is never cleared.
14 Comments
Robert Seacord (Manager)
I think we need to add a brief description of the Geronimo vulnerability. Also the link is broken. I found something here:
http://www.mail-archive.com/dev@geronimo.apache.org/msg29606.html
but I'm not sure it is the authoritative reference.
Dhruv Mohindra
Are we retaining the related vulns section for now? The link works for me.
Thomas Hawtin
This issue is more general. I don't see a guideline for general resource acquisition and release.
I'd also like to see a non-compliant example with the common error of including the acquire within the try block, which leads to an unmatched release. A reference to the Execute Around idiom would also be useful.
Dhruv Mohindra
We have a broader guideline FIO06-J. Ensure all resources are properly closed when they are no longer needed. It could use a "execute-around" compliant solution too. FTM, I've added it to this guideline based on your suggestion.
Can you possibly elaborate on the unmatched release problem you state? The following code throws an
IllegalMonitorStateException
and is consequently not sneaky enough so I'm not sure if I'm missing something:Thanks!
Thomas Hawtin
Supposing your doSomething example was called whilst already the (reentrant) lock. Then the outer lock is released.
Also the behaviour of a unmatched release is dependent upon the design (possibly accidental) of the resource.
David Svoboda
We're not sure what you mean by your example. I don't see how Dhruv's doSomething example (in his comment two above this one) can fall into the problem you cite.
The problem you cite seems to occur with code that does: (1) acquire a lock (2) do some code (3) call a try block which (4) (re)acquires the lock and (5) unlocks the lock in its finally block. This rule suggests that the try block should begin immediately after the lock is acquired. (Perhaps we need to make this more explicit). IOW my step 2 should be trivially empty.
A Bishop
I think it is because the examples have the lock handling confounded with the file handling. Both require correct cleanup, but is the file handling important to the illustration of this rule?
David Svoboda
I added a noncompliant code example that unlocks a lock that might not have been locked. That would be unlocky. :)
I also cited FIO04-J as a general rule about releasing resources. (I consider a lock a resource, even if it does not implement Closeable).
And I wordsmithed the text around the examples to be clearer. All the code samples are correct wrt closing the open file.
Diarmuid Healy
It might be more straight forward to state the rule as "always acquire a lock as the first statement of a try { ... } block and always release it as the first statement of a corresponding finally { ... } block.", as this is what most/any compliant solution is going to be like.
While not strictly necessary, it may make the rule more straight forward to follow.
All the compliant solutions use lock.lock() as the last line before entering a try block, which is fine, but it might be simpler and more general to just move this down a line to inside the try block. Regardless of what happens after this point the finally block will execute the first line which would be lock.unlock();
David Svoboda
We don't want to make the rule as specific as you suggest, simply because we wish to allow for other code constructs that ensure locks are released.
yiqing
Hi all, I have some questions about this rule.
For unmatched release, is the below case compliant or noncompliant?
lock()
method itself may throw exception when the number of reentrant locks exceeds the limit. As described in java document:"A Lock implementation may be able to detect erroneous use of the lock, such as an invocation that would cause deadlock, and may throw an (unchecked) exception in such circumstances."
And in source code of
lock()
in ReentrantLock class,throw
statement does exist.So when
lock()
occurs exception, above case will excute unlock() which leads to an unmatched release, right? If so,lock()
should always call outside of try() block. Please correct me if my understanding is wrong, thanks!David Svoboda
The Lock page you cite suggests that implementations may throw unchecked exceptions, and should document what circumstances permit such an exception and the state of the lock. But good defensive programming suggests that you should not try to unlock a lock that may not have been locked....unless there exists a way to know if the lock is acquired. (ReentrantLock provides an isLocked() method, but the Lock() interface does not.)
I have updated the code examples to always lock() outside of the try block, as is recommended by the documentation of ReentrantLock.
yiqing
Thanks for your reply! But after your change, the second "Noncompliant Code Example (finally Block)" is a compliant case now which is the same as the next "Compliant Solution (finally Block)" code. It is recognized as noncompliant because there may be unmatched release issue before your change. Do you mean that it's no need to consider this noncompliant code about unmatched release in this rule?
And for the second question above, I want to confirm with you again. For a compliant case, should it follow that
1) lock() is immediately followed by try and unlock() is the first statement of finally.
or
2) lock() is outside of try and unlock() is in finally.
Thank you!
David Svoboda
Whoops, you're right. Reverted both noncompliant examples, the point was to lock outside the try blocks for just the compliant solutions.