[analyzer] Assume escape is possible through system functions taking void*

[zaks.anna]

Summary

The analyzer assumes that system functions will not free memory or modify the arguments in other ways, so we assume that arguments do not escape when those are called. However, this may lead to false positive leak errors. For example, in code like this where the pointers added to the rb_tree are freed later on:

		struct alarm_event *e = calloc(1, sizeof(*e));

<snip>

		rb_tree_insert_node(&alarm_tree, e);

Add a heuristic to assume that calls to system functions taking void* arguments allow for pointer escape.

[dcoughlin]

Has a typo but otherwise LGTM.

include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

258

Did you consider using std::function<bool(QualType)> instead of a function pointer so that clients could use lambdas that capture to provide the condition?

test/Analysis/Inputs/system-header-simulator.h

85

Typo "strauctures"

[zaks.anna]

Committed in r251449.

include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

public:
   QualType getResultType() const;
 
   /// \brief Returns the return value of the call.
   ///
   /// This should only be called if the CallEvent was created using a state in
   /// which the return value has already been bound to the origin expression.
   SVal getReturnValue() const;
 
+  /// \brief Returns true if the type of any of the non-null arguments satisfies
+  /// the condition.
+  bool hasNonNullArgumentsWithType(bool (*Condition)(QualType)) const;
+
   /// \brief Returns true if any of the arguments appear to represent callbacks.
   bool hasNonZeroCallbackArg() const;
 
+  /// \brief Returns true if any of the arguments is void*.
+  bool hasVoidPointerToNonConstArg() const;
+
   /// \brief Returns true if any of the arguments are known to escape to long-
   /// term storage, even if this method will not modify them.
   // NOTE: The exact semantics of this are still being defined!
   // We don't really want a list of hardcoded exceptions in the long run,
   // but we don't want duplicated lists of known APIs in the short term either.
   virtual bool argumentsMayEscape() const {
     return hasNonZeroCallbackArg();
   }

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
   // mysterious reasons.
   if (!(isa<SimpleFunctionCall>(Call) || isa<ObjCMethodCall>(Call)))
     return true;
 
   // Check Objective-C messages by selector name.
   if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(Call)) {
     // If it's not a framework call, or if it takes a callback, assume it
     // can free memory.
-    if (!Call->isInSystemHeader() || Call->hasNonZeroCallbackArg())
+    if (!Call->isInSystemHeader() || Call->argumentsMayEscape())
       return true;
 
     // If it's a method we know about, handle it explicitly post-call.
     // This should happen before the "freeWhenDone" check below.
     if (isKnownDeallocObjCMethodName(*Msg))
       return false;
 
     // If there's a "freeWhenDone" parameter, but the method isn't one we know

lib/StaticAnalyzer/Core/CallEvent.cpp

QualType CallEvent::getResultType() const {
   case VK_RValue:
     // No adjustment is necessary.
     break;
   }
 
   return ResultTy;
 }
 
