False positive on unix.BlockInCriticalSection: not checking destructors

[halfgaar]

False positive on unix.BlockInCriticalSection; it doesn't see destructors that unlock locks.

Version:

$HOME/opt/Qt/Tools/QtCreator/libexec/qtcreator/clang/bin/clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 20.1.3
  Optimized build.

Repro:

#include <unistd.h>
#include <sys/eventfd.h>
#include <mutex>

int main()
{

  int fd = eventfd(0, EFD_NONBLOCK);
  std::mutex m;

  while (true)
  {
   // This lock gets yielded, but unix.BlockInCriticalSection doesn't see it
    std::unique_lock ul(m); 

    uint64_t eventfd_value = 0;
    if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      return 1;

  }

  return 0;

}

Error:

$HOME/opt/Qt/Tools/QtCreator/libexec/qtcreator/clang/bin/clang-tidy -p /tmp/QtCreator-HakOdJ/ClazyWINiRx/compile_commands.json -checks=-*,clang-analyzer-unix.BlockInCriticalSection main.cpp

/home/halfgaar/tmp/block/main.cpp:15:9: warning: Call to blocking function 'read' inside of critical section [clang-analyzer-unix.BlockInCriticalSection]
   15 |     if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/halfgaar/tmp/block/main.cpp:11:3: note: Loop condition is true.  Entering loop body
   11 |   while (true)
      |   ^
/home/halfgaar/tmp/block/main.cpp:13:22: note: Calling constructor for 'unique_lock<std::mutex>'
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Calling 'unique_lock::lock'
   69 |         lock();
      |         ^~~~~~
/usr/include/c++/11/bits/unique_lock.h:133:7: note: Field '_M_device' is non-null
  133 |         if (!_M_device)
      |              ^
/usr/include/c++/11/bits/unique_lock.h:133:2: note: Taking false branch
  133 |         if (!_M_device)
      |         ^
/usr/include/c++/11/bits/unique_lock.h:135:11: note: Field '_M_owns' is false
  135 |         else if (_M_owns)
      |                  ^
/usr/include/c++/11/bits/unique_lock.h:135:7: note: Taking false branch
  135 |         else if (_M_owns)
      |              ^
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Calling 'mutex::lock'
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/std_mutex.h:100:17: note: Calling '__gthread_mutex_lock'
  100 |       int __e = __gthread_mutex_lock(&_M_mutex);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:748:3: note: Taking true branch
  748 |   if (__gthread_active_p ())
      |   ^
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:749:21: note: Entering critical section here
  749 |     return __gthrw_(pthread_mutex_lock) (__mutex);
      |                     ^
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:96:25: note: expanded from macro '__gthrw_'
   96 | # define __gthrw_(name) name
      |                         ^
/usr/include/c++/11/bits/std_mutex.h:100:17: note: Returning from '__gthread_mutex_lock'
  100 |       int __e = __gthread_mutex_lock(&_M_mutex);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/std_mutex.h:103:11: note: Assuming '__e' is 0
  103 |       if (__e)
      |           ^~~
/usr/include/c++/11/bits/std_mutex.h:103:7: note: Taking false branch
  103 |       if (__e)
      |       ^
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Returning from 'mutex::lock'
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Entering critical section here
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Returning from 'unique_lock::lock'
   69 |         lock();
      |         ^~~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Entering critical section for the 1st time here
   69 |         lock();
      |         ^~~~~~
/home/halfgaar/tmp/block/main.cpp:13:22: note: Returning from constructor for 'unique_lock<std::mutex>'
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/home/halfgaar/tmp/block/main.cpp:13:22: note: Entering critical section for the 2nd time here
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/home/halfgaar/tmp/block/main.cpp:15:9: note: Call to blocking function 'read' inside of critical section
   15 |     if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Wiebe Cazemier (halfgaar)

False positive on `unix.BlockInCriticalSection`; it doesn't see destructors that unlock locks.

Version:

$HOME/opt/Qt/Tools/QtCreator/libexec/qtcreator/clang/bin/clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 20.1.3
  Optimized build.

Repro:

#include <unistd.h>
#include <sys/eventfd.h>
#include <mutex>

int main()
{

  int fd = eventfd(0, EFD_NONBLOCK);
  std::mutex m;

  while (true)
  {
   // This lock gets yielded, but unix.BlockInCriticalSection doesn't see it
    std::unique_lock ul(m); 

    uint64_t eventfd_value = 0;
    if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      return 1;

  }

  return 0;

}

Error:

$HOME/opt/Qt/Tools/QtCreator/libexec/qtcreator/clang/bin/clang-tidy -p /tmp/QtCreator-HakOdJ/ClazyWINiRx/compile_commands.json -checks=-*,clang-analyzer-unix.BlockInCriticalSection main.cpp

/home/halfgaar/tmp/block/main.cpp:15:9: warning: Call to blocking function 'read' inside of critical section [clang-analyzer-unix.BlockInCriticalSection]
   15 |     if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/halfgaar/tmp/block/main.cpp:11:3: note: Loop condition is true.  Entering loop body
   11 |   while (true)
      |   ^
/home/halfgaar/tmp/block/main.cpp:13:22: note: Calling constructor for 'unique_lock<std::mutex>'
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Calling 'unique_lock::lock'
   69 |         lock();
      |         ^~~~~~
/usr/include/c++/11/bits/unique_lock.h:133:7: note: Field '_M_device' is non-null
  133 |         if (!_M_device)
      |              ^
