Skip to content

[clang][analyzer] Split NullDereferenceChecker into modeling and reporting #122139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 10, 2025
Merged

[clang][analyzer] Split NullDereferenceChecker into modeling and reporting #122139

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
135839a
[clang][analyzer] Split NullDereferenceChecker into a modeling and ch…
balazske Jan 8, 2025
580b864
fixed formatting error
balazske Jan 8, 2025
a8053df
fixed unused variable and checker specification
balazske Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@NagyDonat NagyDonat Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is HasDocumentation compatible with Hidden? My understanding was that Hidden checkers are purely implementation details and they don't have their separate documentation.

Copy link
Collaborator Author

@balazske balazske Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


def NullDereferenceChecker : Checker<"NullDereference">,
HelpText<"Check for dereferences of null pointers">,
CheckerOptions<[
CmdLineOption<Boolean,
Expand All @@ -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 "
Expand Down
51 changes: 37 additions & 14 deletions clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -351,12 +356,30 @@ 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",
Comment on lines +372 to +379
Copy link
Contributor

@NagyDonat NagyDonat Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

categories::LogicError));
}

bool ento::shouldRegisterDereferenceChecker(const CheckerManager &mgr) {
bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &Mgr) {
return true;
}
1 change: 1 addition & 0 deletions clang/test/Analysis/analyzer-enabled-checkers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading