[clang][analyzer] Teach the BlockInCriticalSectionChecker about O_NONBLOCK streams #127049
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: None (flovent) Changesthis PR close #124474 Full diff: https://github.com/llvm/llvm-project/pull/127049.diff 2 Files Affected:
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);
+}
|
|
i did something not right about git, so i closed before PR #126752
|
Xazax-hun
approved these changes
Feb 13, 2025
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
steakhal
reviewed
Feb 13, 2025
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
steakhal
changed the title
steakhal
approved these changes
Feb 16, 2025
steakhal
changed the title
|
Can I merge this? @flovent @Xazax-hun |
these commits looks very good, I don't mind at all, and happy to have my first commit to CSA.
I am ok to merge |
Xazax-hun
reviewed
Feb 17, 2025
Xazax-hun
approved these changes
Feb 17, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/81/builds/4823 Here is the relevant piece of the build log for the reference |
This doesn't seem to be relevant. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this PR close #124474
when calling
readandrecvfunction 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
openfunction withO_NONBLOCKflag.