[clang][analyzer] fix false positive of BlockInCriticalSectionChecker

Author: flovent

clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

@@ -145,7 +145,8 @@ using MutexDescriptor =
     std::variant<FirstArgMutexDescriptor, MemberMutexDescriptor,
                  RAIIMutexDescriptor>;
 
-class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
+class BlockInCriticalSectionChecker
+    : public Checker<check::PostCall, check::DeadSymbols> {
 private:
   const std::array<MutexDescriptor, 8> MutexDescriptors{
       // NOTE: There are standard library implementations where some methods
@@ -179,6 +180,8 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
                                              {CDM::CLibrary, {"read"}},
                                              {CDM::CLibrary, {"recv"}}};
 
+  const CallDescription OpenFunction{CDM::CLibrary, {"open"}, 2};
+
   const BugType BlockInCritSectionBugType{
       this, "Call to blocking function in critical section", "Blocking Error"};
 
@@ -197,6 +200,8 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
   void handleUnlock(const MutexDescriptor &Mutex, const CallEvent &Call,
                     CheckerContext &C) const;
 
+  void handleOpen(const CallEvent &Call, CheckerContext &C) const;
+
   [[nodiscard]] bool isBlockingInCritSection(const CallEvent &Call,
                                              CheckerContext &C) const;
 
@@ -205,11 +210,14 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
   /// Process lock.
   /// Process blocking functions (sleep, getc, fgets, read, recv)
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 };
 
 } // end anonymous namespace
 
 REGISTER_LIST_WITH_PROGRAMSTATE(ActiveCritSections, CritSectionMarker)
+REGISTER_SET_WITH_PROGRAMSTATE(NonBlockFileDescriptor, SymbolRef)
 
 // Iterator traits for ImmutableList data structure
 // that enable the use of STL algorithms.
@@ -306,6 +314,25 @@ void BlockInCriticalSectionChecker::handleUnlock(
   C.addTransition(State);
 }
 
+void BlockInCriticalSectionChecker::handleOpen(const CallEvent &Call,
+                                               CheckerContext &C) const {
+  const auto *Flag = Call.getArgExpr(1);
+  static std::optional<int> ValueOfONonBlockVFlag =
+      tryExpandAsInteger("O_NONBLOCK", C.getBugReporter().getPreprocessor());
+  if (!ValueOfONonBlockVFlag)
+    return;
+
+  SVal FlagSV = C.getState()->getSVal(Flag, C.getLocationContext());
+  const llvm::APSInt *FlagV = FlagSV.getAsInteger();
+  if (!FlagV)
+    return;
+
+  if ((*FlagV & ValueOfONonBlockVFlag.value()) != 0)
+    if (SymbolRef SR = Call.getReturnValue().getAsSymbol()) {
+      C.addTransition(C.getState()->add<NonBlockFileDescriptor>(SR));
+    }
+}
+
 bool BlockInCriticalSectionChecker::isBlockingInCritSection(
     const CallEvent &Call, CheckerContext &C) const {
   return BlockingFunctions.contains(Call) &&
@@ -315,16 +342,54 @@ bool BlockInCriticalSectionChecker::isBlockingInCritSection(
 void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
                                                   CheckerContext &C) const {
   if (isBlockingInCritSection(Call, C)) {
+    // for 'read' and 'recv' call, check whether it's file descriptor(first
+    // argument) is
+    // created by 'open' API with O_NONBLOCK flag or is equal to -1, they will
+    // not cause block in these situations, don't report
+    StringRef FuncName = Call.getCalleeIdentifier()->getName();
+    if (FuncName == "read" || FuncName == "recv") {
+      const auto *Arg = Call.getArgExpr(0);
+      if (!Arg)
+        return;
+
+      SVal SV = C.getSVal(Arg);
+      if (const auto *IntValue = SV.getAsInteger()) {
+        if (*IntValue == -1)
+          return;
+      }
+
+      SymbolRef SR = C.getSVal(Arg).getAsSymbol();
+      if (SR && C.getState()->contains<NonBlockFileDescriptor>(SR)) {
+        return;
+      }
+    }
     reportBlockInCritSection(Call, C);
   } else if (std::optional<MutexDescriptor> LockDesc =
                  checkDescriptorMatch(Call, C, /*IsLock=*/true)) {
     handleLock(*LockDesc, Call, C);
   } else if (std::optional<MutexDescriptor> UnlockDesc =
                  checkDescriptorMatch(Call, C, /*IsLock=*/false)) {
     handleUnlock(*UnlockDesc, Call, C);
+  } else if (OpenFunction.matches(Call)) {
+    handleOpen(Call, C);
   }
 }
 
+void BlockInCriticalSectionChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+                                                     CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  // Remove the dead symbols from the NonBlockFileDescriptor set.
+  NonBlockFileDescriptorTy Tracked = State->get<NonBlockFileDescriptor>();
+  for (SymbolRef SR : Tracked) {
+    if (SymReaper.isDead(SR)) {
+      State = State->remove<NonBlockFileDescriptor>(SR);
+    }
+  }
+
+  C.addTransition(State);
+}
+
 void BlockInCriticalSectionChecker::reportBlockInCritSection(
     const CallEvent &Call, CheckerContext &C) const {
   ExplodedNode *ErrNode = C.generateNonFatalErrorNode(C.getState());

clang/test/Analysis/issue-124474.cpp

@@ -0,0 +1,49 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=unix.BlockInCriticalSection \
+// RUN:   -std=c++11 \
+// RUN:   -analyzer-output text \
+// RUN:   -verify %s
+
+// expected-no-diagnostics
+
+namespace std {
+  struct mutex {
+    void lock() {}
+    void unlock() {}
+  };
+  template<typename T>
+  struct lock_guard {
+    lock_guard<T>(std::mutex) {}
+    ~lock_guard<T>() {}
+  };
+  template<typename T>
+  struct unique_lock {
+    unique_lock<T>(std::mutex) {}
+    ~unique_lock<T>() {}
+  };
+  template<typename T>
+  struct not_real_lock {
+    not_real_lock<T>(std::mutex) {}
+  };
+  } // namespace std
+
+std::mutex mtx;
+using ssize_t = long long;
+using size_t = unsigned long long;
+int open (const char *__file, int __oflag, ...);
+ssize_t read(int fd, void *buf, size_t count);
+void close(int fd);
+#define O_RDONLY	     00
+# define O_NONBLOCK	  04000
+
+void foo()
+{
+    std::lock_guard<std::mutex> lock(mtx);
+
+    const char *filename = "example.txt";
+    int fd = open(filename, O_RDONLY | O_NONBLOCK);
+
+    char buffer[200] = {};
+    read(fd, buffer, 199); // no-warning: fd is a non-block file descriptor
+    close(fd);
+}