tryLock unreleased resource

Question

The code below has been flagged as a violation by Fortify ("unreleased resource" for the lock)

try {
  if (lock.tryLock(1, TimeUnit.SECONDS)) {
    try {
      //do something
    }
    finally {
       lock.unlock();
   }
}
catch (InterruptedException e) {
  // something
}

Could you please explain why? Should there be a finally statement for the InterruptedException try/catch? I thought that the inner try would handle this case as well.

Is it just the `Lock` interface? – John Vint Mar 15 '16 at 19:08 — John Vint, Mar 15 '16 at 19:08
I think this is a Fortify bug - the code looks good. – Boris the Spider Mar 15 '16 at 19:10 — Boris the Spider, Mar 15 '16 at 19:10
@BoristheSpider I think you're right – John Vint Mar 15 '16 at 19:21 — John Vint, Mar 15 '16 at 19:21

score 4 · Accepted Answer · answered Mar 15 '16 at 19:53

4

The Fortify documentation describes the unreleased resource bug as:

The program can potentially fail to release a system resource.

That's not true for your example. If tryLock returns false or throws an exception, no lock was acquired, the try block isn't entered, and there's nothing to release (so no finally block required in the outer try-block). If tryLock returns true, the inner try block is entered and the lock is released in the finally.

What you have seems identical to how the API documentation recommends you do this:

  Lock lock = ...;
  if (lock.tryLock()) {
      try {
          // manipulate protected state
      } finally {
          lock.unlock();
      }
  } else {
      // perform alternative actions
  }

The only difference is your example uses the override of tryLock that times out, in which case it returns false. So from what's posted this seems ok.

answered Mar 15 '16 at 19:53

Nathan Hughes

94,330
19
181
276

There's another key difference: the local `lock` variable. If OP is referencing a class member, it's possible for the reference to be replaced while the lock is held. – shmosel Oct 26 '17 at 03:41
@shmosel: that’s an interesting point. But if the lock is getting replaced it seems like there are worse problems. – Nathan Hughes Oct 26 '17 at 03:45
I'm not suggesting it is, just that Fortify can't be sure. – shmosel Oct 26 '17 at 03:46
@shmosel: definitely static code analysis has its limits. – Nathan Hughes Oct 26 '17 at 03:49

Justin Lee · Answer 2 · 2021-10-29T03:08:41.500

0

I might have a decent solution:

long timeout = ...;
Lock someLock = ...;
try {
    if (!someLock.tryLock(timeout, TimeUnit.MILLISECONDS)) {
        // TODO: print some error;
        return;
    }
} catch (InterruptedException e) {
    // TODO: print some error;
    return;
}

try{
    // TODO: Some Logic
} finally {
    someLock.unlock();
}

edited Oct 29 '21 at 03:08

answered Oct 14 '21 at 11:53

Justin Lee

800
1
11
22

tryLock unreleased resource

2 Answers2