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 invoking lock 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.

ToolVersionCheckerDescription
Parasoft Jtest
2023.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.

Related Guidelines

Bibliography



14 Comments

  1. 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.

    1. Are we retaining the related vulns section for now? The link works for me.

  2. 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.

    1. 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:

      public final class Client {
        public void doSomething(File file) {
          final Lock lock = new ReentrantLock();
          try {      
            InputStream in = new FileInputStream(file); // throws exception
            lock.lock();
            // Perform operations on the open file      
          } catch (FileNotFoundException fnf) {
            // Handle the exception
          } finally {
            lock.unlock(); // unmatched release, throws exception
          }
        }
      }
      

      Thanks!

      1. 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.

        1. 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.

          1. 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?

            1. 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.

  3. 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();

    1. 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.


  4. Hi all, I have some questions about this rule.

    1. For unmatched release, is the below case compliant or noncompliant?

      class Client {
          private final Lock lock = new ReentrantLock();
      
          // "lock()" itself may throw exception when the number of reentrant locks exceeds the limit.
          public void doSomething1(File file) {
              InputStream in = null;
              try {
                  lock.lock(); // throws exception, lock() failed
                  in = new FileInputStream(file);
              } catch (FileNotFoundException fnf) {
      
              } finally {
                  lock.unlock(); // unmatched release, throws exception
                  if (in != null) {
                      try {
                          in.close();
                      } catch (IOException e) {
      
                      }
                  }
              }
          }
      }

      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.

              final boolean nonfairTryAcquire(int acquires) {
                  final Thread current = Thread.currentThread();
                  int c = getState();
                  if (c == 0) {
                      if (compareAndSetState(0, acquires)) {
                          setExclusiveOwnerThread(current);
                          return true;
                      }
                  }
                  else if (current == getExclusiveOwnerThread()) {
                      int nextc = c + acquires;
                      if (nextc < 0) // overflow
                          throw new Error("Maximum lock count exceeded");  // throw exception here
                      setState(nextc);
                      return true;
                  }
                  return false;
              }

      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!



    2.  ReentrantLock document suggests that it is recommended practice to always immediately follow a call to lock with a try block. And all compliant cases in this wiki follows this. And unlock() is always placed in the first statement of the finally. Should we follow these two principles to achieve this rule?
    1. 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.

      1. 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!

        1. Whoops, you're right. Reverted both noncompliant examples, the point was to lock outside the try blocks for just the compliant solutions.