NoQ updated this revision to Diff 187873.
NoQ marked an inline comment as done.
NoQ added a comment.
Address comments.
@Charusso: I agreed not to rush for D58367 <https://reviews.llvm.org/D58367>
and implemented an old-style visitor here instead :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58368/new/
https://reviews.llvm.org/D58368
Files:
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
clang/test/Analysis/mig.mm
Index: clang/test/Analysis/mig.mm
===================================================================
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,5 +1,5 @@
// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN: -fblocks -verify %s
+// RUN: -analyzer-output=text -fblocks -verify %s
// XNU APIs.
@@ -20,9 +20,11 @@
MIG_SERVER_ROUTINE
kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
- vm_deallocate(port, address, size);
- if (size > 10) {
+ vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+ if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+ // expected-note@-1{{Taking true branch}}
return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
}
return KERN_SUCCESS;
}
@@ -42,6 +44,18 @@
vm_deallocate(port, address, size);
}
+// When releasing two parameters, add a note for both of them.
+// Also when returning a variable, explain why do we think that it contains
+// a non-success code.
+MIG_SERVER_ROUTINE
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_address_t addr2, vm_size_t size) {
+ kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+ vm_deallocate(port, addr1, size); // expected-note{{Value passed through parameter 'addr1' is deallocated}}
+ vm_deallocate(port, addr2, size); // expected-note{{Value passed through parameter 'addr2' is deallocated}}
+ return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+}
+
// Check that we work on Objective-C messages and blocks.
@interface I
- (kern_return_t)fooAtPort:(mach_port_name_t)port withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
@@ -51,8 +65,9 @@
- (kern_return_t)fooAtPort:(mach_port_name_t)port
withAddress:(vm_address_t)address
ofSize:(vm_size_t)size MIG_SERVER_ROUTINE {
- vm_deallocate(port, address, size);
+ vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
}
@end
@@ -60,8 +75,9 @@
kern_return_t (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
^MIG_SERVER_ROUTINE (mach_port_name_t port,
vm_address_t address, vm_size_t size) {
- vm_deallocate(port, address, size);
+ vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
};
}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -41,19 +41,56 @@
public:
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+
+ class Visitor : public BugReporterVisitor {
+ public:
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ static int X = 0;
+ ID.AddPointer(&X);
+ }
+
+ std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC, BugReport &R);
+ };
};
} // end anonymous namespace
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
+// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
+// specialization for this sort of types.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
+
+std::shared_ptr<PathDiagnosticPiece>
+MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
+ BugReport &R) {
+ const auto *NewPVD = static_cast<const ParmVarDecl *>(
+ N->getState()->get<ReleasedParameter>());
+ const auto *OldPVD = static_cast<const ParmVarDecl *>(
+ N->getFirstPred()->getState()->get<ReleasedParameter>());
+ if (OldPVD == NewPVD)
+ return nullptr;
+
+ assert(NewPVD && "What is deallocated cannot be un-deallocated!");
+ SmallString<64> Str;
+ llvm::raw_svector_ostream OS(Str);
+ OS << "Value passed through parameter '" << NewPVD->getName()
+ << "' is deallocated";
+
+ PathDiagnosticLocation Loc =
+ PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
+ return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
+}
-static bool isCurrentArgSVal(SVal V, CheckerContext &C) {
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
SymbolRef Sym = V.getAsSymbol();
if (!Sym)
- return false;
+ return nullptr;
const auto *VR = dyn_cast_or_null<VarRegion>(Sym->getOriginRegion());
- return VR && VR->hasStackParametersStorage() &&
- VR->getStackFrame()->inTopFrame();
+ if (VR && VR->hasStackParametersStorage() &&
+ VR->getStackFrame()->inTopFrame())
+ return cast<ParmVarDecl>(VR->getDecl());
+
+ return nullptr;
}
static bool isInMIGCall(CheckerContext &C) {
@@ -96,8 +133,11 @@
// TODO: Unhardcode "1".
SVal Arg = Call.getArgSVal(1);
- if (isCurrentArgSVal(Arg, C))
- C.addTransition(C.getState()->set<ReleasedParameter>(true));
+ const ParmVarDecl *PVD = getOriginParam(Arg, C);
+ if (!PVD)
+ return;
+
+ C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
}
void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
@@ -139,6 +179,9 @@
"deallocate it again",
N);
+ R->addRange(RS->getSourceRange());
+ bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+ R->addVisitor(llvm::make_unique<Visitor>());
C.emitReport(std::move(R));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
