[clang][analyzer] Split NullDereferenceChecker into modeling and reporting #122139
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
…ecker part. The checker currently reports beneath the null dereference dereferences of undefined value and of label addresses. If we want to add more kinds of invalid dereferences (or split the existing functionality) it is more useful to make it separate checkers. To make this possible the existing checker is split into a DereferenceModeling part and a NullDereference checker that actually only switches on the check of null dereference. This is similar architecture as in MallocChecker and CStringChecker. The change is almost NFC but a new (modeling) checker is added. If the NullDereference checker is turned off the found invalid dereferences will still stop the analysis without emitted warning (this is different compared to the old behavior).
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesThe checker currently reports beneath the null dereference dereferences of undefined value and of label addresses. If we want to add more kinds of invalid dereferences (or split the existing functionality) it is more useful to make it separate checkers. The change is almost NFC but a new (modeling) checker is added. If the NullDereference checker is turned off the found invalid dereferences will still stop the analysis without emitted warning (this is different compared to the old behavior). Full diff: https://github.com/llvm/llvm-project/pull/122139.diff 4 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index b34e940682fc5e..fa2af57f1a09b4 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -206,7 +206,12 @@ def CallAndMessageChecker : Checker<"CallAndMessage">,
Documentation<HasDocumentation>,
Dependencies<[CallAndMessageModeling]>;
-def DereferenceChecker : Checker<"NullDereference">,
+def DereferenceModeling : Checker<"DereferenceModeling">,
+ HelpText<"General support for dereference related checkers">,
+ Documentation<HasDocumentation>,
+ Hidden;
+
+def NullDereferenceChecker : Checker<"NullDereference">,
HelpText<"Check for dereferences of null pointers">,
CheckerOptions<[
CmdLineOption<Boolean,
@@ -215,7 +220,8 @@ def DereferenceChecker : Checker<"NullDereference">,
"true",
Released>
]>,
- Documentation<HasDocumentation>;
+ Documentation<HasDocumentation>,
+ Dependencies<[DereferenceModeling]>;
def NonNullParamChecker : Checker<"NonNullParamChecker">,
HelpText<"Check for null pointers passed as arguments to a function whose "
diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
index 0355eede75eae7..dbcfe1f9b06fb6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -33,12 +33,6 @@ class DereferenceChecker
EventDispatcher<ImplicitNullDerefEvent> > {
enum DerefKind { NullPointer, UndefinedPointerValue, AddressOfLabel };
- BugType BT_Null{this, "Dereference of null pointer", categories::LogicError};
- BugType BT_Undef{this, "Dereference of undefined pointer value",
- categories::LogicError};
- BugType BT_Label{this, "Dereference of the address of a label",
- categories::LogicError};
-
void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
CheckerContext &C) const;
@@ -56,6 +50,12 @@ class DereferenceChecker
bool loadedFrom = false);
bool SuppressAddressSpaces = false;
+
+ bool CheckNullDereference = false;
+
+ std::unique_ptr<BugType> BT_Null;
+ std::unique_ptr<BugType> BT_Undef;
+ std::unique_ptr<BugType> BT_Label;
};
} // end anonymous namespace
@@ -155,22 +155,27 @@ static bool isDeclRefExprToReference(const Expr *E) {
void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State,
const Stmt *S, CheckerContext &C) const {
+ if (!CheckNullDereference) {
+ C.addSink();
+ return;
+ }
+
const BugType *BT = nullptr;
llvm::StringRef DerefStr1;
llvm::StringRef DerefStr2;
switch (K) {
case DerefKind::NullPointer:
- BT = &BT_Null;
+ BT = BT_Null.get();
DerefStr1 = " results in a null pointer dereference";
DerefStr2 = " results in a dereference of a null pointer";
break;
case DerefKind::UndefinedPointerValue:
- BT = &BT_Undef;
+ BT = BT_Undef.get();
DerefStr1 = " results in an undefined pointer dereference";
DerefStr2 = " results in a dereference of an undefined pointer value";
break;
case DerefKind::AddressOfLabel:
- BT = &BT_Label;
+ BT = BT_Label.get();
DerefStr1 = " results in an undefined pointer dereference";
DerefStr2 = " results in a dereference of an address of a label";
break;
@@ -351,12 +356,24 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
C.addTransition(State, this);
}
-void ento::registerDereferenceChecker(CheckerManager &mgr) {
- auto *Chk = mgr.registerChecker<DereferenceChecker>();
- Chk->SuppressAddressSpaces = mgr.getAnalyzerOptions().getCheckerBooleanOption(
- mgr.getCurrentCheckerName(), "SuppressAddressSpaces");
+void ento::registerDereferenceModeling(CheckerManager &Mgr) {
+ Mgr.registerChecker<DereferenceChecker>();
+}
+
+bool ento::shouldRegisterDereferenceModeling(const CheckerManager &Mgr) {
+ return true;
+}
+
+void ento::registerNullDereferenceChecker(CheckerManager &Mgr) {
+ auto *Chk = Mgr.getChecker<DereferenceChecker>();
+ Chk->CheckNullDereference = true;
+ Chk->SuppressAddressSpaces = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ Mgr.getCurrentCheckerName(), "SuppressAddressSpaces");
+ Chk->BT_Null.reset(new BugType(Mgr.getCurrentCheckerName(), "Dereference of null pointer", categories::LogicError));
+ Chk->BT_Undef.reset(new BugType(Mgr.getCurrentCheckerName(), "Dereference of undefined pointer value", categories::LogicError));
+ Chk->BT_Label.reset(new BugType(Mgr.getCurrentCheckerName(), "Dereference of the address of a label", categories::LogicError));
}
-bool ento::shouldRegisterDereferenceChecker(const CheckerManager &mgr) {
+bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &Mgr) {
return true;
}
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 160e35c77462d1..c70aeb21ab0459 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -14,6 +14,7 @@
// CHECK-NEXT: core.BitwiseShift
// CHECK-NEXT: core.CallAndMessageModeling
// CHECK-NEXT: core.CallAndMessage
+// CHECK-NEXT: core.DereferenceModeling
// CHECK-NEXT: core.DivideZero
// CHECK-NEXT: core.DynamicTypePropagation
// CHECK-NEXT: core.NonNullParamChecker
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 0910f030b0f072..faf0a8f19d919e 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -22,6 +22,7 @@
// CHECK-NEXT: core.BitwiseShift
// CHECK-NEXT: core.CallAndMessageModeling
// CHECK-NEXT: core.CallAndMessage
+// CHECK-NEXT: core.DereferenceModeling
// CHECK-NEXT: core.DivideZero
// CHECK-NEXT: core.DynamicTypePropagation
// CHECK-NEXT: core.NonNullParamChecker
|
Collaborator
Author
|
I wanted to avoid change the current behavior of |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Collaborator
Author
|
Our plan is to add a new check for dereference of fixed address (like |
NagyDonat
approved these changes
Jan 9, 2025
Comment on lines
210
to
212
| HelpText<"General support for dereference related checkers">, | ||
| Documentation<HasDocumentation>, | ||
| Hidden; |
Contributor
There was a problem hiding this comment.
Is HasDocumentation compatible with Hidden? My understanding was that Hidden checkers are purely implementation details and they don't have their separate documentation.
Collaborator
Author
There was a problem hiding this comment.
Probably NotDocumented is better, I updated it. The code was copied from another example in the file, probably CallAndMessageModeling where still HasDocumentation is used (but it has no documentation).
Comment on lines
+372
to
+379
| Chk->BT_Null.reset(new BugType(Mgr.getCurrentCheckerName(), | ||
| "Dereference of null pointer", | ||
| categories::LogicError)); | ||
| Chk->BT_Undef.reset(new BugType(Mgr.getCurrentCheckerName(), | ||
| "Dereference of undefined pointer value", | ||
| categories::LogicError)); | ||
| Chk->BT_Label.reset(new BugType(Mgr.getCurrentCheckerName(), | ||
| "Dereference of the address of a label", |
Contributor
There was a problem hiding this comment.
It's a pity that this commit reintroduces this dynamic initialization boilerplate. We should really introduce a clear framework for supporting bug types in a checker class that implements multiple sub-checkers...
(This is just a general long-term complaint, your change is probably the best possible short-term solution...)
steakhal
changed the title
balazske
changed the title
steakhal
approved these changes
Jan 10, 2025
BaiXilin
pushed a commit
to BaiXilin/llvm-project
that referenced
this pull request
Jan 12, 2025
…rting (llvm#122139) The checker currently reports beneath the null dereference dereferences of undefined value and of label addresses. If we want to add more kinds of invalid dereferences (or split the existing functionality) it is more useful to make it separate checkers. To make this possible the existing checker is split into a DereferenceModeling part and a NullDereference checker that actually only switches on the check of null dereference. This is similar architecture as in MallocChecker and CStringChecker. The change is almost NFC but a new (modeling) checker is added. If the NullDereference checker is turned off the found invalid dereferences will still stop the analysis without emitted warning (this is different compared to the old behavior).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The checker currently reports beneath the null dereference dereferences of undefined value and of label addresses. If we want to add more kinds of invalid dereferences (or split the existing functionality) it is more useful to make it separate checkers.
To make this possible the existing checker is split into a DereferenceModeling part and a NullDereference checker that actually only switches on the check of null dereference. This is similar architecture as in MallocChecker and CStringChecker.
The change is almost NFC but a new (modeling) checker is added. If the NullDereference checker is turned off the found invalid dereferences will still stop the analysis without emitted warning (this is different compared to the old behavior).