[clang] [clang][analyzer] fix false positive of BlockInCriticalSectionChecker (PR #127049)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 13 04:22:11 PST 2025
https://github.com/flovent created https://github.com/llvm/llvm-project/pull/127049
this PR close #124474
when calling `read` and `recv` function for a non-block file descriptor or a invalid file descriptor(`-1`), it will not cause block inside a critical section.
this commit checks for non-block file descriptor assigned by `open` function with `O_NONBLOCK` flag.
>From c916dadbaf6021eda606d76784115698a9800571 Mon Sep 17 00:00:00 2001
From: flovent <flbven at protonmail.com>
Date: Thu, 13 Feb 2025 20:17:20 +0800
Subject: [PATCH] [clang][analyzer] fix false positive of
BlockInCriticalSectionChecker
---
.../BlockInCriticalSectionChecker.cpp | 67 ++++++++++++++++++-
clang/test/Analysis/issue-124474.cpp | 49 ++++++++++++++
2 files changed, 115 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Analysis/issue-124474.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 7460781799d08..db784f2cc77b2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/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,6 +342,27 @@ 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)) {
@@ -322,9 +370,26 @@ void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
} 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());
diff --git a/clang/test/Analysis/issue-124474.cpp b/clang/test/Analysis/issue-124474.cpp
new file mode 100644
index 0000000000000..09e3d4f3f9ad9
--- /dev/null
+++ b/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);
+}
More information about the cfe-commits
mailing list