[clang][analyzer] Possible false positive from unix.BlockInCriticalSection

[hcho3]

Clang-tidy generates a false positive for the unix.BlockInCriticalSection check when a thread that enters a critical section performs a recv() with a non-blocking socket. This is because clang-tidy assumes fcntl (system call to set the socket to be non-blocking) to always fail.

Example: https://github.com/dmlc/xgboost/blob/0def8e0bae41e0eeeb4ee078e6e699ad6bef1986/src/collective/loop.cc#L88-L105

Error message

``` /workspace/include/xgboost/collective/socket.h:696:12: warning: Call to blocking function 'recv' inside of critical section [clang-analyzer-unix.BlockInCriticalSection] 696 | return recv(handle_, _buf, len, flags); | ^ /workspace/src/collective/loop.cc:272:5: note: Calling 'Loop::Process' 272 | this->Process(); | ^~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:154:3: note: Loop condition is true. Entering loop body 154 | while (true) { | ^ /workspace/src/collective/loop.cc:156:24: note: Calling constructor for 'unique_lock' 156 | std::unique_lock lock{mu_}; | ^~~~~~~~~ /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_lock.h:69:2: note: Entering critical section here 69 | lock(); | ^~~~~~ /workspace/src/collective/loop.cc:156:24: note: Returning from constructor for 'unique_lock' 156 | std::unique_lock lock{mu_}; | ^~~~~~~~~ /workspace/src/collective/loop.cc:167:11: note: Assuming field 'stop_' is false 167 | if (stop_) { | ^~~~~ /workspace/src/collective/loop.cc:167:7: note: Taking false branch 167 | if (stop_) { | ^ /workspace/src/collective/loop.cc:174:14: note: Assuming the condition is false 174 | while (!queue_.empty()) { | ^~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:174:7: note: Loop condition is false. Execution continues on line 179 174 | while (!queue_.empty()) { | ^ /workspace/src/collective/loop.cc:182:17: note: Calling 'Loop::ProcessQueue' 182 | auto rc = this->ProcessQueue(&qcopy); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:30:7: note: Assuming field 'stop_' is false 30 | if (stop_) { | ^~~~~ /workspace/src/collective/loop.cc:30:3: note: Taking false branch 30 | if (stop_) { | ^ /workspace/src/collective/loop.cc:38:10: note: Assuming the condition is true 38 | while (!qcopy.empty()) { | ^~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:38:3: note: Loop condition is true. Entering loop body 38 | while (!qcopy.empty()) { | ^ /workspace/src/collective/loop.cc:43:29: note: Assuming 'i' is < 'n_ops' 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^~~~~~~~~ /workspace/src/collective/loop.cc:43:5: note: Loop condition is true. Entering loop body 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:47:7: note: Control jumps to 'case kSleep:' at line 56 47 | switch (op.code) { | ^ /workspace/src/collective/loop.cc:57:11: note: Execution continues on line 65 57 | break; | ^ /workspace/src/collective/loop.cc:43:29: note: Assuming 'i' is >= 'n_ops' 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^~~~~~~~~ /workspace/src/collective/loop.cc:43:5: note: Loop condition is false. Execution continues on line 69 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:70:9: note: Assuming the condition is false 70 | if (!poll.fds.empty()) { | ^~~~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:70:5: note: Taking false branch 70 | if (!poll.fds.empty()) { | ^ /workspace/src/collective/loop.cc:80:11: note: Assuming the condition is true 80 | CHECK(!qcopy.empty()); | ^ /workspace/include/xgboost/logging.h:144:24: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^~~~ /workspace/include/xgboost/base.h:53:54: note: expanded from macro 'XGBOOST_EXPECT' 53 | #define XGBOOST_EXPECT(cond, ret) __builtin_expect((cond), (ret)) | ^~~~ /workspace/src/collective/loop.cc:80:5: note: Taking false branch 80 | CHECK(!qcopy.empty()); | ^ /workspace/include/xgboost/logging.h:144:3: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^ /workspace/src/collective/loop.cc:83:5: note: Loop condition is true. Entering loop body 83 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:88:11: note: Assuming field 'sock' is non-null 88 | if (!op.sock) { | ^~~~~~~~ /workspace/src/collective/loop.cc:88:7: note: Taking false branch 88 | if (!op.sock) { | ^ /workspace/src/collective/loop.cc:91:9: note: Assuming the condition is false 91 | CHECK(op.sock->NonBlocking()); | ^ /workspace/include/xgboost/logging.h:144:22: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ /workspace/include/xgboost/base.h:53:54: note: expanded from macro 'XGBOOST_EXPECT' 53 | #define XGBOOST_EXPECT(cond, ret) __builtin_expect((cond), (ret)) | ^~~~ /workspace/src/collective/loop.cc:91:9: note: Taking false branch 91 | CHECK(op.sock->NonBlocking()); | ^ /workspace/include/xgboost/logging.h:144:3: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^ /workspace/src/collective/loop.cc:94:7: note: Control jumps to 'case kRead:' at line 95 94 | switch (op.code) { | ^ /workspace/src/collective/loop.cc:96:11: note: Taking true branch 96 | if (poll.CheckRead(*op.sock)) { | ^ /workspace/src/collective/loop.cc:97:28: note: Calling 'TCPSocket::Recv' 97 | n_bytes_done = op.sock->Recv(op.ptr + op.off, op.n - op.off); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /workspace/include/xgboost/collective/socket.h:696:12: note: Call to blocking function 'recv' inside of critical section 696 | return recv(handle_, _buf, len, flags); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -------------------------------- ```

