[analyzer] Modernize FuchsiaHandleChecker #111588
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Pavel Skripkin (pskrgag) ChangesThis PR modernizes FuchsiaHandleChecker to fix real-world problem. Previous checker logic was modeling handles via To fix it, PR moves part of the modeling related to acquiring and releasing to Also, instead of old approach with hand-written notes, checker was moved to BugVisitor, which makes code more readable. The PR is almost NFC, but there 2 changes:
Patch is 23.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/111588.diff 2 Files Affected:
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.
...
[truncated]
|
|
CI again looks unrelated |
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
I read the patch and added my suggestions in inline comments, but I don't promise that I found every little corner case.
Moreover, I'm strongly opposed to introducing a BugReporterVisitor instead of directly creating the notes, because in this case you already know the locations where you want to add the notes (because that's where you update the HandleState), so it's completely pointless to delay the note creation to an awkward indirect post-procession step performed by a visitor. This way you wouldn't need to complicate the handle state by adding the (otherwise completely irrelevant) argument index in it.
In general, bug reporter visitors are complex tools that are good for complex situations where you want to find arbitrary complicated patterns in the bug report path, but should be avoided in simpler cases.
Admittedly, the old note creation code looks like an Obfuscated C Code Contest entry, but if you clear it up by introducing a few "natural" helper functions, you'll get an implementation that's significantly easier to understand than the "hide these numbers in the state, then later come back and reconstruct the situation" visitor.
|
|
||
| static HandleState getMaybeAllocated(SymbolRef ErrorSym) { | ||
| return HandleState(Kind::MaybeAllocated, ErrorSym); | ||
| static HandleState getMaybeAllocated(SymbolRef ErrorSym, std::uint8_t Idx) { |
There was a problem hiding this comment.
Why do you use std::uint8_t Idx here while Idx is unsigned elsewhere?
| C.addTransition(State); | ||
|
|
||
| return State != OldState; |
There was a problem hiding this comment.
If evalCall() returns false, it shouldn't do anything, in particular it shouldn't add a transition. I didn't analyze all the possible effects, but this mostly harmless transition might produce nasty bugs in some corner case.
| 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); |
There was a problem hiding this comment.
This logic doesn't bind a return value to the call expression in the case when the function declaration doesn't have a Fuchsia attribute.
If you have a function where some arguments have Fuchsia-specific attributes (so evalArgsAttrs and the invalidation activate), but the function declaration itself doesn't have an attribute, then you end up with a function call to which no return value was bound.
I vaguely recall that this would be equivalent to binding UnknownVal to the call, which is not a fatal issue, but it'd be better to ensure that we always bind a conjured value when the evaluation does something.
There was a problem hiding this comment.
Redone logic a bit. Now return value is always bound if checker models a call
Aha, Ok. I just saw it in various easy checkers like |
Yes, ValistChecker is another example where it's useless. By the way that's the first checker that I wrote (not completely alone) when I was an intern many years ago -- maybe when I'll have some free time I'll get rid of the visitor there. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
I'd love to better understand what is the root cause for these weird diagnostics when the functions are inlined just in case there is a less intrusive solution. If there is nothing else we could do, I am not opposed to evalCall, but I think it should be a last resort. |
The problem was in void close_outline(zx_handle_t handle ZX_RELEASE_HANDLE);
void close_inline(zx_handle_t handle ZX_RELEASE_HANDLE) // <- Modeled close here. handle state is now "Closed"
{
close_outline(handle); // <- Use after close report
}The same goes for I guess, this kind of reports could be suppressed by custom bug visitor, but I thougth |
Thanks for elaborating! I think the idea was to have annotations all the way down to syscalls, so if we end up inlining the body with the syscalls we still get reasonable behavior. Is the problem that some calls cannot be annotated or is the problem that the inlined bodies often have coding patterns that are to complicated for the analyzer to understand? |
It is definitely more clean locally. My concerns is about the global behavior. Imagine a function that deals with both Fuchsia handles and plain old C file handles. If we start to do evalCall and no longer inline the body, we no longer get the modeling for This is the main problem with evalCall, it does not really compose. |
I get it, but it seems very unlikely... Anyway, I don't have strong opinion, I just want to fix annoying false positives with ctu =) |
Thanks for working on this, I really appreciate it! :) I would even say this is not related to ctu, because users could run into this behavior within a single translation unit. I just want to make sure we exhausted other options before considering something that might not compose.
I usually find it hard to predict how a certain codebase would evolve. In case someone wants to generalize this checker to non-fuchsia handles (and now suddenly we evalcall way more functions), or fuchsia starts to add support for mixing handles (e.g., converting from a C handle to a fuchsia one), and suddenly this becomes a likely scenario that users could hit. To be clear, I also don't expect this to happen, at least not any time soon, but I don't want to avoid picking a solution that is not extensible in certain directions unless we absolutely have to. |
Oh, sorry I missed that comment. The problem in our code is that function that releases a handle calls a syscall via macro, so it's not possible to directly annotate it. That's why analyzer just thinks that handle leaks, since there is no point where handle gets released after function inline. I guess, it could be solved on my side if I drop library from ctu analisys, but it helps a lot with uninit bugs. |
Ah, I see! Thanks for sharing! I think there are a couple potential workarounds here if you cannot change the macros:
I think in this particular case maybe the APINotes solution would make the most sense. See https://clang.llvm.org/docs/APINotes.html for details. |
I guess, this may confuse checker more, since not all syscalls actually consume handles. I will try and share my results =) Thanks! |
|
I might misunderstand something, but seems like APINotes mechanism does not work for handles. At least as-is. The problem is that |
|
I think APINotes support attributes on param declarations. That being said, I think APINotes might need an extension to explicitly add support for handle attributes. But I think that should be a straightforward patch. |
Hm, cannot find it, but anyway will check if it's possible to extend APINotes. Thanks! |
4 participants
This PR modernizes FuchsiaHandleChecker to fix real-world problem. Previous checker logic was modeling handles via
checkPreCallandcheckPostCall, which doesn't work well for CTU, since function body gets inlined and CSA produces odd reports like: https://godbolt.org/z/xWbEMhc9PTo fix it, PR moves part of the modeling related to acquiring and releasing to
evalCall.Also note generation logic was redone to make it more readable.
The PR is almost NFC, but
check{Pre, Post}Call->evalCallis not an NFC, since it changes behavior regarding modeling functions with visible body