[analyzer] Support for OSObjects out parameters in RetainCountChecker

[george.karpenkov]

Moved logic into the summary manager.

[george.karpenkov]

Moved attributes into a separate revision.

[george.karpenkov]

Support custom diagnostics.

[NoQ]

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

535

Mmm, what does L stand for here?

541–545

std::any_of()?

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

842

Typo: "because".

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

114

Should we somehow indicate that we assumed that the function returns a success value? I.e., bug visitor / tag.

Also what is the intended behavior if we replace write_into_out_param_on_success with write_into_out_param_on_failure here?

114

That is, i really want this test added, and then it's good to commit, other nits are minor.

[george.karpenkov]

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

535

vaLue.
Other letters are usually taken =/ Is there a different convention ?

541–545

That's super verbose with iterators and requires putting those values in the array.
Unfortunately, libc++ does not have a

<template T, template Others...>
bool isAnyOf(T Val, Others... OtherVals)

function

[NoQ]

Yay thx.

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

535

L usually stands for Loc; V is more common for raw SVals.

clang/include/clang/Basic/Attr.td

 def OSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"os_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSReturnsRetained : InheritableAttr {
   let Spellings = [Clang<"os_returns_retained">];
-  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
+  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"os_returns_not_retained">];
-  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
+  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSConsumesThis : InheritableAttr {
   let Spellings = [Clang<"os_consumes_this">];
   let Subjects = SubjectList<[NonStaticCXXMethod]>;
   let Documentation = [RetainBehaviorDocs];
 }

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

void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ,
   if (SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol()) {
     if (RE.getKind() == RetEffect::NoRetHard)
       state = removeRefBinding(state, Sym);
   }
 
   C.addTransition(state);
 }
 