Suggested fix: Add a note in the documentation about possible false positives with non-blocking sockets.

[llvmbot]

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

Author: Philip Hyunsu Cho (hcho3)

Clang-tidy generates a false positive for the `unix.BlockInCriticalSection` check when a thread that enters a critical section performs a `recv()` with a non-blocking socket. Example: https://github.com/dmlc/xgboost/blob/0def8e0bae41e0eeeb4ee078e6e699ad6bef1986/src/collective/loop.cc#L88-L105 <details> <summary>Error message</summary> ``` /workspace/include/xgboost/collective/socket.h:696:12: warning: Call to blocking function 'recv' inside of critical section [clang-analyzer-unix.BlockInCriticalSection] 696 | return recv(handle_, _buf, len, flags); | ^ /workspace/src/collective/loop.cc:272:5: note: Calling 'Loop::Process' 272 | this->Process(); | ^~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:154:3: note: Loop condition is true. Entering loop body 154 | while (true) { | ^ /workspace/src/collective/loop.cc:156:24: note: Calling constructor for 'unique_lock<std::mutex>' 156 | std::unique_lock lock{mu_}; | ^~~~~~~~~ /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_lock.h:69:2: note: Entering critical section here 69 | lock(); | ^~~~~~ /workspace/src/collective/loop.cc:156:24: note: Returning from constructor for 'unique_lock<std::mutex>' 156 | std::unique_lock lock{mu_}; | ^~~~~~~~~ /workspace/src/collective/loop.cc:167:11: note: Assuming field 'stop_' is false 167 | if (stop_) { | ^~~~~ /workspace/src/collective/loop.cc:167:7: note: Taking false branch 167 | if (stop_) { | ^ /workspace/src/collective/loop.cc:174:14: note: Assuming the condition is false 174 | while (!queue_.empty()) { | ^~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:174:7: note: Loop condition is false. Execution continues on line 179 174 | while (!queue_.empty()) { | ^ /workspace/src/collective/loop.cc:182:17: note: Calling 'Loop::ProcessQueue' 182 | auto rc = this->ProcessQueue(&qcopy); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:30:7: note: Assuming field 'stop_' is false 30 | if (stop_) { | ^~~~~ /workspace/src/collective/loop.cc:30:3: note: Taking false branch 30 | if (stop_) { | ^ /workspace/src/collective/loop.cc:38:10: note: Assuming the condition is true 38 | while (!qcopy.empty()) { | ^~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:38:3: note: Loop condition is true. Entering loop body 38 | while (!qcopy.empty()) { | ^ /workspace/src/collective/loop.cc:43:29: note: Assuming 'i' is < 'n_ops' 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^~~~~~~~~ /workspace/src/collective/loop.cc:43:5: note: Loop condition is true. Entering loop body 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:47:7: note: Control jumps to 'case kSleep:' at line 56 47 | switch (op.code) { | ^ /workspace/src/collective/loop.cc:57:11: note: Execution continues on line 65 57 | break; | ^ /workspace/src/collective/loop.cc:43:29: note: Assuming 'i' is >= 'n_ops' 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^~~~~~~~~ /workspace/src/collective/loop.cc:43:5: note: Loop condition is false. Execution continues on line 69 43 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:70:9: note: Assuming the condition is false 70 | if (!poll.fds.empty()) { | ^~~~~~~~~~~~~~~~~ /workspace/src/collective/loop.cc:70:5: note: Taking false branch 70 | if (!poll.fds.empty()) { | ^ /workspace/src/collective/loop.cc:80:11: note: Assuming the condition is true 80 | CHECK(!qcopy.empty()); | ^ /workspace/include/xgboost/logging.h:144:24: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^~~~ /workspace/include/xgboost/base.h:53:54: note: expanded from macro 'XGBOOST_EXPECT' 53 | #define XGBOOST_EXPECT(cond, ret) __builtin_expect((cond), (ret)) | ^~~~ /workspace/src/collective/loop.cc:80:5: note: Taking false branch 80 | CHECK(!qcopy.empty()); | ^ /workspace/include/xgboost/logging.h:144:3: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^ /workspace/src/collective/loop.cc:83:5: note: Loop condition is true. Entering loop body 83 | for (std::size_t i = 0; i < n_ops; ++i) { | ^ /workspace/src/collective/loop.cc:88:11: note: Assuming field 'sock' is non-null 88 | if (!op.sock) { | ^~~~~~~~ /workspace/src/collective/loop.cc:88:7: note: Taking false branch 88 | if (!op.sock) { | ^ /workspace/src/collective/loop.cc:91:9: note: Assuming the condition is false 91 | CHECK(op.sock->NonBlocking()); | ^ /workspace/include/xgboost/logging.h:144:22: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ /workspace/include/xgboost/base.h:53:54: note: expanded from macro 'XGBOOST_EXPECT' 53 | #define XGBOOST_EXPECT(cond, ret) __builtin_expect((cond), (ret)) | ^~~~ /workspace/src/collective/loop.cc:91:9: note: Taking false branch 91 | CHECK(op.sock->NonBlocking()); | ^ /workspace/include/xgboost/logging.h:144:3: note: expanded from macro 'CHECK' 144 | if (XGBOOST_EXPECT(!(cond), false)) \ | ^ /workspace/src/collective/loop.cc:94:7: note: Control jumps to 'case kRead:' at line 95 94 | switch (op.code) { | ^ /workspace/src/collective/loop.cc:96:11: note: Taking true branch 96 | if (poll.CheckRead(*op.sock)) { | ^ /workspace/src/collective/loop.cc:97:28: note: Calling 'TCPSocket::Recv' 97 | n_bytes_done = op.sock->Recv(op.ptr + op.off, op.n - op.off); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /workspace/include/xgboost/collective/socket.h:696:12: note: Call to blocking function 'recv' inside of critical section 696 | return recv(handle_, _buf, len, flags); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -------------------------------- ``` </details> Suggested fix: Add a note in the documentation about possible false positives with non-blocking sockets.

