r345338 - [analyzer] Correct modelling of OSDynamicCast: eagerly state split
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 25 16:38:07 PDT 2018
Author: george.karpenkov
Date: Thu Oct 25 16:38:07 2018
New Revision: 345338
URL: http://llvm.org/viewvc/llvm-project?rev=345338&view=rev
Log:
[analyzer] Correct modelling of OSDynamicCast: eagerly state split
Previously, OSDynamicCast was modeled as an identity.
This is not correct: the output of OSDynamicCast may be zero even if the
input was not zero (if the class is not of desired type), and thus the
modeling led to false positives.
Instead, we are doing eager state split:
in one branch, the returned value is identical to the input parameter,
and in the other branch, the returned value is zero.
This patch required a substantial refactoring of canEval infrastructure,
as now it can return different function summaries, and not just true/false.
rdar://45497400
Differential Revision: https://reviews.llvm.org/D53624
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
cfe/trunk/test/Analysis/osobject-retain-release.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h?rev=345338&r1=345337&r2=345338&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h Thu Oct 25 16:38:07 2018
@@ -636,9 +636,19 @@ public:
InitializeMethodSummaries();
}
- bool canEval(const CallExpr *CE,
- const FunctionDecl *FD,
- bool &hasTrustedImplementationAnnotation);
+ enum class BehaviorSummary {
+ // Function does not return.
+ NoOp,
+
+ // Function returns the first argument.
+ Identity,
+
+ // Function either returns zero, or the input parameter.
+ IdentityOrZero
+ };
+
+ Optional<BehaviorSummary> canEval(const CallExpr *CE, const FunctionDecl *FD,
+ bool &hasTrustedImplementationAnnotation);
bool isTrustedReferenceCountImplementation(const FunctionDecl *FD);
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp?rev=345338&r1=345337&r2=345338&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp Thu Oct 25 16:38:07 2018
@@ -774,14 +774,17 @@ bool RetainCountChecker::evalCall(const
const LocationContext *LCtx = C.getLocationContext();
+ using BehaviorSummary = RetainSummaryManager::BehaviorSummary;
+ Optional<BehaviorSummary> BSmr =
+ SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation);
+
// See if it's one of the specific functions we know how to eval.
- if (!SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation))
+ if (!BSmr)
return false;
// Bind the return value.
- // For now, all the functions which we can evaluate and which take
- // at least one argument are identities.
- if (CE->getNumArgs() >= 1) {
+ if (BSmr == BehaviorSummary::Identity ||
+ BSmr == BehaviorSummary::IdentityOrZero) {
SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
// If the receiver is unknown or the function has
@@ -793,7 +796,24 @@ bool RetainCountChecker::evalCall(const
RetVal =
SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
}
- state = state->BindExpr(CE, LCtx, RetVal, false);
+ state = state->BindExpr(CE, LCtx, RetVal, /*Invalidate=*/false);
+
+ if (BSmr == BehaviorSummary::IdentityOrZero) {
+ // Add a branch where the output is zero.
+ ProgramStateRef NullOutputState = C.getState();
+
+ // Assume that output is zero on the other branch.
+ NullOutputState = NullOutputState->BindExpr(
+ CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false);
+
+ C.addTransition(NullOutputState);
+
+ // And on the original branch assume that both input and
+ // output are non-zero.
+ if (auto L = RetVal.getAs<DefinedOrUnknownSVal>())
+ state = state->assume(*L, /*Assumption=*/true);
+
+ }
}
C.addTransition(state);
Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=345338&r1=345337&r2=345338&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Thu Oct 25 16:38:07 2018
@@ -65,6 +65,10 @@ static bool isOSObjectSubclass(const Dec
return isSubclass(D, "OSObject");
}
+static bool isOSObjectDynamicCast(StringRef S) {
+ return S == "safeMetaCast";
+}
+
static bool isOSIteratorSubclass(const Decl *D) {
return isSubclass(D, "OSIterator");
}
@@ -231,6 +235,9 @@ RetainSummaryManager::generateSummary(co
if (TrackOSObjects && PD && isOSObjectSubclass(PD)) {
if (const IdentifierInfo *II = FD->getIdentifier()) {
+ if (isOSObjectDynamicCast(II->getName()))
+ return getDefaultSummary();
+
// All objects returned with functions starting with "get" are getters.
if (II->getName().startswith("get")) {
@@ -515,20 +522,21 @@ bool RetainSummaryManager::isTrustedRefe
return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
}
-bool RetainSummaryManager::canEval(const CallExpr *CE,
- const FunctionDecl *FD,
- bool &hasTrustedImplementationAnnotation) {
+Optional<RetainSummaryManager::BehaviorSummary>
+RetainSummaryManager::canEval(const CallExpr *CE, const FunctionDecl *FD,
+ bool &hasTrustedImplementationAnnotation) {
IdentifierInfo *II = FD->getIdentifier();
if (!II)
- return false;
+ return None;
StringRef FName = II->getName();
FName = FName.substr(FName.find_first_not_of('_'));
QualType ResultTy = CE->getCallReturnType(Ctx);
if (ResultTy->isObjCIdType()) {
- return II->isStr("NSMakeCollectable");
+ if (II->isStr("NSMakeCollectable"))
+ return BehaviorSummary::Identity;
} else if (ResultTy->isPointerType()) {
// Handle: (CF|CG|CV)Retain
// CFAutorelease
@@ -536,31 +544,34 @@ bool RetainSummaryManager::canEval(const
if (cocoa::isRefType(ResultTy, "CF", FName) ||
cocoa::isRefType(ResultTy, "CG", FName) ||
cocoa::isRefType(ResultTy, "CV", FName))
- return isRetain(FD, FName) || isAutorelease(FD, FName) ||
- isMakeCollectable(FName);
-
- // Process OSDynamicCast: should just return the first argument.
- // For now, treating the cast as a no-op, and disregarding the case where
- // the output becomes null due to the type mismatch.
- if (TrackOSObjects && FName == "safeMetaCast") {
- return true;
+ if (isRetain(FD, FName) || isAutorelease(FD, FName) ||
+ isMakeCollectable(FName))
+ return BehaviorSummary::Identity;
+
+ // safeMetaCast is called by OSDynamicCast.
+ // We assume that OSDynamicCast is either an identity (cast is OK,
+ // the input was non-zero),
+ // or that it returns zero (when the cast failed, or the input
+ // was zero).
+ if (TrackOSObjects && isOSObjectDynamicCast(FName)) {
+ return BehaviorSummary::IdentityOrZero;
}
const FunctionDecl* FDD = FD->getDefinition();
if (FDD && isTrustedReferenceCountImplementation(FDD)) {
hasTrustedImplementationAnnotation = true;
- return true;
+ return BehaviorSummary::Identity;
}
}
if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
const CXXRecordDecl *Parent = MD->getParent();
if (TrackOSObjects && Parent && isOSObjectSubclass(Parent))
- return FName == "release" || FName == "retain";
+ if (FName == "release" || FName == "retain")
+ return BehaviorSummary::NoOp;
}
- return false;
-
+ return None;
}
const RetainSummary *
Modified: cfe/trunk/test/Analysis/osobject-retain-release.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/osobject-retain-release.cpp?rev=345338&r1=345337&r2=345338&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/osobject-retain-release.cpp (original)
+++ cfe/trunk/test/Analysis/osobject-retain-release.cpp Thu Oct 25 16:38:07 2018
@@ -17,6 +17,8 @@ struct OSObject {
virtual void release() {};
virtual ~OSObject(){}
+ unsigned int foo() { return 42; }
+
static OSObject *generateObject(int);
static const OSMetaClass * const metaClass;
@@ -78,8 +80,31 @@ void check_dynamic_cast() {
arr->release();
}
+unsigned int check_dynamic_cast_no_null_on_orig(OSObject *obj) {
+ OSArray *arr = OSDynamicCast(OSArray, obj);
+ if (arr) {
+ return arr->getCount();
+ } else {
+
+ // The fact that dynamic cast has failed should not imply that
+ // the input object was null.
+ return obj->foo(); // no-warning
+ }
+}
+
+void check_dynamic_cast_null_branch(OSObject *obj) {
+ OSArray *arr1 = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject}}
+ OSArray *arr = OSDynamicCast(OSArray, obj);
+ if (!arr) // expected-note{{Taking true branch}}
+ return; // expected-warning{{Potential leak}}
+ // expected-note at -1{{Object leaked}}
+ arr1->release();
+}
+
void check_dynamic_cast_null_check() {
- OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
+ OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1)); // expected-note{{Call to function 'generateObject' returns an OSObject}}
+ // expected-warning at -1{{Potential leak of an object}}
+ // expected-note at -2{{Object leaked}}
if (!arr)
return;
arr->release();
More information about the cfe-commits
mailing list