-static ProgramStateRef updateOutParameter(ProgramStateRef State,
-                                          SVal ArgVal,
-                                          ArgEffectKind Effect) {
+/// \return Whether the chain of typedefs starting from {@code QT}
+/// has a typedef with a given name {@code Name}.
+static bool hasTypedefNamed(QualType QT,
+                            StringRef Name) {
+  while (auto *T = dyn_cast<TypedefType>(QT)) {
+    const auto &Context = T->getDecl()->getASTContext();
+    if (T->getDecl()->getIdentifier() == &Context.Idents.get(Name))
+      return true;
+    QT = T->getDecl()->getUnderlyingType();
+  }
+  return false;
+}
+
+static SmallVector<ProgramStateRef, 2>
+updateOutParameter(ProgramStateRef State,
+                   const RetainSummary &Summ,
+                   const CallEvent &CE) {
+  SVal L = CE.getReturnValue();
+
+  // Splitting is required to support out parameters,
+  // as out parameters might be created only on the "success" branch.
+  // We want to avoid eagerly splitting unless out parameters are actually
+  // needed.
+  // So we do calculations on three states: original and two split ones,
+  // and if no split was necessary return the original one,
+  // and return the split ones otherwise.
+  bool SplitNecessary = false;
+
+  ProgramStateRef AssumeTrue = State;
+  ProgramStateRef AssumeFalse = State;
+  QualType QT = CE.getResultType();
+  bool ShouldSplit = !QT.isNull() && !QT->isVoidType();
+
+  // The usual convention is for non-zero to mean success, but in the kernel
+  // it's reverted if the typedef chain has a typedef kern_return_t.
+  bool SuccessInverted = hasTypedefNamed(QT, "kern_return_t");
+  if (!QT.isNull()) {
+    if (auto DL = L.getAs<DefinedOrUnknownSVal>()) {
+      AssumeTrue = AssumeTrue->assume(*DL, true);
+      AssumeFalse = AssumeFalse->assume(*DL, false);
+    }
+  }
+
+  for (unsigned idx = 0, e = CE.getNumArgs(); idx != e; ++idx) {
+    SVal ArgVal = CE.getArgSVal(idx);
+    ArgEffect AE = Summ.getArg(idx);
+
     auto *ArgRegion = dyn_cast_or_null<TypedValueRegion>(ArgVal.getAsRegion());
     if (!ArgRegion)
-    return State;
+      continue;
 
     QualType PointeeTy = ArgRegion->getValueType();
-  if (!coreFoundation::isCFObjectRef(PointeeTy))
-    return State;
-
     SVal PointeeVal = State->getSVal(ArgRegion);
     SymbolRef Pointee = PointeeVal.getAsLocSymbol();
     if (!Pointee)
-    return State;
+      continue;
 
-  switch (Effect) {
-  case UnretainedOutParameter:
-    State = setRefBinding(State, Pointee,
-                          RefVal::makeNotOwned(ObjKind::CF, PointeeTy));
+    switch (AE.getKind()) {
+    case UnretainedOutParameter: {
+      auto updateOutParam = [&](ProgramStateRef St) {
+        return setRefBinding(St, Pointee,
+                             RefVal::makeNotOwned(AE.getObjKind(), PointeeTy));
+      };
+      State = updateOutParam(State);
+      AssumeTrue = updateOutParam(AssumeTrue);
+      AssumeFalse = updateOutParam(AssumeFalse);
       break;
-  case RetainedOutParameter:
+    }
+    case RetainedOutParameter: {
+      if (AE.getObjKind() == ObjKind::OS) {
+        auto updateOutParam = [&](ProgramStateRef St) {
+          return setRefBinding(St, Pointee,
+                               RefVal::makeOwned(ObjKind::OS, PointeeTy));
+        };
+        if (!ShouldSplit) {
+          State = updateOutParam(State);
+        } else if (!SuccessInverted) {
+          SplitNecessary = true;
+          AssumeTrue = updateOutParam(AssumeTrue);
+        } else {
+          SplitNecessary = true;
+          AssumeFalse = updateOutParam(AssumeFalse);
+        }
+      }
+
+      // For others:
       // Do nothing. Retained out parameters will either point to a +1 reference
-    // or NULL, but the way you check for failure differs depending on the API.
-    // Consequently, we don't have a good way to track them yet.
+      // or NULL, but the way you check for failure differs depending on the
+      // API. Consequently, we don't have a good way to track them yet.
       break;
-
+    }
     default:
-    llvm_unreachable("only for out parameters");
+      llvm_unreachable("Only for out parameters");
+    }
   }
 
-  return State;
+  if (SplitNecessary) {
+    assert(ShouldSplit);
+    return {AssumeTrue, AssumeFalse};
+  } else {
+    return {State};
+  }
 }
 
 void RetainCountChecker::checkSummary(const RetainSummary &Summ,
                                       const CallEvent &CallOrMsg,
                                       CheckerContext &C) const {
   ProgramStateRef state = C.getState();
 
   // Evaluate the effect of the arguments.
   RefVal::Kind hasErr = (RefVal::Kind) 0;
   SourceRange ErrorRange;
   SymbolRef ErrorSym = nullptr;
 
   // Helper tag for providing diagnostics: indicate whether dealloc was sent
   // at this location.
   static CheckerProgramPointTag DeallocSentTag(this, DeallocTagDescription);
   bool DeallocSent = false;
 
   for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) {
     SVal V = CallOrMsg.getArgSVal(idx);
 
     ArgEffect Effect = Summ.getArg(idx);
-    if (Effect.getKind() == RetainedOutParameter ||
-        Effect.getKind() == UnretainedOutParameter) {
-      state = updateOutParameter(state, V, Effect.getKind());
-    } else if (SymbolRef Sym = V.getAsLocSymbol()) {
+    if (SymbolRef Sym = V.getAsLocSymbol()) {
       if (const RefVal *T = getRefBinding(state, Sym)) {
 
         if (shouldEscapeOSArgumentOnCall(CallOrMsg, idx, T))
           Effect = ArgEffect(StopTrackingHard, ObjKind::OS);
 
         state = updateSymbol(state, Sym, *T, Effect, hasErr, C);
         if (hasErr) {
           ErrorRange = CallOrMsg.getArgSourceRange(idx);
if (RE.notOwned()) {
       const Expr *Ex = CallOrMsg.getOriginExpr();
       assert(Ex);
       ResultTy = GetReturnType(Ex, C.getASTContext());
     }
     if (Optional<RefVal> updatedRefVal = refValFromRetEffect(RE, ResultTy))
       state = setRefBinding(state, Sym, *updatedRefVal);
   }
 
+  SmallVector<ProgramStateRef, 2> Out =
+      updateOutParameter(state, Summ, CallOrMsg);
+
+  for (ProgramStateRef St : Out) {
     if (DeallocSent) {
-    C.addTransition(state, C.getPredecessor(), &DeallocSentTag);
+      C.addTransition(St, C.getPredecessor(), &DeallocSentTag);
     } else {
-    C.addTransition(state);
+      C.addTransition(St);
+    }
   }
 }
 
 ProgramStateRef RetainCountChecker::updateSymbol(ProgramStateRef state,
                                                  SymbolRef sym, RefVal V,
                                                  ArgEffect AE,
                                                  RefVal::Kind &hasErr,
                                                  CheckerContext &C) const {

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

bool RetainSummaryManager::applyFunctionParamAnnotationEffect(
   QualType QT = pd->getType();
   if (auto K =
           hasAnyEnabledAttrOf<NSConsumedAttr, CFConsumedAttr, OSConsumedAttr,
                               GeneralizedConsumedAttr>(pd, QT)) {
     Template->addArg(AF, parm_idx, ArgEffect(DecRef, *K));
     return true;
   } else if (auto K =
                  hasAnyEnabledAttrOf<CFReturnsRetainedAttr,
+                                     OSReturnsRetainedAttr,
                                      GeneralizedReturnsRetainedAttr>(pd, QT)) {
     Template->addArg(AF, parm_idx, ArgEffect(RetainedOutParameter, *K));
     return true;
-  } else if (auto K = hasAnyEnabledAttrOf<CFReturnsNotRetainedAttr>(pd, QT)) {
+  } else if (auto K = hasAnyEnabledAttrOf<CFReturnsNotRetainedAttr,
+                                          OSReturnsNotRetainedAttr,
+                                          GeneralizedReturnsNotRetainedAttr>(pd, QT)) {
     Template->addArg(AF, parm_idx, ArgEffect(UnretainedOutParameter, *K));
     return true;
   } else {
     if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
       for (const auto *OD : MD->overridden_methods()) {
         const ParmVarDecl *OP = OD->parameters()[parm_idx];
         if (applyFunctionParamAnnotationEffect(OP, parm_idx, OD, Template))
           return true;
       }
     }
   }
 
   return false;
 }
 
 void
 RetainSummaryManager::updateSummaryFromAnnotations(const RetainSummary *&Summ,
                                                    const FunctionDecl *FD) {
   if (!FD)

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

 struct OSMetaClassBase {
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
 void escape(void *);
 void escape_with_source(void *p) {}
 bool coin();
 
+bool write_into_out_param_on_success(OS_RETURNS_RETAINED OSObject **obj);
+
+void use_out_param() {
+  OSObject *obj;
+  if (write_into_out_param_on_success(&obj)) {
+    obj->release();
+  }
+}
+
+void use_out_param_leak() {
+  OSObject *obj;
+  write_into_out_param_on_success(&obj); // expected-note{{Call to function 'write_into_out_param_on_success' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj'}}
+ // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
+
+typedef int kern_return_t;
+typedef kern_return_t IOReturn;
+typedef kern_return_t OSReturn;
+#define kOSReturnSuccess  0
+#define kIOReturnSuccess 0
+
+bool write_into_two_out_params(OS_RETURNS_RETAINED OSObject **a,
+                               OS_RETURNS_RETAINED OSObject **b);
+
+void use_write_into_two_out_params() {
+  OSObject *obj1;
+  OSObject *obj2;
+  if (write_into_two_out_params(&obj1, &obj2)) {
+    obj1->release();
+    obj2->release();
+  }
+}
+
+void use_write_two_out_params_leak() {
+  OSObject *obj1;
+  OSObject *obj2;
+  write_into_two_out_params(&obj1, &obj2); // expected-note{{Call to function 'write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}}
+                                           // expected-note@-1{{Call to function 'write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj1'}}
+  // expected-warning@-1{{Potential leak of an object stored into 'obj2'}}
+  // expected-note@-2{{Object leaked: object allocated and stored into 'obj1' is not referenced later in this execution path and has a retain count of +1}}
+  // expected-note@-3{{Object leaked: object allocated and stored into 'obj2' is not referenced later in this execution path and has a retain count of +1}}
+
+void always_write_into_two_out_params(OS_RETURNS_RETAINED OSObject **a,
+                                      OS_RETURNS_RETAINED OSObject **b);
+
+void use_always_write_into_two_out_params() {
+  OSObject *obj1;
+  OSObject *obj2;
+  always_write_into_two_out_params(&obj1, &obj2);
+  obj1->release();
+  obj2->release();
+}
+
+void use_always_write_into_two_out_params_leak() {
+  OSObject *obj1;
+  OSObject *obj2;
+  always_write_into_two_out_params(&obj1, &obj2); // expected-note{{Call to function 'always_write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}}
+                                                  // expected-note@-1{{Call to function 'always_write_into_two_out_params' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj1'}}
+  // expected-warning@-1{{Potential leak of an object stored into 'obj2'}}
+  // expected-note@-2{{Object leaked: object allocated and stored into 'obj1' is not referenced later in this execution path and has a retain count of +1}}
+  // expected-note@-3{{Object leaked: object allocated and stored into 'obj2' is not referenced later in this execution path and has a retain count of +1}}
+
+char *write_into_out_param_on_nonnull(OS_RETURNS_RETAINED OSObject **obj);
+
+void use_out_param_osreturn_on_nonnull() {
+  OSObject *obj;
+  if (write_into_out_param_on_nonnull(&obj)) {
+    obj->release();
+  }
+}
+
+void use_out_param_leak_osreturn_on_nonnull() {
+  OSObject *obj;
+  write_into_out_param_on_nonnull(&obj); // expected-note{{Call to function 'write_into_out_param_on_nonnull' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj'}}
+  // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
+
+bool write_optional_out_param(OS_RETURNS_RETAINED OSObject **obj=nullptr);
+
+void use_optional_out_param() {
+  if (write_optional_out_param()) {};
+}
+
+OSReturn write_into_out_param_on_os_success(OS_RETURNS_RETAINED OSObject **obj);
+
+void write_into_non_retained_out_param(OS_RETURNS_NOT_RETAINED OSObject **obj);
+
+void use_write_into_non_retained_out_param() {
+  OSObject *obj;
+  write_into_non_retained_out_param(&obj);
+}
+
+void use_write_into_non_retained_out_param_uaf() {
+  OSObject *obj;
+  write_into_non_retained_out_param(&obj); // expected-note{{Call to function 'write_into_non_retained_out_param' returns an OSObject of type OSObject with a +0 retain count}}
+  obj->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
+                  // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
+}
+
+void always_write_into_out_param(OS_RETURNS_RETAINED OSObject **obj);
+
+void use_void_out_param_osreturn() {
+  OSObject *obj;
+  always_write_into_out_param(&obj);
+  obj->release();
+}
+
+void use_void_out_param_osreturn_leak() {
+  OSObject *obj;
+  always_write_into_out_param(&obj); // expected-note{{Call to function 'always_write_into_out_param' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj'}}
+  // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
+
+void use_out_param_osreturn() {
+  OSObject *obj;
+  if (write_into_out_param_on_os_success(&obj) == kOSReturnSuccess) {
+    obj->release();
+  }
+}
+
+void use_out_param_leak_osreturn() {
+  OSObject *obj;
+  write_into_out_param_on_os_success(&obj); // expected-note{{Call to function 'write_into_out_param_on_os_success' returns an OSObject of type OSObject with a +1 retain count}}
+} // expected-warning{{Potential leak of an object stored into 'obj'}}
+  // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
+
 bool os_consume_violation_two_args(OS_CONSUME OSObject *obj, bool extra) {
   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 did not consume the reference}}
 }

 }
 
 OSObject *getObject();
 typedef bool (^Blk)(OSObject *);
 
 void test_escape_to_unknown_block(Blk blk) {
   blk(getObject()); // no-crash
 }
-