[analyzer] Display a diagnostics when an inlined function violates its os_consumed summary

[george.karpenkov]

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

[NoQ]

Looks great!

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

118 ↗

(On Diff #177605)

Why did they return void but make an out-parameter T_T

232 ↗

(On Diff #177605)

Typo intended?

235–238 ↗

(On Diff #177605)

Slightly shorter:

if (SC->inTopFrame())
  return;

const StackFrameContext *PC = SC->getParent()->getStackFrame();

267–273 ↗

(On Diff #177605)

A tiny bit shorter:

const ParmVarDecl *PVD = Parameters[I];
if (!PVD->hasAttr<OSConsumedAttr>())
  return nullptr;

if (SymbolRef SR = Call->getArgSVal(I).getAsLocSymbol()) {

clang/test/Analysis/osobject-retain-release.cpp

118 ↗

(On Diff #177605)

// no-warning?

[george.karpenkov]

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

118 ↗

(On Diff #177605)

this is a formatting change.

nevertheless, which one? os has a bigger lifetime than this function.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

118 ↗

(On Diff #177605)

I meant Sym.

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

case RefVal::ReturnedNotOwned:
       return true;
 
     default:
       return false;
     }
   return true;
 }
 
-static void generateDiagnosticsForCallLike(
-  ProgramStateRef CurrSt,
+static void generateDiagnosticsForCallLike(ProgramStateRef CurrSt,
                                            const LocationContext *LCtx,
-  const RefVal &CurrV,
-  SymbolRef &Sym,
+                                           const RefVal &CurrV, SymbolRef &Sym,
                                            const Stmt *S,
                                            llvm::raw_string_ostream &os) {
   if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
     // Get the name of the callee (if it is available)
     // from the tracked SVal.
     SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx);
     const FunctionDecl *FD = X.getAsFunctionDecl();
 
     // If failed, try to get it from AST.
     if (!FD)
       FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl());
 
     if (const auto *MD = dyn_cast<CXXMethodDecl>(CE->getCalleeDecl())) {
       os << "Call to method '" << MD->getQualifiedNameAsString() << '\'';
     } else if (FD) {
       os << "Call to function '" << FD->getQualifiedNameAsString() << '\'';
     } else {
       os << "function call";
     }
-  } else if (isa<CXXNewExpr>(S)){
-    os << "Operator new";
+  } else if (isa<CXXNewExpr>(S)) {
+    os << "Operator 'new'";
   } else {
     assert(isa<ObjCMessageExpr>(S));
     CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager();
     CallEventRef<ObjCMethodCall> Call =
         Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx);
 
     switch (Call->getMessageKind()) {
     case OCM_Message:
std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
                                                   const ExplodedNode *N,
                                                   BugReport &BR) override;
 };
 
 } // end namespace retaincountchecker
 } // 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<PathDiagnosticEventPiece>
+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<const ParmVarDecl *> Parameters = Call->parameters();
+  for (unsigned I=0; I < Call->getNumArgs() && I < Parameters.size(); ++I) {
+    const ParmVarDecl *PVD = Parameters[I];
+
+    if (!PVD->hasAttr<OSConsumedAttr>())
+      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<PathDiagnosticEventPiece>(L, os.str());
+}
+
 std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
+  const SourceManager &SM = BRC.getSourceManager();
+  CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
+  if (auto CE = N->getLocationAs<CallExitBegin>()) {
+    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<StmtPoint>())
     return nullptr;
 
   // Check if the type state has changed.
   const ExplodedNode *PrevNode = N->getFirstPred();
   ProgramStateRef PrevSt = PrevNode->getState();
if (isa<ObjCArrayLiteral>(S)) {
         os << "oxed expression produces an object with a +0 retain count";
       }
     } else if (isa<ObjCIvarRefExpr>(S)) {
       os << "Object loaded from instance variable";
     } else {
       generateDiagnosticsForCallLike(CurrSt, LCtx, CurrV, Sym, S, os);
     }
 
-    PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
-                                  N->getLocationContext());
+    PathDiagnosticLocation Pos(S, SM, N->getLocationContext());
     return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str());
   }
 
   // Gather up the effects that were performed on the object at this
   // program point
   SmallVector<ArgEffect, 2> 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
     // ObjCMessageExprs.
     const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();
 
     if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
       // Iterate through the parameter expressions and see if the symbol

cfe/trunk/test/Analysis/osobject-retain-release.cpp

struct OtherStruct {
   OtherStruct(OSArray *arr);
 };
 
 struct OSMetaClassBase {
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
 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;
   escape(obj);
 }
 
 void test_no_infinite_check_recursion(MyArray *arr) {
   OSObject *input = new OSObject;
void check_free_use_after_free() {
   OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}}
   arr->retain(); // expected-note{{Reference count incremented. The object now has a +2 retain count}}
   arr->free(); // expected-note{{Object released}}
   arr->retain(); // expected-warning{{Reference-counted object is used after it is released}}
                  // expected-note@-1{{Reference-counted object is used after it is released}}
 }
 
 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'}}
 }
 
 unsigned int check_leak_factory() {
   OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' 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'}}