Semmle 1.21
Skip to end of metadata
Go to start of metadata

Name: Mutex locked twice

Description: Calling the lock method of a mutex twice in succession might cause a deadlock.

ID: cpp/twice-locked

Kind: problem

Severity: error

Precision: low

Query: TwiceLocked.ql
/**
 * @name Mutex locked twice
 * @description Calling the lock method of a mutex twice in succession
 *              might cause a deadlock.
 * @kind problem
 * @id cpp/twice-locked
 * @problem.severity error
 * @precision low
 * @tags security
 *       external/cwe/cwe-764
 *       external/cwe/cwe-833
 */
import cpp
import semmle.code.cpp.commons.Synchronization
import LockFlow

/**
 * Holds if `call` locks `v`, via the access `a`, but `v` might already
 * be locked when we reach `call`. The access `a` might be in a function
 * which is called indirectly from `call`.
 */
cached private predicate twiceLocked(
  FunctionCall call, Variable v, VariableAccess a) {
  lockedOnEntry(v.getAnAccess(), call) and
  lockedInCall(a, call)
}

/*
 * When this query finds a result, there are often multiple call sites
 * associated with one instance of the problem. For this reason, we do not
 * include `call` in the result. However, it is sometimes helpful to
 * include `call.getLocation()` in the result, because it can help to find
 * the control flow path which might be responsible.
 */
from FunctionCall call, Variable v, VariableAccess access2
where
  twiceLocked(call, v, access2) and
  v = access2.getTarget() and

  // If the second lock is a `try_lock` then it won't cause a deadlock.
  // We want to be extra sure that the second lock is not a `try_lock`
  // to make sure that we don't generate too many false positives, so
  // we use three heuristics:
  //
  //  1. The call is to a function named "try_lock".
  //  2. The result of the call is used in a condition. For example:
  //       if (pthread_mutex_lock(mtx) != 0) return -1;
  //  3. The call is a condition. Because the analysis is interprocedural,
  //     `call` might be an indirect call to `lock`, so this heuristic
  //     catches some cases which the second heuristic does not.
  not (trylockCall(access2, _) or
       tryLockCondition(access2, _, _) or
       call.isCondition())
select access2, "Mutex " + v + " might be locked already, which could cause a deadlock."

Mutexes come in two flavors: recursive and non-recursive. For example, the C++ mutex library provides both std::mutex and std::recursive_mutex. A non-recursive mutex cannot be locked until it has been unlocked by its previous owner, even if it is already owned by the current thread. Deadlock is often caused by a thread attempting to lock the same mutex twice, usually in a recursive algorithm.

Recommendation

If a recursive method needs to acquire a lock, then split it into two methods. The first method is public and is responsible for locking and unlocking the mutex. It delegates the rest of the work to a second private method. The second method does not need to lock or unlock the mutex because that is done by the first method.

Example

In this example, the method f is recursive, so it causes a deadlock by attempting to lock the mutex twice.

class C {
  std::mutex mutex;
public:
  // BAD: recursion causes deadlock.
  int f(int n) {
    mutex.lock();
    int result = (n == 0) ? 1 : n*f(n-1);
    mutex.unlock();
    return result;
  }
};

In this second example, the recursion is delegated to an internal method so the mutex is only locked once.

class C {
  std::mutex mutex;
  int f_impl(int n) {
    return (n == 0) ? 1 : n*f_impl(n-1);
  }
public:
  // GOOD: recursion is delegated to f_impl.
  int f(int n) {
    mutex.lock();
    int result = f_impl(n);
    mutex.unlock();
    return result;
  }
};

References
  • Common Weakness Enumeration: CWE-764.
  • Common Weakness Enumeration: CWE-833.