From: George Karpenkov Date: Tue, 11 Dec 2018 01:13:40 +0000 (+0000) Subject: [analyzer] Display a diagnostics when an inlined function violates its os_consumed... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=48f04c6368e202cd7f550a24e92fa129bbf72f8c;p=clang [analyzer] Display a diagnostics when an inlined function violates its os_consumed summary This is currently a diagnostics, but might be upgraded to an error in the future, especially if we introduce os_return_on_success attributes. rdar://46359592 Differential Revision: https://reviews.llvm.org/D55530 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@348820 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp index d2dbcbdcd3..9dff0be138 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -113,13 +113,11 @@ static bool shouldGenerateNote(llvm::raw_string_ostream &os, return true; } -static void generateDiagnosticsForCallLike( - ProgramStateRef CurrSt, - const LocationContext *LCtx, - const RefVal &CurrV, - SymbolRef &Sym, - const Stmt *S, - llvm::raw_string_ostream &os) { +static void generateDiagnosticsForCallLike(ProgramStateRef CurrSt, + const LocationContext *LCtx, + const RefVal &CurrV, SymbolRef &Sym, + const Stmt *S, + llvm::raw_string_ostream &os) { if (const CallExpr *CE = dyn_cast(S)) { // Get the name of the callee (if it is available) // from the tracked SVal. @@ -137,8 +135,8 @@ static void generateDiagnosticsForCallLike( } else { os << "function call"; } - } else if (isa(S)){ - os << "Operator new"; + } else if (isa(S)) { + os << "Operator 'new'"; } else { assert(isa(S)); CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager(); @@ -230,9 +228,94 @@ public: } // end namespace ento } // end namespace clang + +/// Find the first node with the parent stack frame. +static const ExplodedNode *getCalleeNode(const ExplodedNode *Pred) { + const StackFrameContext *SC = Pred->getStackFrame(); + if (SC->inTopFrame()) + return nullptr; + const StackFrameContext *PC = SC->getParent()->getStackFrame(); + if (!PC) + return nullptr; + + const ExplodedNode *N = Pred; + while (N && N->getStackFrame() != PC) { + N = N->getFirstPred(); + } + return N; +} + + +/// Insert a diagnostic piece at function exit +/// if a function parameter is annotated as "os_consumed", +/// but it does not actually consume the reference. +static std::shared_ptr +annotateConsumedSummaryMismatch(const ExplodedNode *N, + CallExitBegin &CallExitLoc, + const SourceManager &SM, + CallEventManager &CEMgr) { + + const ExplodedNode *CN = getCalleeNode(N); + if (!CN) + return nullptr; + + CallEventRef<> Call = CEMgr.getCaller(N->getStackFrame(), N->getState()); + + std::string sbuf; + llvm::raw_string_ostream os(sbuf); + ArrayRef Parameters = Call->parameters(); + for (unsigned I=0; I < Call->getNumArgs() && I < Parameters.size(); ++I) { + const ParmVarDecl *PVD = Parameters[I]; + + if (!PVD->hasAttr()) + return nullptr; + + if (SymbolRef SR = Call->getArgSVal(I).getAsLocSymbol()) { + const RefVal *CountBeforeCall = getRefBinding(CN->getState(), SR); + const RefVal *CountAtExit = getRefBinding(N->getState(), SR); + + if (!CountBeforeCall || !CountAtExit) + continue; + + unsigned CountBefore = CountBeforeCall->getCount(); + unsigned CountAfter = CountAtExit->getCount(); + + bool AsExpected = CountBefore > 0 && CountAfter == CountBefore - 1; + if (!AsExpected) { + os << "Parameter '"; + PVD->getNameForDiagnostic(os, PVD->getASTContext().getPrintingPolicy(), + /*Qualified=*/false); + os << "' is marked as consuming, but the function does not consume " + << "the reference\n"; + } + } + } + + if (os.str().empty()) + return nullptr; + + // FIXME: remove the code duplication with NoStoreFuncVisitor. + PathDiagnosticLocation L; + if (const ReturnStmt *RS = CallExitLoc.getReturnStmt()) { + L = PathDiagnosticLocation::createBegin(RS, SM, N->getLocationContext()); + } else { + L = PathDiagnosticLocation( + Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(), SM); + } + + return std::make_shared(L, os.str()); +} + std::shared_ptr CFRefReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { + const SourceManager &SM = BRC.getSourceManager(); + CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager(); + if (auto CE = N->getLocationAs()) { + if (auto PD = annotateConsumedSummaryMismatch(N, *CE, SM, CEMgr)) + return PD; + } + // FIXME: We will eventually need to handle non-statement-based events // (__attribute__((cleanup))). if (!N->getLocation().getAs()) @@ -293,15 +376,13 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, generateDiagnosticsForCallLike(CurrSt, LCtx, CurrV, Sym, S, os); } - PathDiagnosticLocation Pos(S, BRC.getSourceManager(), - N->getLocationContext()); + PathDiagnosticLocation Pos(S, SM, N->getLocationContext()); return std::make_shared(Pos, os.str()); } // Gather up the effects that were performed on the object at this // program point SmallVector AEffects; - const ExplodedNode *OrigNode = BRC.getNodeResolver().getOriginalNode(N); if (const RetainSummary *Summ = SummaryLog.lookup(OrigNode)) { // We only have summaries attached to nodes after evaluating CallExpr and diff --git a/test/Analysis/osobject-retain-release.cpp b/test/Analysis/osobject-retain-release.cpp index b7a299da91..b8eb462d20 100644 --- a/test/Analysis/osobject-retain-release.cpp +++ b/test/Analysis/osobject-retain-release.cpp @@ -90,6 +90,32 @@ struct OSMetaClassBase { }; void escape(void *); +bool coin(); + +bool os_consume_violation(OS_CONSUME OSObject *obj) { + if (coin()) { // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + escape(obj); + return true; + } + return false; // expected-note{{Parameter 'obj' is marked as consuming, but the function does not consume the reference}} +} + +void os_consume_ok(OS_CONSUME OSObject *obj) { + escape(obj); +} + +void use_os_consume_violation() { + OSObject *obj = new OSObject; // expected-note{{Operator 'new' returns an OSObject of type OSObject with a +1 retain count}} + os_consume_violation(obj); // expected-note{{Calling 'os_consume_violation'}} + // expected-note@-1{{Returning from 'os_consume_violation'}} +} // expected-note{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} + // expected-warning@-1{{Potential leak of an object stored into 'obj'}} + +void use_os_consume_ok() { + OSObject *obj = new OSObject; + os_consume_ok(obj); +} void test_escaping_into_voidstar() { OSObject *obj = new OSObject; @@ -152,7 +178,7 @@ void check_free_use_after_free() { } unsigned int check_leak_explicit_new() { - OSArray *arr = new OSArray; // expected-note{{Operator new returns an OSObject of type OSArray with a +1 retain count}} + OSArray *arr = new OSArray; // expected-note{{Operator 'new' returns an OSObject of type OSArray with a +1 retain count}} return arr->getCount(); // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}} // expected-warning@-1{{Potential leak of an object stored into 'arr'}} }