use BugReportVisitor instead set

Author: flovent

clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

@@ -145,8 +145,63 @@ using MutexDescriptor =
     std::variant<FirstArgMutexDescriptor, MemberMutexDescriptor,
                  RAIIMutexDescriptor>;
 
-class BlockInCriticalSectionChecker
-    : public Checker<check::PostCall, check::DeadSymbols> {
+class NonBlockOpenVisitor : public BugReporterVisitor {
+private:
+  SymbolRef SR;
+  int O_NONBLOCKValue;
+  const CallDescription OpenFunction;
+  bool Satisfied;
+
+public:
+  NonBlockOpenVisitor(SymbolRef SR, int O_NONBLOCKValue)
+      : SR(SR), O_NONBLOCKValue(O_NONBLOCKValue),
+        OpenFunction(CDM::CLibrary, {"open"}, 2), Satisfied(false) {}
+
+  static void *getTag() {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    ID.AddPointer(getTag());
+    ID.AddPointer(SR);
+  }
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                   BugReporterContext &BRC,
+                                   PathSensitiveBugReport &BR) override {
+    if (Satisfied)
+      return nullptr;
+
+    std::optional<StmtPoint> Point = N->getLocation().getAs<StmtPoint>();
+    if (!Point)
+      return nullptr;
+
+    auto *CE = dyn_cast<CallExpr>(Point->getStmt());
+    if (!CE || !OpenFunction.matchesAsWritten(*CE))
+      return nullptr;
+
+    SVal SV = N->getSVal(CE);
+    if (SV.getAsSymbol() != SR)
+      return nullptr;
+
+    Satisfied = true;
+
+    // Check if open's second argument contains O_NONBLOCK
+    const auto *Flag = CE->getArg(1);
+    SVal FlagSV = N->getSVal(Flag);
+    const llvm::APSInt *FlagV = FlagSV.getAsInteger();
+    if (!FlagV)
+      return nullptr;
+
+    if ((*FlagV & O_NONBLOCKValue) != 0)
+      BR.markInvalid(getTag(), nullptr);
+
+    return nullptr;
+  }
+};
+
+class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
 private:
   const std::array<MutexDescriptor, 8> MutexDescriptors{
       // NOTE: There are standard library implementations where some methods
@@ -180,8 +235,6 @@ class BlockInCriticalSectionChecker
                                              {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"};
 
@@ -203,8 +256,6 @@ class BlockInCriticalSectionChecker
   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;
 
@@ -213,14 +264,11 @@ class BlockInCriticalSectionChecker
   /// 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.
@@ -317,24 +365,6 @@ void BlockInCriticalSectionChecker::handleUnlock(
   C.addTransition(State);
 }
 
-void BlockInCriticalSectionChecker::handleOpen(const CallEvent &Call,
-                                               CheckerContext &C) const {
-  const auto *Flag = Call.getArgExpr(1);
-  SVal FlagSV = C.getState()->getSVal(Flag, C.getLocationContext());
-  const llvm::APSInt *FlagV = FlagSV.getAsInteger();
-  if (!FlagV)
-    return;
-
-  if (!O_NONBLOCKValue)
-    O_NONBLOCKValue =
-        tryExpandAsInteger("O_NONBLOCK", C.getBugReporter().getPreprocessor());
-
-  if (*O_NONBLOCKValue && ((*FlagV & **O_NONBLOCKValue) != 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) &&
@@ -344,52 +374,16 @@ 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") {
-      SVal SV = Call.getArgSVal(0);
-      SValBuilder &SVB = C.getSValBuilder();
-      ProgramStateRef state = C.getState();
-      ConditionTruthVal CTV =
-          state->areEqual(SV, SVB.makeIntVal(-1, C.getASTContext().IntTy));
-      if (CTV.isConstrainedTrue())
-        return;
-
-      SymbolRef SR = SV.getAsSymbol();
-      if (SR && state->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());
@@ -402,6 +396,28 @@ void BlockInCriticalSectionChecker::reportBlockInCritSection(
      << "' inside of critical section";
   auto R = std::make_unique<PathSensitiveBugReport>(BlockInCritSectionBugType,
                                                     os.str(), ErrNode);
+  // 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") {
+    SVal SV = Call.getArgSVal(0);
+    SValBuilder &SVB = C.getSValBuilder();
+    ProgramStateRef state = C.getState();
+    ConditionTruthVal CTV =
+        state->areEqual(SV, SVB.makeIntVal(-1, C.getASTContext().IntTy));
+    if (CTV.isConstrainedTrue())
+      return;
+
+    if (SymbolRef SR = SV.getAsSymbol()) {
+      if (!O_NONBLOCKValue)
+        O_NONBLOCKValue = tryExpandAsInteger(
+            "O_NONBLOCK", C.getBugReporter().getPreprocessor());
+      if (*O_NONBLOCKValue)
+        R->addVisitor<NonBlockOpenVisitor>(SR, **O_NONBLOCKValue);
+    }
+  }
   R->addRange(Call.getSourceRange());
   R->markInteresting(Call.getReturnValue());
   C.emitReport(std::move(R));