[analyzer] Fix a crash in NoStateChangeVisitor with body-farmed stack frames.

[NoQ]

Summary

LocationContext::getDecl() isn't useful for obtaining the "farmed" body because the (synthetic) body statement isn't actually attached to the (natural-grown) declaration in the AST. This manifests in hard crashes every time the visitor tries to scan the body-farmed stack frame.

Note that it's technically possible to obtain and scan the body. It's useless to do so though, given that you can't put notes into it, given that it doesn't map into any user source code.

The C test case is libdispatch/blocks-specific but the C++ test case is something that can happen to everybody, even though the premise is roughly the same.

[steakhal]

I dont even see why should the no-state-changed-visitor attempt to do anything with that call. It should have disarm itself when it reaches the allocation. It its not the case, we should probably consider fixing that as well in a followup.
Please consider rearranging the code to make it possible for the synthetized callee to actually deallocate the memory.

[NoQ]

Oh, great point. Added tests that were crashing the other way round so that to demonstrate that this fix is still justified. But I agree that the visitor can totally terminate early so that to make the original tests pass for two reasons!

[steakhal]

Thanks.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
       if (const FunctionDecl *FD = CE->getDirectCallee())
         return FD->getQualifiedNameAsString();
     return "";
   }
 
   bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
     using namespace clang::ast_matchers;
     const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
-    if (!FD)
+
+    // Given that the stack frame was entered, the body should always be
+    // theoretically obtainable. In case of body farms, the synthesized body
+    // is not attached to declaration, thus triggering the '!FD->hasBody()'
+    // branch. That said, would a synthesized body ever intend to handle
+    // ownership? As of today they don't. And if they did, how would we
+    // put notes inside it, given that it doesn't match any source locations?
+    if (!FD || !FD->hasBody())
       return false;
+
     // TODO: Operator delete is hardly the only deallocator -- Can we reuse
     // isFreeingCall() or something thats already here?
     auto Deallocations = match(
         stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
              ), *FD->getBody(), ACtx);
     // TODO: Ownership my change with an attempt to store the allocated memory.
     return !Deallocations.empty();
   }

clang/test/Analysis/malloc-bodyfarms.c

+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker core,unix -verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+void *calloc(size_t, size_t);
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef void (^dispatch_block_t)(void);
+void dispatch_sync(dispatch_queue_t, dispatch_block_t);
+
+void test_no_state_change_in_body_farm(dispatch_queue_t queue) {
+  dispatch_sync(queue, ^{}); // no-crash
+  calloc(1, 1);
+} // expected-warning{{Potential memory leak}}
+
+void test_no_state_change_in_body_farm_2(dispatch_queue_t queue) {
+  void *p = calloc(1, 1);
+  dispatch_sync(queue, ^{}); // no-crash
+  p = 0;
+} // expected-warning{{Potential leak of memory pointed to by 'p'}}

clang/test/Analysis/malloc-bodyfarms.cpp

+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker core,unix -verify %s
+
+namespace std {
+typedef struct once_flag_s {
+  int _M_once = 0;
+} once_flag;
+
+template <class Callable, class... Args>
+void call_once(once_flag &o, Callable&& func, Args&&... args);
+} // namespace std
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+void callee() {}
+
+void test_no_state_change_in_body_farm() {
+  std::once_flag flag;
+  call_once(flag, callee); // no-crash
+  malloc(1);
+} // expected-warning{{Potential memory leak}}
+
+void test_no_state_change_in_body_farm_2() {
+  void *p = malloc(1);
+  std::once_flag flag;
+  call_once(flag, callee); // no-crash
+  p = 0;
+} // expected-warning{{Potential leak of memory pointed to by 'p'}}