/usr/include/c++/11/bits/unique_lock.h:133:2: note: Taking false branch
  133 |         if (!_M_device)
      |         ^
/usr/include/c++/11/bits/unique_lock.h:135:11: note: Field '_M_owns' is false
  135 |         else if (_M_owns)
      |                  ^
/usr/include/c++/11/bits/unique_lock.h:135:7: note: Taking false branch
  135 |         else if (_M_owns)
      |              ^
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Calling 'mutex::lock'
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/std_mutex.h:100:17: note: Calling '__gthread_mutex_lock'
  100 |       int __e = __gthread_mutex_lock(&_M_mutex);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:748:3: note: Taking true branch
  748 |   if (__gthread_active_p ())
      |   ^
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:749:21: note: Entering critical section here
  749 |     return __gthrw_(pthread_mutex_lock) (__mutex);
      |                     ^
/usr/include/x86_64-linux-gnu/c++/11/bits/gthr-default.h:96:25: note: expanded from macro '__gthrw_'
   96 | # define __gthrw_(name) name
      |                         ^
/usr/include/c++/11/bits/std_mutex.h:100:17: note: Returning from '__gthread_mutex_lock'
  100 |       int __e = __gthread_mutex_lock(&_M_mutex);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/std_mutex.h:103:11: note: Assuming '__e' is 0
  103 |       if (__e)
      |           ^~~
/usr/include/c++/11/bits/std_mutex.h:103:7: note: Taking false branch
  103 |       if (__e)
      |       ^
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Returning from 'mutex::lock'
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/unique_lock.h:139:6: note: Entering critical section here
  139 |             _M_device->lock();
      |             ^~~~~~~~~~~~~~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Returning from 'unique_lock::lock'
   69 |         lock();
      |         ^~~~~~
/usr/include/c++/11/bits/unique_lock.h:69:2: note: Entering critical section for the 1st time here
   69 |         lock();
      |         ^~~~~~
/home/halfgaar/tmp/block/main.cpp:13:22: note: Returning from constructor for 'unique_lock<std::mutex>'
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/home/halfgaar/tmp/block/main.cpp:13:22: note: Entering critical section for the 2nd time here
   13 |     std::unique_lock ul(m);
      |                      ^~~~~
/home/halfgaar/tmp/block/main.cpp:15:9: note: Call to blocking function 'read' inside of critical section
   15 |     if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[NagyDonat]

The bug report in your reproducer is a true positive: the analyzer reports that a blocking function (read) was called in a situation where the mutex is locked. The analyzer would see that the destructor eventually unlocks the lock, but doesn't reach that point because it stops to report this problem.

As that code is executed, things happen in the following order:

1. We enter the while loop.
2. We construct the unique_lock object ul, calling the constructor of unique_lock which locks the mutex.
3. We call read (which is a blocking function: its waits for I/O so execution can take lots of time).
4. The object ul goes out of scope (either when we return from the function or when we reach the } that closes the body of the while statement); so the destructor of unique_lock unlocks the mutex.
5. There may be several other iterations of the loop, but those are irrelevant for us – the analyzer report is already created at step 3 of the first iteration.

Also note that this "unix.BlockInCriticalSection" checker only tries to find "blocking call within a critical section" errors, the "lock was not released" errors would be handled by a different checker ("alpha.unix.PthreadLock" – which is unfortunately not yet production quality).

[halfgaar]

Sorry, I was too hasty in my repro. Here's a simplified case of the actual code:

#include <unistd.h>
#include <sys/eventfd.h>
#include <mutex>
#include <vector>
#include <iostream>
#include <thread>

template<typename T>
class MutexLocked
{
    std::unique_lock<std::mutex> l;
    T *d = nullptr;

public:

    MutexLocked() = default;

    MutexLocked(T &other, std::mutex &m) :
        l(m),
        d(&other)
    {

    }

    MutexLocked(const MutexLocked<T> &other) = delete;

    MutexLocked<T> &operator=(const MutexLocked<T> &other) = delete;

    MutexLocked(MutexLocked<T> &&other) noexcept :
        l(std::move(other.l)),
        d(other.d)
    {
        other.d = nullptr;
    }

    ~MutexLocked()
    {
        std::cout << "Destroying MutexLocked, which destroys the unique_lock" << std::endl;
        d = nullptr;
    }
};

template<typename T>
class MutexOwned
{
    std::mutex m;
    T d;

public:
    template<typename... Args>
    MutexOwned(Args... args) :
        d(args...)
    {

    }

    MutexLocked<T> lock()
    {
        MutexLocked<T> r(d, m);
        return r;
    }
};

int main()
{

    int fd = eventfd(0, EFD_NONBLOCK);
    MutexOwned<std::vector<std::string>> strings;

    while (true)
    {
        std::this_thread::sleep_for(std::chrono::seconds(1));

        uint64_t eventfd_value = 0;
        if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
            std::cout << "No event written to eventfd, but that's OK" << std::endl;

        {
            auto locked = strings.lock();
            continue;
        }

        if (read(fd, &eventfd_value, sizeof(uint64_t)) < 0)
            std::cout << "No event written to eventfd, but that's OK" << std::endl;
    }

    return 0;
}

It doesn't seem to track it through templates.

Incidentally, it also says the value stored in fd is never read, which also seems a false positive, because it's given to the read function.

I can't reopen the issue, so I hope you can.