-static bool isCallbackArg(SVal V, QualType T) {
-  // If the parameter is 0, it's harmless.
-  if (V.isZeroConstant())
-    return false;
-
+static bool isCallback(QualType T) {
   // If a parameter is a block or a callback, assume it can modify pointer.
   if (T->isBlockPointerType() ||
       T->isFunctionPointerType() ||
       T->isObjCSelType())
     return true;
 
   // Check if a callback is passed inside a struct (for both, struct passed by
   // reference and by value). Dig just one level into the struct for now.
 
   if (T->isAnyPointerType() || T->isReferenceType())
     T = T->getPointeeType();
 
   if (const RecordType *RT = T->getAsStructureType()) {
     const RecordDecl *RD = RT->getDecl();
     for (const auto *I : RD->fields()) {
       QualType FieldT = I->getType();
       if (FieldT->isBlockPointerType() || FieldT->isFunctionPointerType())
         return true;
     }
   }
+  return false;
+}
 
+static bool isVoidPointerToNonConst(QualType T) {
+  if (const PointerType *PT = T->getAs<PointerType>()) {
+    QualType PointeeTy = PT->getPointeeType();
+    if (PointeeTy.isConstQualified())
+      return false;
+    return PointeeTy->isVoidType();
+  } else
     return false;
 }
 
-bool CallEvent::hasNonZeroCallbackArg() const {
+bool CallEvent::hasNonNullArgumentsWithType(bool (*Condition)(QualType)) const {
   unsigned NumOfArgs = getNumArgs();
 
   // If calling using a function pointer, assume the function does not
-  // have a callback. TODO: We could check the types of the arguments here.
+  // satisfy the callback.
+  // TODO: We could check the types of the arguments here.
   if (!getDecl())
     return false;
 
   unsigned Idx = 0;
   for (CallEvent::param_type_iterator I = param_type_begin(),
-                                       E = param_type_end();
        I != E && Idx < NumOfArgs; ++I, ++Idx) {
     if (NumOfArgs <= Idx)
       break;
 
-    if (isCallbackArg(getArgSVal(Idx), *I))
+    // If the parameter is 0, it's harmless.
+    if (getArgSVal(Idx).isZeroConstant())
+      continue;
+
+    if (Condition(*I))
       return true;
   }
-
   return false;
 }
 
+bool CallEvent::hasNonZeroCallbackArg() const {
+  return hasNonNullArgumentsWithType(isCallback);
+}
+
+bool CallEvent::hasVoidPointerToNonConstArg() const {
+  return hasNonNullArgumentsWithType(isVoidPointerToNonConst);
+}
+
 bool CallEvent::isGlobalCFunction(StringRef FunctionName) const {
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(getDecl());
   if (!FD)
     return false;
 
   return CheckerContext::isCLibraryFunction(FD, FunctionName);
 }
 
void AnyFunctionCall::getInitialStackFrameContents(
                                         BindingsTy &Bindings) const {
   const FunctionDecl *D = cast<FunctionDecl>(CalleeCtx->getDecl());
   SValBuilder &SVB = getState()->getStateManager().getSValBuilder();
   addParameterValuesToBindings(CalleeCtx, Bindings, SVB, *this,
                                D->parameters());
 }
 
 bool AnyFunctionCall::argumentsMayEscape() const {
-  if (hasNonZeroCallbackArg())
+  if (CallEvent::argumentsMayEscape() || hasVoidPointerToNonConstArg())
     return true;
 
   const FunctionDecl *D = getDecl();
   if (!D)
     return true;
 
   const IdentifierInfo *II = D->getIdentifier();
   if (!II)

test/Analysis/Inputs/system-header-simulator.h

 void xpc_connection_set_context(xpc_connection_t connection, void *context);
 void xpc_connection_set_finalizer_f(xpc_connection_t connection, xpc_finalizer_t finalizer);
 void xpc_connection_resume(xpc_connection_t connection);
 
 //The following are fake system header functions for generic testing.
 void fakeSystemHeaderCallInt(int *);
 void fakeSystemHeaderCallIntPtr(int **);
 
+// Some data strauctures may hold onto the pointer and free it later.
+void fake_insque(void *, void *);
+typedef struct fake_rb_tree { void *opaque[8]; } fake_rb_tree_t;
+void fake_rb_tree_init(fake_rb_tree_t *, const void *);
+void *fake_rb_tree_insert_node(fake_rb_tree_t *, void *);
+
 typedef struct __SomeStruct {
   char * p;
 } SomeStruct;
 void fakeSystemHeaderCall(SomeStruct *);

test/Analysis/malloc.c

 }
 
 int *radar15580979() {
   int *data = (int *)malloc(32);
   int *p = data ?: (int*)malloc(32); // no warning
   return p;
 }
 
+// Some data structures may hold onto the pointer and free it later.
+void testEscapeThroughSystemCallTakingVoidPointer1(void *queue) {
+  int *data = (int *)malloc(32);
+  fake_insque(queue, data); // no warning
+}
+
+void testEscapeThroughSystemCallTakingVoidPointer2(fake_rb_tree_t *rbt) {
+  int *data = (int *)malloc(32);
+  fake_rb_tree_init(rbt, data);
+} //expected-warning{{Potential leak}}
+
+void testEscapeThroughSystemCallTakingVoidPointer3(fake_rb_tree_t *rbt) {
+  int *data = (int *)malloc(32);
+  fake_rb_tree_init(rbt, data);
+  fake_rb_tree_insert_node(rbt, data); // no warning
+}
+
 // ----------------------------------------------------------------------------
 // False negatives.
 
 void testMallocWithParam(int **p) {
   *p = (int*) malloc(sizeof(int));
   *p = 0; // FIXME: should warn here
 }