[steakhal]

What clang tidy version fo you use?

[hcho3]

Clang-tidy 19, obtained from the LLVM apt repository

[steakhal]

Yea, I suspected. I also looked at new FPs of this kind for this checker. I havent looked at those yet in detail, but its on my plate.

I suspect is has something to do with the CallDescription changes there. @NagyDonat

[NagyDonat]

In addition to my (mostly non-functional) CallDescription changes, this checker was heavily refactored by @gamesh411, so I'm forwarding your ping to him :smile:

[gamesh411]

Currently, the modeling does not consider any modifications done through fnctl. I think it would be feasible to support this. IMO, adding a limitation section to the docs is the best option for now, as I am working on unifying the tracking of mutex-related events during symbolic execution (via a modeling checker), and adding this case is now part of the backlog as well.

[steakhal]

Here is a repro for a case we observed. In clang-18, we don't report the sleep as blocking, while in clang-19 we would: https://compiler-explorer.com/z/boos8vPTc

namespace std {
struct __mutex_base {
  void lock();
};
struct mutex : __mutex_base {
  void unlock();
  bool try_lock();
};
template <class... MutexTypes> struct scoped_lock {
  explicit scoped_lock(MutexTypes&... m);
  ~scoped_lock();
};
template <class MutexType> class scoped_lock<MutexType> {
public:
  explicit scoped_lock(MutexType& m) : m(m) { m.lock(); }
  ~scoped_lock() { m.unlock(); }
private:
  MutexType& m;
};
} // namespace std

extern "C" unsigned int sleep(unsigned int seconds);

int magic_number;
std::mutex m;

void fixed() {
  int current;
  for (int items_processed = 0; items_processed < 100; ++items_processed) {
    {
      std::scoped_lock guard(m);
      current = magic_number;
    }
    sleep(current); // expected no warning
  }
}

Could you please have a look if it's the same issue as the user reported in this ticket? And why do we have these reports? @gamesh411

[necto]

@hcho3 could you, please, try the fix from https://github.com/llvm/llvm-project/pull/106240 to see if it addresses the false positive?

[hcho3]

I am not familiar with the LLVM project. Is there a way to build clang-tidy separately from other components of LLVM?

[EugeneZelenko]

@hliao2: You need to build at least LLVM and Clang, because Clang-tidy depends on both.

[steakhal]

FYI We accept this as a regression in clang-19, but the fix is gonna be part of clang-20 (or built from sources).