[clang] [analyzer] Modernize FuchsiaHandleChecker (PR #111588)
Pavel Skripkin via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 9 08:46:54 PDT 2024
https://github.com/pskrgag updated https://github.com/llvm/llvm-project/pull/111588
>From a3805292ea37cf06d1cf227768034b30a42a685f Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Mon, 7 Oct 2024 23:01:24 +0300
Subject: [PATCH 1/5] wip: initial versio
---
.../Checkers/FuchsiaHandleChecker.cpp | 429 ++++++++++++------
clang/test/Analysis/fuchsia_handle.cpp | 19 +-
2 files changed, 310 insertions(+), 138 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index 079bc61a87d963..9caa2462907c9c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -99,6 +99,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
#include "llvm/ADT/StringExtras.h"
#include <optional>
@@ -115,7 +116,14 @@ class HandleState {
private:
enum class Kind { MaybeAllocated, Allocated, Released, Escaped, Unowned } K;
SymbolRef ErrorSym;
- HandleState(Kind K, SymbolRef ErrorSym) : K(K), ErrorSym(ErrorSym) {}
+
+ // Paramater index through which handle was allocated.
+ //
+ // Already normalized for notes: i.e. 0 means returned via return value, while
+ // > 0 indicates parameter index.
+ unsigned ParamIdx;
+ HandleState(Kind K, SymbolRef ErrorSym, unsigned PI)
+ : K(K), ErrorSym(ErrorSym), ParamIdx(PI) {}
public:
bool operator==(const HandleState &Other) const {
@@ -127,26 +135,30 @@ class HandleState {
bool isEscaped() const { return K == Kind::Escaped; }
bool isUnowned() const { return K == Kind::Unowned; }
- static HandleState getMaybeAllocated(SymbolRef ErrorSym) {
- return HandleState(Kind::MaybeAllocated, ErrorSym);
+ static HandleState getMaybeAllocated(SymbolRef ErrorSym, std::uint8_t Idx) {
+ return HandleState(Kind::MaybeAllocated, ErrorSym, Idx);
}
- static HandleState getAllocated(ProgramStateRef State, HandleState S) {
+ static HandleState getAllocated(ProgramStateRef State, HandleState S,
+ unsigned Idx) {
assert(S.maybeAllocated());
assert(State->getConstraintManager()
.isNull(State, S.getErrorSym())
.isConstrained());
- return HandleState(Kind::Allocated, nullptr);
+ return HandleState(Kind::Allocated, nullptr, Idx);
}
- static HandleState getReleased() {
- return HandleState(Kind::Released, nullptr);
+ static HandleState getReleased(unsigned Idx) {
+ assert(Idx != 0 && "Wrong index for handle release");
+ return HandleState(Kind::Released, nullptr, Idx);
}
static HandleState getEscaped() {
- return HandleState(Kind::Escaped, nullptr);
+ return HandleState(Kind::Escaped, nullptr, 0);
}
- static HandleState getUnowned() {
- return HandleState(Kind::Unowned, nullptr);
+ static HandleState getUnowned(unsigned Idx) {
+ return HandleState(Kind::Unowned, nullptr, Idx);
}
+ unsigned getIndex() const { return ParamIdx; }
+
SymbolRef getErrorSym() const { return ErrorSym; }
void Profile(llvm::FoldingSetNodeID &ID) const {
@@ -185,8 +197,8 @@ template <typename Attr> static bool hasFuchsiaUnownedAttr(const Decl *D) {
}
class FuchsiaHandleChecker
- : public Checker<check::PostCall, check::PreCall, check::DeadSymbols,
- check::PointerEscape, eval::Assume> {
+ : public Checker<check::PostCall, check::PreCall, eval::Call,
+ check::DeadSymbols, check::PointerEscape, eval::Assume> {
BugType LeakBugType{this, "Fuchsia handle leak", "Fuchsia Handle Error",
/*SuppressOnSink=*/true};
BugType DoubleReleaseBugType{this, "Fuchsia handle double release",
@@ -198,6 +210,13 @@ class FuchsiaHandleChecker
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+ bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+
+ ProgramStateRef evalFunctionAttrs(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const;
+ ProgramStateRef evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const;
+ bool needsInvalidate(const CallEvent &Call) const;
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
@@ -267,12 +286,128 @@ class FuchsiaHandleSymbolVisitor final : public SymbolVisitor {
private:
SmallVector<SymbolRef, 1024> Symbols;
};
+
+class FuchsiaBugVisitor final : public BugReporterVisitor {
+ // Handle that caused a problem.
+ SymbolRef Sym;
+
+ bool IsLeak;
+
+public:
+ FuchsiaBugVisitor(SymbolRef S, bool Leak = false) : Sym(S), IsLeak(Leak) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.AddPointer(Sym);
+ }
+
+ static inline bool isAllocated(const HandleState *RSCurr,
+ const HandleState *RSPrev, const Stmt *Stmt) {
+ return isa_and_nonnull<CallExpr>(Stmt) &&
+ ((RSCurr && (RSCurr->isAllocated() || RSCurr->maybeAllocated()) &&
+ (!RSPrev ||
+ !(RSPrev->isAllocated() || RSPrev->maybeAllocated()))));
+ }
+
+ static inline bool isAllocatedUnowned(const HandleState *RSCurr,
+ const HandleState *RSPrev,
+ const Stmt *Stmt) {
+ return isa_and_nonnull<CallExpr>(Stmt) &&
+ ((RSCurr && RSCurr->isUnowned()) &&
+ (!RSPrev || !RSPrev->isUnowned()));
+ }
+
+ static inline bool isReleased(const HandleState *RSCurr,
+ const HandleState *RSPrev, const Stmt *Stmt) {
+ return isa_and_nonnull<CallExpr>(Stmt) &&
+ ((RSCurr && RSCurr->isReleased()) &&
+ (!RSPrev || !RSPrev->isReleased()));
+ }
+
+ PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) override;
+
+ PathDiagnosticPieceRef getEndPath(BugReporterContext &BRC,
+ const ExplodedNode *EndPathNode,
+ PathSensitiveBugReport &BR) override {
+ if (!IsLeak)
+ return nullptr;
+
+ PathDiagnosticLocation L = BR.getLocation();
+ // Do not add the statement itself as a range in case of leak.
+ return std::make_shared<PathDiagnosticEventPiece>(L, BR.getDescription(),
+ false);
+ }
+};
} // end anonymous namespace
+PathDiagnosticPieceRef
+FuchsiaBugVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) {
+ ProgramStateRef State = N->getState();
+ ProgramStateRef StatePrev = N->getFirstPred()->getState();
+
+ const HandleState *RSCurr = State->get<HStateMap>(Sym);
+ const HandleState *RSPrev = StatePrev->get<HStateMap>(Sym);
+
+ const Stmt *S = N->getStmtForDiagnostics();
+
+ StringRef Msg;
+ SmallString<256> Buf;
+ llvm::raw_svector_ostream OS(Buf);
+
+ if (isAllocated(RSCurr, RSPrev, S)) {
+ auto Index = RSCurr->getIndex();
+
+ if (Index > 0)
+ OS << "Handle allocated through " << Index
+ << llvm::getOrdinalSuffix(Index) << " parameter";
+ else
+ OS << "Function returns an open handle";
+
+ Msg = OS.str();
+ } else if (isAllocatedUnowned(RSCurr, RSPrev, S)) {
+ auto Index = RSCurr->getIndex();
+
+ if (Index > 0)
+ OS << "Unowned handle allocated through " << Index
+ << llvm::getOrdinalSuffix(Index) << " parameter";
+ else
+ OS << "Function returns an unowned handle";
+
+ Msg = OS.str();
+ } else if (isReleased(RSCurr, RSPrev, S)) {
+ auto Index = RSCurr->getIndex();
+
+ assert(Index > 0);
+
+ OS << "Handle released through " << Index << llvm::getOrdinalSuffix(Index)
+ << " parameter";
+ Msg = OS.str();
+ }
+
+ if (Msg.empty())
+ return nullptr;
+
+ PathDiagnosticLocation Pos;
+ if (!S) {
+ auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
+ if (!PostImplCall)
+ return nullptr;
+ Pos = PathDiagnosticLocation(PostImplCall->getLocation(),
+ BRC.getSourceManager());
+ } else {
+ Pos = PathDiagnosticLocation(S, BRC.getSourceManager(),
+ N->getLocationContext());
+ }
+
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true);
+}
+
/// Returns the symbols extracted from the argument or empty vector if it cannot
/// be found. It is unlikely to have over 1024 symbols in one argument.
-static SmallVector<SymbolRef, 1024>
-getFuchsiaHandleSymbols(QualType QT, SVal Arg, ProgramStateRef State) {
+SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
+ ProgramStateRef &State) {
int PtrToHandleLevel = 0;
while (QT->isAnyPointerType() || QT->isReferenceType()) {
++PtrToHandleLevel;
@@ -314,6 +449,127 @@ getFuchsiaHandleSymbols(QualType QT, SVal Arg, ProgramStateRef State) {
return {};
}
+bool FuchsiaHandleChecker::needsInvalidate(const CallEvent &Call) const {
+ const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+
+ assert(FuncDecl && "Should have FuncDecl at this point");
+
+ for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
+ if (Arg >= FuncDecl->getNumParams())
+ break;
+ const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
+
+ if (PVD->getType()->isAnyPointerType() &&
+ (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
+ hasFuchsiaAttr<AcquireHandleAttr>(PVD) ||
+ hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD))) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+ProgramStateRef
+FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const {
+ const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+ SymbolRef ResultSymbol = nullptr;
+
+ assert(FuncDecl && "Should have FuncDecl at this point");
+
+ if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
+ if (TypeDefTy->getDecl()->getName() == ErrorTypeName) {
+ SValBuilder &SVB = C.getSValBuilder();
+ const LocationContext *LC = C.getLocationContext();
+ auto RetVal = SVB.conjureSymbolVal(
+ Call.getOriginExpr(), LC, FuncDecl->getReturnType(), C.blockCount());
+
+ State = State->BindExpr(Call.getOriginExpr(), LC, RetVal);
+ ResultSymbol = RetVal.getAsSymbol();
+ }
+
+ for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
+ if (Arg >= FuncDecl->getNumParams())
+ break;
+ const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
+ SmallVector<SymbolRef, 1024> Handles =
+ getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
+
+ for (SymbolRef Handle : Handles) {
+ if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
+ State =
+ State->set<HStateMap>(Handle, HandleState::getReleased(Arg + 1));
+ } else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
+ State = State->set<HStateMap>(
+ Handle, HandleState::getMaybeAllocated(ResultSymbol, Arg + 1));
+ } else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD)) {
+ State = State->set<HStateMap>(Handle, HandleState::getUnowned(Arg + 1));
+ }
+ }
+ }
+
+ return State;
+}
+
+ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(
+ const CallEvent &Call, CheckerContext &C, ProgramStateRef State) const {
+ const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+
+ assert(FuncDecl && "Should have FuncDecl at this point");
+
+ if (!hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl) &&
+ !hasFuchsiaUnownedAttr<AcquireHandleAttr>(FuncDecl))
+ return State;
+
+ SValBuilder &SVB = C.getSValBuilder();
+ const LocationContext *LC = C.getLocationContext();
+ auto RetVal = SVB.conjureSymbolVal(Call.getOriginExpr(), LC,
+ FuncDecl->getReturnType(), C.blockCount());
+ State = State->BindExpr(Call.getOriginExpr(), LC, RetVal);
+
+ SymbolRef RetSym = RetVal.getAsSymbol();
+
+ assert(RetSym && "Symbol should be there");
+
+ if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
+ State = State->set<HStateMap>(RetSym,
+ HandleState::getMaybeAllocated(nullptr, 0));
+ } else {
+ State = State->set<HStateMap>(RetSym, HandleState::getUnowned(0));
+ }
+
+ return State;
+}
+
+bool FuchsiaHandleChecker::evalCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+ // Checker depends on attributes attached to function definition, so there is
+ // no way to proccess futher w/o declaration.
+ if (!FuncDecl)
+ return false;
+
+ ProgramStateRef State = C.getState();
+ ProgramStateRef OldState = State;
+
+ if (needsInvalidate(Call)) {
+ // If checker models a call, then body won't be inlined, so
+ // all pointers (including annotated ones) should be invalidated.
+ //
+ // This call will also create a conjured symbol for each reference,
+ // which then will be obtained by getFuchsiaHandleSymbols().
+ State = Call.invalidateRegions(C.blockCount(), State);
+ }
+
+ State = evalFunctionAttrs(Call, C, State);
+ State = evalArgsAttrs(Call, C, State);
+
+ C.addTransition(State);
+
+ return State != OldState;
+}
+
void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
@@ -336,84 +592,45 @@ void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
SmallVector<SymbolRef, 1024> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
- // Handled in checkPostCall.
- if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
- hasFuchsiaAttr<AcquireHandleAttr>(PVD))
- continue;
-
for (SymbolRef Handle : Handles) {
const HandleState *HState = State->get<HStateMap>(Handle);
- if (!HState || HState->isEscaped())
+ if (HState && HState->isEscaped())
continue;
-
- if (hasFuchsiaAttr<UseHandleAttr>(PVD) ||
- PVD->getType()->isIntegerType()) {
- if (HState->isReleased()) {
+ if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
+ if (HState && HState->isReleased()) {
+ reportDoubleRelease(Handle, Call.getArgSourceRange(Arg), C);
+ return;
+ } else if (HState && HState->isUnowned()) {
+ reportUnownedRelease(Handle, Call.getArgSourceRange(Arg), C);
+ return;
+ }
+ } else if (hasFuchsiaAttr<UseHandleAttr>(PVD) ||
+ PVD->getType()->isIntegerType()) {
+ if (HState && HState->isReleased()) {
reportUseAfterFree(Handle, Call.getArgSourceRange(Arg), C);
return;
}
}
}
}
+
C.addTransition(State);
}
void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
if (!FuncDecl)
return;
- // If we analyzed the function body, then ignore the annotations.
if (C.wasInlined)
return;
- ProgramStateRef State = C.getState();
-
- std::vector<std::function<std::string(BugReport & BR)>> Notes;
- SymbolRef ResultSymbol = nullptr;
- if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
- if (TypeDefTy->getDecl()->getName() == ErrorTypeName)
- ResultSymbol = Call.getReturnValue().getAsSymbol();
-
- // Function returns an open handle.
- if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
- SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
- Notes.push_back([RetSym, FuncDecl](BugReport &BR) -> std::string {
- auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
- if (PathBR->getInterestingnessKind(RetSym)) {
- std::string SBuf;
- llvm::raw_string_ostream OS(SBuf);
- OS << "Function '" << FuncDecl->getDeclName()
- << "' returns an open handle";
- return SBuf;
- } else
- return "";
- });
- State =
- State->set<HStateMap>(RetSym, HandleState::getMaybeAllocated(nullptr));
- } else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(FuncDecl)) {
- // Function returns an unowned handle
- SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
- Notes.push_back([RetSym, FuncDecl](BugReport &BR) -> std::string {
- auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
- if (PathBR->getInterestingnessKind(RetSym)) {
- std::string SBuf;
- llvm::raw_string_ostream OS(SBuf);
- OS << "Function '" << FuncDecl->getDeclName()
- << "' returns an unowned handle";
- return SBuf;
- } else
- return "";
- });
- State = State->set<HStateMap>(RetSym, HandleState::getUnowned());
- }
-
for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
- unsigned ParamDiagIdx = PVD->getFunctionScopeIndex() + 1;
SmallVector<SymbolRef, 1024> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
@@ -421,56 +638,9 @@ void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
const HandleState *HState = State->get<HStateMap>(Handle);
if (HState && HState->isEscaped())
continue;
- if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
- if (HState && HState->isReleased()) {
- reportDoubleRelease(Handle, Call.getArgSourceRange(Arg), C);
- return;
- } else if (HState && HState->isUnowned()) {
- reportUnownedRelease(Handle, Call.getArgSourceRange(Arg), C);
- return;
- } else {
- Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
- auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
- if (PathBR->getInterestingnessKind(Handle)) {
- std::string SBuf;
- llvm::raw_string_ostream OS(SBuf);
- OS << "Handle released through " << ParamDiagIdx
- << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
- return SBuf;
- } else
- return "";
- });
- State = State->set<HStateMap>(Handle, HandleState::getReleased());
- }
- } else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
- Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
- auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
- if (PathBR->getInterestingnessKind(Handle)) {
- std::string SBuf;
- llvm::raw_string_ostream OS(SBuf);
- OS << "Handle allocated through " << ParamDiagIdx
- << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
- return SBuf;
- } else
- return "";
- });
- State = State->set<HStateMap>(
- Handle, HandleState::getMaybeAllocated(ResultSymbol));
- } else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD)) {
- Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
- auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
- if (PathBR->getInterestingnessKind(Handle)) {
- std::string SBuf;
- llvm::raw_string_ostream OS(SBuf);
- OS << "Unowned handle allocated through " << ParamDiagIdx
- << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
- return SBuf;
- } else
- return "";
- });
- State = State->set<HStateMap>(Handle, HandleState::getUnowned());
- } else if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
- PVD->getType()->isIntegerType()) {
+ if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
+ !hasFuchsiaAttr<ReleaseHandleAttr>(PVD) &&
+ PVD->getType()->isIntegerType()) {
// Working around integer by-value escapes.
// The by-value escape would not be captured in checkPointerEscape.
// If the function was not analyzed (otherwise wasInlined should be
@@ -480,24 +650,8 @@ void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
}
}
}
- const NoteTag *T = nullptr;
- if (!Notes.empty()) {
- T = C.getNoteTag([this, Notes{std::move(Notes)}](
- PathSensitiveBugReport &BR) -> std::string {
- if (&BR.getBugType() != &UseAfterReleaseBugType &&
- &BR.getBugType() != &LeakBugType &&
- &BR.getBugType() != &DoubleReleaseBugType &&
- &BR.getBugType() != &ReleaseUnownedBugType)
- return "";
- for (auto &Note : Notes) {
- std::string Text = Note(BR);
- if (!Text.empty())
- return Text;
- }
- return "";
- });
- }
- C.addTransition(State, T);
+
+ C.addTransition(State);
}
void FuchsiaHandleChecker::checkDeadSymbols(SymbolReaper &SymReaper,
@@ -556,7 +710,9 @@ ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
// Allocation succeeded.
if (CurItem.second.maybeAllocated())
State = State->set<HStateMap>(
- CurItem.first, HandleState::getAllocated(State, CurItem.second));
+ CurItem.first,
+ HandleState::getAllocated(State, CurItem.second,
+ CurItem.second.getIndex()));
} else if (ErrorVal.isConstrainedFalse()) {
// Allocation failed.
if (CurItem.second.maybeAllocated())
@@ -669,6 +825,7 @@ void FuchsiaHandleChecker::reportBug(SymbolRef Sym, ExplodedNode *ErrorNode,
if (Range)
R->addRange(*Range);
R->markInteresting(Sym);
+ R->addVisitor<FuchsiaBugVisitor>(Sym, &Type == &LeakBugType);
C.emitReport(std::move(R));
}
diff --git a/clang/test/Analysis/fuchsia_handle.cpp b/clang/test/Analysis/fuchsia_handle.cpp
index f86cc50df045da..71918e7f48f06c 100644
--- a/clang/test/Analysis/fuchsia_handle.cpp
+++ b/clang/test/Analysis/fuchsia_handle.cpp
@@ -28,6 +28,13 @@ zx_status_t zx_channel_create(
zx_status_t zx_handle_close(
zx_handle_t handle ZX_HANDLE_RELEASE);
+zx_status_t zx_handle_close_inline(
+ zx_handle_t handle ZX_HANDLE_RELEASE)
+{
+ /* Doing smth important, like calling syscall */
+ return 0;
+}
+
ZX_HANDLE_ACQUIRE_UNOWNED
zx_handle_t zx_process_self();
@@ -54,7 +61,7 @@ struct MyType {
void checkUnownedHandle01() {
zx_handle_t h0;
- h0 = zx_process_self(); // expected-note {{Function 'zx_process_self' returns an unowned handle}}
+ h0 = zx_process_self(); // expected-note {{Function returns an unowned handle}}
zx_handle_close(h0); // expected-warning {{Releasing an unowned handle}}
// expected-note at -1 {{Releasing an unowned handle}}
}
@@ -175,8 +182,16 @@ void checkLeak01(int tag) {
zx_handle_close(sb);
}
+void checkLeakInline(int tag) {
+ zx_handle_t sa, sb;
+ if (zx_channel_create(0, &sa, &sb))
+ return;
+ zx_handle_close_inline(sa);
+ zx_handle_close_inline(sb);
+} // No leak warnings
+
void checkLeakFromReturn01(int tag) {
- zx_handle_t sa = return_handle(); // expected-note {{Function 'return_handle' returns an open handle}}
+ zx_handle_t sa = return_handle(); // expected-note {{Function returns an open handle}}
(void)sa;
} // expected-note {{Potential leak of handle}}
// expected-warning at -1 {{Potential leak of handle}}
>From c746b401a7335a0869837bae5d3b162468a45454 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Wed, 9 Oct 2024 16:48:07 +0300
Subject: [PATCH 2/5] rollback notes creating
---
.../Checkers/FuchsiaHandleChecker.cpp | 259 +++++++-----------
clang/test/Analysis/fuchsia_handle.cpp | 4 +-
2 files changed, 104 insertions(+), 159 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index 9caa2462907c9c..ec81c3191439c8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -117,13 +117,7 @@ class HandleState {
enum class Kind { MaybeAllocated, Allocated, Released, Escaped, Unowned } K;
SymbolRef ErrorSym;
- // Paramater index through which handle was allocated.
- //
- // Already normalized for notes: i.e. 0 means returned via return value, while
- // > 0 indicates parameter index.
- unsigned ParamIdx;
- HandleState(Kind K, SymbolRef ErrorSym, unsigned PI)
- : K(K), ErrorSym(ErrorSym), ParamIdx(PI) {}
+ HandleState(Kind K, SymbolRef ErrorSym) : K(K), ErrorSym(ErrorSym) {}
public:
bool operator==(const HandleState &Other) const {
@@ -135,30 +129,26 @@ class HandleState {
bool isEscaped() const { return K == Kind::Escaped; }
bool isUnowned() const { return K == Kind::Unowned; }
- static HandleState getMaybeAllocated(SymbolRef ErrorSym, std::uint8_t Idx) {
- return HandleState(Kind::MaybeAllocated, ErrorSym, Idx);
+ static HandleState getMaybeAllocated(SymbolRef ErrorSym) {
+ return HandleState(Kind::MaybeAllocated, ErrorSym);
}
- static HandleState getAllocated(ProgramStateRef State, HandleState S,
- unsigned Idx) {
+ static HandleState getAllocated(ProgramStateRef State, HandleState S) {
assert(S.maybeAllocated());
assert(State->getConstraintManager()
.isNull(State, S.getErrorSym())
.isConstrained());
- return HandleState(Kind::Allocated, nullptr, Idx);
+ return HandleState(Kind::Allocated, nullptr);
}
- static HandleState getReleased(unsigned Idx) {
- assert(Idx != 0 && "Wrong index for handle release");
- return HandleState(Kind::Released, nullptr, Idx);
+ static HandleState getReleased() {
+ return HandleState(Kind::Released, nullptr);
}
static HandleState getEscaped() {
- return HandleState(Kind::Escaped, nullptr, 0);
+ return HandleState(Kind::Escaped, nullptr);
}
- static HandleState getUnowned(unsigned Idx) {
- return HandleState(Kind::Unowned, nullptr, Idx);
+ static HandleState getUnowned() {
+ return HandleState(Kind::Unowned, nullptr);
}
- unsigned getIndex() const { return ParamIdx; }
-
SymbolRef getErrorSym() const { return ErrorSym; }
void Profile(llvm::FoldingSetNodeID &ID) const {
@@ -209,13 +199,17 @@ class FuchsiaHandleChecker
this, "Fuchsia handle release of unowned handle", "Fuchsia Handle Error"};
public:
+ using ReportNote = std::function<std::string(BugReport &BR)>;
+ using NotesVec = SmallVector<ReportNote, 3>;
+
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
ProgramStateRef evalFunctionAttrs(const CallEvent &Call, CheckerContext &C,
- ProgramStateRef State) const;
+ ProgramStateRef State,
+ NotesVec &Notes) const;
ProgramStateRef evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
- ProgramStateRef State) const;
+ ProgramStateRef State, NotesVec &Notes) const;
bool needsInvalidate(const CallEvent &Call) const;
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
@@ -225,6 +219,13 @@ class FuchsiaHandleChecker
const InvalidatedSymbols &Escaped,
const CallEvent *Call,
PointerEscapeKind Kind) const;
+ ReportNote createReturnNote(
+ SymbolRef RetVal,
+ std::function<void(llvm::raw_string_ostream &)> Message) const;
+
+ ReportNote
+ createArgNote(SymbolRef RetVal, unsigned ParamIdx,
+ std::function<void(llvm::raw_string_ostream &)> Message) const;
ExplodedNode *reportLeaks(ArrayRef<SymbolRef> LeakedHandles,
CheckerContext &C, ExplodedNode *Pred) const;
@@ -286,124 +287,8 @@ class FuchsiaHandleSymbolVisitor final : public SymbolVisitor {
private:
SmallVector<SymbolRef, 1024> Symbols;
};
-
-class FuchsiaBugVisitor final : public BugReporterVisitor {
- // Handle that caused a problem.
- SymbolRef Sym;
-
- bool IsLeak;
-
-public:
- FuchsiaBugVisitor(SymbolRef S, bool Leak = false) : Sym(S), IsLeak(Leak) {}
-
- void Profile(llvm::FoldingSetNodeID &ID) const override {
- ID.AddPointer(Sym);
- }
-
- static inline bool isAllocated(const HandleState *RSCurr,
- const HandleState *RSPrev, const Stmt *Stmt) {
- return isa_and_nonnull<CallExpr>(Stmt) &&
- ((RSCurr && (RSCurr->isAllocated() || RSCurr->maybeAllocated()) &&
- (!RSPrev ||
- !(RSPrev->isAllocated() || RSPrev->maybeAllocated()))));
- }
-
- static inline bool isAllocatedUnowned(const HandleState *RSCurr,
- const HandleState *RSPrev,
- const Stmt *Stmt) {
- return isa_and_nonnull<CallExpr>(Stmt) &&
- ((RSCurr && RSCurr->isUnowned()) &&
- (!RSPrev || !RSPrev->isUnowned()));
- }
-
- static inline bool isReleased(const HandleState *RSCurr,
- const HandleState *RSPrev, const Stmt *Stmt) {
- return isa_and_nonnull<CallExpr>(Stmt) &&
- ((RSCurr && RSCurr->isReleased()) &&
- (!RSPrev || !RSPrev->isReleased()));
- }
-
- PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
- BugReporterContext &BRC,
- PathSensitiveBugReport &BR) override;
-
- PathDiagnosticPieceRef getEndPath(BugReporterContext &BRC,
- const ExplodedNode *EndPathNode,
- PathSensitiveBugReport &BR) override {
- if (!IsLeak)
- return nullptr;
-
- PathDiagnosticLocation L = BR.getLocation();
- // Do not add the statement itself as a range in case of leak.
- return std::make_shared<PathDiagnosticEventPiece>(L, BR.getDescription(),
- false);
- }
-};
} // end anonymous namespace
-PathDiagnosticPieceRef
-FuchsiaBugVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
- PathSensitiveBugReport &BR) {
- ProgramStateRef State = N->getState();
- ProgramStateRef StatePrev = N->getFirstPred()->getState();
-
- const HandleState *RSCurr = State->get<HStateMap>(Sym);
- const HandleState *RSPrev = StatePrev->get<HStateMap>(Sym);
-
- const Stmt *S = N->getStmtForDiagnostics();
-
- StringRef Msg;
- SmallString<256> Buf;
- llvm::raw_svector_ostream OS(Buf);
-
- if (isAllocated(RSCurr, RSPrev, S)) {
- auto Index = RSCurr->getIndex();
-
- if (Index > 0)
- OS << "Handle allocated through " << Index
- << llvm::getOrdinalSuffix(Index) << " parameter";
- else
- OS << "Function returns an open handle";
-
- Msg = OS.str();
- } else if (isAllocatedUnowned(RSCurr, RSPrev, S)) {
- auto Index = RSCurr->getIndex();
-
- if (Index > 0)
- OS << "Unowned handle allocated through " << Index
- << llvm::getOrdinalSuffix(Index) << " parameter";
- else
- OS << "Function returns an unowned handle";
-
- Msg = OS.str();
- } else if (isReleased(RSCurr, RSPrev, S)) {
- auto Index = RSCurr->getIndex();
-
- assert(Index > 0);
-
- OS << "Handle released through " << Index << llvm::getOrdinalSuffix(Index)
- << " parameter";
- Msg = OS.str();
- }
-
- if (Msg.empty())
- return nullptr;
-
- PathDiagnosticLocation Pos;
- if (!S) {
- auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
- if (!PostImplCall)
- return nullptr;
- Pos = PathDiagnosticLocation(PostImplCall->getLocation(),
- BRC.getSourceManager());
- } else {
- Pos = PathDiagnosticLocation(S, BRC.getSourceManager(),
- N->getLocationContext());
- }
-
- return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true);
-}
-
/// Returns the symbols extracted from the argument or empty vector if it cannot
/// be found. It is unlikely to have over 1024 symbols in one argument.
SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
@@ -449,6 +334,20 @@ SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
return {};
}
+FuchsiaHandleChecker::ReportNote FuchsiaHandleChecker::createReturnNote(
+ SymbolRef Sym,
+ std::function<void(llvm::raw_string_ostream &)> Message) const {
+ return [Sym, Message](BugReport &BR) -> std::string {
+ auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
+ if (!PathBR->getInterestingnessKind(Sym))
+ return "";
+ std::string SBuf;
+ llvm::raw_string_ostream OS(SBuf);
+ Message(OS);
+ return SBuf;
+ };
+}
+
bool FuchsiaHandleChecker::needsInvalidate(const CallEvent &Call) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
@@ -470,9 +369,10 @@ bool FuchsiaHandleChecker::needsInvalidate(const CallEvent &Call) const {
return false;
}
-ProgramStateRef
-FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
- ProgramStateRef State) const {
+ProgramStateRef FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call,
+ CheckerContext &C,
+ ProgramStateRef State,
+ NotesVec &Notes) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
SymbolRef ResultSymbol = nullptr;
@@ -493,18 +393,33 @@ FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
+ unsigned ParamDiagIdx = PVD->getFunctionScopeIndex() + 1;
SmallVector<SymbolRef, 1024> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
for (SymbolRef Handle : Handles) {
if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
- State =
- State->set<HStateMap>(Handle, HandleState::getReleased(Arg + 1));
+ Notes.push_back(createReturnNote(
+ Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ OS << "Handle released through " << ParamDiagIdx
+ << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
+ }));
+ State = State->set<HStateMap>(Handle, HandleState::getReleased());
} else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
+ Notes.push_back(createReturnNote(
+ Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ OS << "Handle allocated through " << ParamDiagIdx
+ << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
+ }));
State = State->set<HStateMap>(
- Handle, HandleState::getMaybeAllocated(ResultSymbol, Arg + 1));
+ Handle, HandleState::getMaybeAllocated(ResultSymbol));
} else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD)) {
- State = State->set<HStateMap>(Handle, HandleState::getUnowned(Arg + 1));
+ Notes.push_back(createReturnNote(
+ Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ OS << "Unowned handle allocated through " << ParamDiagIdx
+ << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
+ }));
+ State = State->set<HStateMap>(Handle, HandleState::getUnowned());
}
}
}
@@ -512,8 +427,10 @@ FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
return State;
}
-ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(
- const CallEvent &Call, CheckerContext &C, ProgramStateRef State) const {
+ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(const CallEvent &Call,
+ CheckerContext &C,
+ ProgramStateRef State,
+ NotesVec &Notes) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
assert(FuncDecl && "Should have FuncDecl at this point");
@@ -533,10 +450,20 @@ ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(
assert(RetSym && "Symbol should be there");
if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
- State = State->set<HStateMap>(RetSym,
- HandleState::getMaybeAllocated(nullptr, 0));
+ Notes.push_back(
+ createReturnNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
+ OS << "Function '" << FuncDecl->getDeclName()
+ << "' returns an open handle";
+ }));
+ State =
+ State->set<HStateMap>(RetSym, HandleState::getMaybeAllocated(nullptr));
} else {
- State = State->set<HStateMap>(RetSym, HandleState::getUnowned(0));
+ Notes.push_back(
+ createReturnNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
+ OS << "Function '" << FuncDecl->getDeclName()
+ << "' returns an unowned handle";
+ }));
+ State = State->set<HStateMap>(RetSym, HandleState::getUnowned());
}
return State;
@@ -562,11 +489,32 @@ bool FuchsiaHandleChecker::evalCall(const CallEvent &Call,
State = Call.invalidateRegions(C.blockCount(), State);
}
- State = evalFunctionAttrs(Call, C, State);
- State = evalArgsAttrs(Call, C, State);
+ NotesVec Notes;
- C.addTransition(State);
+ State = evalFunctionAttrs(Call, C, State, Notes);
+ State = evalArgsAttrs(Call, C, State, Notes);
+
+ const NoteTag *T = nullptr;
+ if (!Notes.empty()) {
+ assert(State != OldState && "State should have been changed");
+
+ T = C.getNoteTag([this, Notes{std::move(Notes)}](
+ PathSensitiveBugReport &BR) -> std::string {
+ if (&BR.getBugType() != &UseAfterReleaseBugType &&
+ &BR.getBugType() != &LeakBugType &&
+ &BR.getBugType() != &DoubleReleaseBugType &&
+ &BR.getBugType() != &ReleaseUnownedBugType)
+ return "";
+ for (auto &Note : Notes) {
+ std::string Text = Note(BR);
+ if (!Text.empty())
+ return Text;
+ }
+ return "";
+ });
+ }
+ C.addTransition(State, T);
return State != OldState;
}
@@ -710,9 +658,7 @@ ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
// Allocation succeeded.
if (CurItem.second.maybeAllocated())
State = State->set<HStateMap>(
- CurItem.first,
- HandleState::getAllocated(State, CurItem.second,
- CurItem.second.getIndex()));
+ CurItem.first, HandleState::getAllocated(State, CurItem.second));
} else if (ErrorVal.isConstrainedFalse()) {
// Allocation failed.
if (CurItem.second.maybeAllocated())
@@ -825,7 +771,6 @@ void FuchsiaHandleChecker::reportBug(SymbolRef Sym, ExplodedNode *ErrorNode,
if (Range)
R->addRange(*Range);
R->markInteresting(Sym);
- R->addVisitor<FuchsiaBugVisitor>(Sym, &Type == &LeakBugType);
C.emitReport(std::move(R));
}
diff --git a/clang/test/Analysis/fuchsia_handle.cpp b/clang/test/Analysis/fuchsia_handle.cpp
index 71918e7f48f06c..59a121da5b8cb6 100644
--- a/clang/test/Analysis/fuchsia_handle.cpp
+++ b/clang/test/Analysis/fuchsia_handle.cpp
@@ -61,7 +61,7 @@ struct MyType {
void checkUnownedHandle01() {
zx_handle_t h0;
- h0 = zx_process_self(); // expected-note {{Function returns an unowned handle}}
+ h0 = zx_process_self(); // expected-note {{Function 'zx_process_self' returns an unowned handle}}
zx_handle_close(h0); // expected-warning {{Releasing an unowned handle}}
// expected-note at -1 {{Releasing an unowned handle}}
}
@@ -191,7 +191,7 @@ void checkLeakInline(int tag) {
} // No leak warnings
void checkLeakFromReturn01(int tag) {
- zx_handle_t sa = return_handle(); // expected-note {{Function returns an open handle}}
+ zx_handle_t sa = return_handle(); // expected-note {{Function 'return_handle' returns an open handle}}
(void)sa;
} // expected-note {{Potential leak of handle}}
// expected-warning at -1 {{Potential leak of handle}}
>From 41592bae3dba035550d5041c5fa3abe4aefc05f7 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Wed, 9 Oct 2024 17:45:40 +0300
Subject: [PATCH 3/5] always bind return value to evaluated functions
---
.../Checkers/FuchsiaHandleChecker.cpp | 131 +++++++++---------
1 file changed, 65 insertions(+), 66 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index ec81c3191439c8..eca8b888d67014 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -199,8 +199,8 @@ class FuchsiaHandleChecker
this, "Fuchsia handle release of unowned handle", "Fuchsia Handle Error"};
public:
- using ReportNote = std::function<std::string(BugReport &BR)>;
- using NotesVec = SmallVector<ReportNote, 3>;
+ using Note = std::function<std::string(BugReport &BR)>;
+ using NotesVec = SmallVector<Note, 4>;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
@@ -210,7 +210,7 @@ class FuchsiaHandleChecker
NotesVec &Notes) const;
ProgramStateRef evalArgsAttrs(const CallEvent &Call, CheckerContext &C,
ProgramStateRef State, NotesVec &Notes) const;
- bool needsInvalidate(const CallEvent &Call) const;
+ bool needsEval(const CallEvent &Call) const;
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
@@ -219,13 +219,9 @@ class FuchsiaHandleChecker
const InvalidatedSymbols &Escaped,
const CallEvent *Call,
PointerEscapeKind Kind) const;
- ReportNote createReturnNote(
- SymbolRef RetVal,
- std::function<void(llvm::raw_string_ostream &)> Message) const;
-
- ReportNote
- createArgNote(SymbolRef RetVal, unsigned ParamIdx,
- std::function<void(llvm::raw_string_ostream &)> Message) const;
+ Note
+ createNote(SymbolRef RetVal,
+ std::function<void(llvm::raw_string_ostream &)> Message) const;
ExplodedNode *reportLeaks(ArrayRef<SymbolRef> LeakedHandles,
CheckerContext &C, ExplodedNode *Pred) const;
@@ -334,7 +330,7 @@ SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
return {};
}
-FuchsiaHandleChecker::ReportNote FuchsiaHandleChecker::createReturnNote(
+FuchsiaHandleChecker::Note FuchsiaHandleChecker::createNote(
SymbolRef Sym,
std::function<void(llvm::raw_string_ostream &)> Message) const {
return [Sym, Message](BugReport &BR) -> std::string {
@@ -348,22 +344,24 @@ FuchsiaHandleChecker::ReportNote FuchsiaHandleChecker::createReturnNote(
};
}
-bool FuchsiaHandleChecker::needsInvalidate(const CallEvent &Call) const {
+bool FuchsiaHandleChecker::needsEval(const CallEvent &Call) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
assert(FuncDecl && "Should have FuncDecl at this point");
+ if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl) ||
+ hasFuchsiaUnownedAttr<AcquireHandleAttr>(FuncDecl))
+ return true;
+
for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
- if (PVD->getType()->isAnyPointerType() &&
- (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
- hasFuchsiaAttr<AcquireHandleAttr>(PVD) ||
- hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD))) {
+ if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
+ hasFuchsiaAttr<AcquireHandleAttr>(PVD) ||
+ hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD))
return true;
- }
}
return false;
@@ -380,13 +378,10 @@ ProgramStateRef FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call,
if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
if (TypeDefTy->getDecl()->getName() == ErrorTypeName) {
- SValBuilder &SVB = C.getSValBuilder();
- const LocationContext *LC = C.getLocationContext();
- auto RetVal = SVB.conjureSymbolVal(
- Call.getOriginExpr(), LC, FuncDecl->getReturnType(), C.blockCount());
-
- State = State->BindExpr(Call.getOriginExpr(), LC, RetVal);
- ResultSymbol = RetVal.getAsSymbol();
+ ResultSymbol =
+ State->getSVal(Call.getOriginExpr(), C.getLocationContext())
+ .getAsSymbol();
+ assert(ResultSymbol && "Result symbol should be there");
}
for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
@@ -399,23 +394,23 @@ ProgramStateRef FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call,
for (SymbolRef Handle : Handles) {
if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
- Notes.push_back(createReturnNote(
- Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ Notes.push_back(
+ createNote(Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
OS << "Handle released through " << ParamDiagIdx
<< llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
}));
State = State->set<HStateMap>(Handle, HandleState::getReleased());
} else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
- Notes.push_back(createReturnNote(
- Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ Notes.push_back(
+ createNote(Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
OS << "Handle allocated through " << ParamDiagIdx
<< llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
}));
State = State->set<HStateMap>(
Handle, HandleState::getMaybeAllocated(ResultSymbol));
} else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD)) {
- Notes.push_back(createReturnNote(
- Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
+ Notes.push_back(
+ createNote(Handle, [ParamDiagIdx](llvm::raw_string_ostream &OS) {
OS << "Unowned handle allocated through " << ParamDiagIdx
<< llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
}));
@@ -432,26 +427,20 @@ ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(const CallEvent &Call,
ProgramStateRef State,
NotesVec &Notes) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
-
assert(FuncDecl && "Should have FuncDecl at this point");
if (!hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl) &&
!hasFuchsiaUnownedAttr<AcquireHandleAttr>(FuncDecl))
return State;
- SValBuilder &SVB = C.getSValBuilder();
- const LocationContext *LC = C.getLocationContext();
- auto RetVal = SVB.conjureSymbolVal(Call.getOriginExpr(), LC,
- FuncDecl->getReturnType(), C.blockCount());
- State = State->BindExpr(Call.getOriginExpr(), LC, RetVal);
-
- SymbolRef RetSym = RetVal.getAsSymbol();
-
- assert(RetSym && "Symbol should be there");
+ SymbolRef RetSym =
+ State->getSVal(Call.getOriginExpr(), C.getLocationContext())
+ .getAsSymbol();
+ assert(RetSym && "Return symbol should be there");
if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
Notes.push_back(
- createReturnNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
+ createNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
OS << "Function '" << FuncDecl->getDeclName()
<< "' returns an open handle";
}));
@@ -459,7 +448,7 @@ ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(const CallEvent &Call,
State->set<HStateMap>(RetSym, HandleState::getMaybeAllocated(nullptr));
} else {
Notes.push_back(
- createReturnNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
+ createNote(RetSym, [FuncDecl](llvm::raw_string_ostream &OS) {
OS << "Function '" << FuncDecl->getDeclName()
<< "' returns an unowned handle";
}));
@@ -472,21 +461,32 @@ ProgramStateRef FuchsiaHandleChecker::evalFunctionAttrs(const CallEvent &Call,
bool FuchsiaHandleChecker::evalCall(const CallEvent &Call,
CheckerContext &C) const {
const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
- // Checker depends on attributes attached to function definition, so there is
- // no way to proccess futher w/o declaration.
+ // Checker depends on attributes attached to function declaration, so there is
+ // no way to proccess futher w/o declaration
if (!FuncDecl)
return false;
ProgramStateRef State = C.getState();
- ProgramStateRef OldState = State;
- if (needsInvalidate(Call)) {
+ if (needsEval(Call)) {
+ // If return value is non-void then it's better to bind one
+ if (!FuncDecl->getReturnType()->isVoidType()) {
+ SValBuilder &SVB = C.getSValBuilder();
+ const LocationContext *LC = C.getLocationContext();
+ auto RetVal = SVB.conjureSymbolVal(
+ Call.getOriginExpr(), LC, FuncDecl->getReturnType(), C.blockCount());
+
+ State = State->BindExpr(Call.getOriginExpr(), LC, RetVal);
+ }
+
// If checker models a call, then body won't be inlined, so
// all pointers (including annotated ones) should be invalidated.
//
- // This call will also create a conjured symbol for each reference,
+ // This call will also create a conjured symbol for each handle symbol,
// which then will be obtained by getFuchsiaHandleSymbols().
State = Call.invalidateRegions(C.blockCount(), State);
+ } else {
+ return false;
}
NotesVec Notes;
@@ -494,28 +494,27 @@ bool FuchsiaHandleChecker::evalCall(const CallEvent &Call,
State = evalFunctionAttrs(Call, C, State, Notes);
State = evalArgsAttrs(Call, C, State, Notes);
- const NoteTag *T = nullptr;
- if (!Notes.empty()) {
- assert(State != OldState && "State should have been changed");
-
- T = C.getNoteTag([this, Notes{std::move(Notes)}](
- PathSensitiveBugReport &BR) -> std::string {
- if (&BR.getBugType() != &UseAfterReleaseBugType &&
- &BR.getBugType() != &LeakBugType &&
- &BR.getBugType() != &DoubleReleaseBugType &&
- &BR.getBugType() != &ReleaseUnownedBugType)
+ assert(!Notes.empty() && "Notes should be produced");
+
+ const NoteTag *T =
+ C.getNoteTag([this, Notes{std::move(Notes)}](
+ PathSensitiveBugReport &BR) -> std::string {
+ if (&BR.getBugType() != &UseAfterReleaseBugType &&
+ &BR.getBugType() != &LeakBugType &&
+ &BR.getBugType() != &DoubleReleaseBugType &&
+ &BR.getBugType() != &ReleaseUnownedBugType)
+ return "";
+ for (auto &Note : Notes) {
+ std::string Text = Note(BR);
+ if (!Text.empty())
+ return Text;
+ }
return "";
- for (auto &Note : Notes) {
- std::string Text = Note(BR);
- if (!Text.empty())
- return Text;
- }
- return "";
- });
+ });
}
C.addTransition(State, T);
- return State != OldState;
+ return true;
}
void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
@@ -542,7 +541,7 @@ void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
for (SymbolRef Handle : Handles) {
const HandleState *HState = State->get<HStateMap>(Handle);
- if (HState && HState->isEscaped())
+ if (!HState || HState->isEscaped())
continue;
if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
if (HState && HState->isReleased()) {
>From d4df259bb3cf88d1540b836a6aaa7622751b0287 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Wed, 9 Oct 2024 17:46:29 +0300
Subject: [PATCH 4/5] address style comments
---
.../Checkers/FuchsiaHandleChecker.cpp | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index eca8b888d67014..0bb19ca7a63e0c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -99,7 +99,6 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
#include "llvm/ADT/StringExtras.h"
#include <optional>
@@ -287,8 +286,8 @@ class FuchsiaHandleSymbolVisitor final : public SymbolVisitor {
/// Returns the symbols extracted from the argument or empty vector if it cannot
/// be found. It is unlikely to have over 1024 symbols in one argument.
-SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
- ProgramStateRef &State) {
+SmallVector<SymbolRef> getFuchsiaHandleSymbols(QualType QT, SVal Arg,
+ ProgramStateRef &State) {
int PtrToHandleLevel = 0;
while (QT->isAnyPointerType() || QT->isReferenceType()) {
++PtrToHandleLevel;
@@ -389,7 +388,7 @@ ProgramStateRef FuchsiaHandleChecker::evalArgsAttrs(const CallEvent &Call,
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
unsigned ParamDiagIdx = PVD->getFunctionScopeIndex() + 1;
- SmallVector<SymbolRef, 1024> Handles =
+ SmallVector<SymbolRef> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
for (SymbolRef Handle : Handles) {
@@ -536,7 +535,7 @@ void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
- SmallVector<SymbolRef, 1024> Handles =
+ SmallVector<SymbolRef> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
for (SymbolRef Handle : Handles) {
@@ -578,7 +577,7 @@ void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
- SmallVector<SymbolRef, 1024> Handles =
+ SmallVector<SymbolRef> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
for (SymbolRef Handle : Handles) {
@@ -682,7 +681,7 @@ ProgramStateRef FuchsiaHandleChecker::checkPointerEscape(
if (Arg >= FuncDecl->getNumParams())
break;
const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
- SmallVector<SymbolRef, 1024> Handles =
+ SmallVector<SymbolRef> Handles =
getFuchsiaHandleSymbols(PVD->getType(), Call->getArgSVal(Arg), State);
for (SymbolRef Handle : Handles) {
if (hasFuchsiaAttr<UseHandleAttr>(PVD) ||
>From d0f4ea2be0d267bbc4a659cda36494e8c684e49e Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Wed, 9 Oct 2024 18:45:53 +0300
Subject: [PATCH 5/5] fix build and runtime error
---
.../Checkers/FuchsiaHandleChecker.cpp | 33 +++++++++----------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index 0bb19ca7a63e0c..1daabe4a0fca4f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -493,24 +493,23 @@ bool FuchsiaHandleChecker::evalCall(const CallEvent &Call,
State = evalFunctionAttrs(Call, C, State, Notes);
State = evalArgsAttrs(Call, C, State, Notes);
- assert(!Notes.empty() && "Notes should be produced");
-
const NoteTag *T =
- C.getNoteTag([this, Notes{std::move(Notes)}](
- PathSensitiveBugReport &BR) -> std::string {
- if (&BR.getBugType() != &UseAfterReleaseBugType &&
- &BR.getBugType() != &LeakBugType &&
- &BR.getBugType() != &DoubleReleaseBugType &&
- &BR.getBugType() != &ReleaseUnownedBugType)
- return "";
- for (auto &Note : Notes) {
- std::string Text = Note(BR);
- if (!Text.empty())
- return Text;
- }
- return "";
- });
- }
+ Notes.empty()
+ ? nullptr
+ : C.getNoteTag([this, Notes{std::move(Notes)}](
+ PathSensitiveBugReport &BR) -> std::string {
+ if (&BR.getBugType() != &UseAfterReleaseBugType &&
+ &BR.getBugType() != &LeakBugType &&
+ &BR.getBugType() != &DoubleReleaseBugType &&
+ &BR.getBugType() != &ReleaseUnownedBugType)
+ return "";
+ for (auto &Note : Notes) {
+ std::string Text = Note(BR);
+ if (!Text.empty())
+ return Text;
+ }
+ return "";
+ });
C.addTransition(State, T);
return true;
More information about the cfe-commits
mailing list