[analyzer] Detect pointers escaped after return statement execution in MallocChecker

[rnkovacs]

Summary

Sometimes an object is destroyed right after the statement returning it is executed. This patch aims to make MallocChecker warn for these cases as well.

FIXME1: Using two traits might not be the best solution (fixed).
FIXME2: The warning is emitted at the end of the function, which might be confusing.

[NoQ]

Aha, so the destructors are called after the return statement! Makes sense.

The return statement is available in ExprEngine::processEndOfFunction(), so i guess we could improve the checkEndFunction() callback to provide it, and then we wouldn't need any of those extra state traits: the returned expression would be a sub-expression of the return statement and the return value would be, well, the value of the returned expression (hopefully). The procedure of adding more arguments to a callback is a bit annoying but straightforward.

I guess it'd be great to add a MallocChecker-only test. It should be possible by mocking a simple object that frees memory in its destructor (in a manner that the Analyzer can understand by inlining all methods of the object) and then returning that memory.

The warning is emitted at the end of the function, which might be confusing.

I'm afraid it might be hard to fix; path-sensitive reports are usually thrown against the node and that node is already too far and the return statement node is too early.

@george.karpenkov would your getEndPath()-fu help here?

[rnkovacs]

Updated to use the extended checkEndFunction() callback (committed in rL337215 - I forgot to add it as a dependency).

The first ones of the 2-2 tests added for InnerPointerChecker and NewDeleteChecker already worked before this patch, the second ones are the newly working ones. I just felt they are somewhat both relevant and maybe worth comparing.

[NoQ]

Yup, tests for temporaries are great to have as well.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2475–2477

The old code makes the warning appear slightly earlier, so we still need it, even if duplicated. Let's comment about that and maybe de-duplicate?

[rnkovacs]

De-duplicate & add comment.

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2488

Let's do a common sub-function, similarly to how MallocChecker::checkPointerEscape and MallocChecker::checkConstPointerEscape both call MallocChecker::checkPointerEscapeAux. Should prevent us from unleashing addTransition hell if the code becomes more complicated.

[rnkovacs]

Add helper function to be used in both callbacks.

[NoQ]

All this stuff looks great! Please commit.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 };
 
 typedef std::pair<const ExplodedNode*, const MemRegion*> LeakInfo;
 
 class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PointerEscape,
                                      check::ConstPointerEscape,
                                      check::PreStmt<ReturnStmt>,
+                                     check::EndFunction,
                                      check::PreCall,
                                      check::PostStmt<CallExpr>,
                                      check::PostStmt<CXXNewExpr>,
                                      check::NewAllocator,
                                      check::PreStmt<CXXDeleteExpr>,
                                      check::PostStmt<BlockExpr>,
                                      check::PostObjCMessage,
                                      check::Location,
public:
   void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
   void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
                          CheckerContext &C) const;
   void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
   void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
+  void checkEndFunction(const ReturnStmt *S, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
                             bool Assumption) const;
   void checkLocation(SVal l, bool isLoad, const Stmt *S,
                      CheckerContext &C) const;
 
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                     const InvalidatedSymbols &Escaped,
                                     const CallEvent *Call,
if (!Sym)
     // the callee could still free the memory.
     // TODO: This logic should be a part of generic symbol escape callback.
     if (const MemRegion *MR = RetVal.getAsRegion())
       if (isa<FieldRegion>(MR) || isa<ElementRegion>(MR))
         if (const SymbolicRegion *BMR =
               dyn_cast<SymbolicRegion>(MR->getBaseRegion()))
           Sym = BMR->getSymbol();
 
   // Check if we are returning freed memory.
   if (Sym)
     checkUseAfterFree(Sym, C, E);
 }
 
+void MallocChecker::checkEndFunction(const ReturnStmt *S,
+                                     CheckerContext &C) const {
+  if (!S)
+    return;
+
+  const Expr *E = S->getRetValue();
+  if (!E)
+    return;
+
+  SymbolRef Sym = C.getSVal(E).getAsSymbol();
+  if (Sym)
+    checkUseAfterFree(Sym, C, E);
+}
+
 // TODO: Blocks should be either inlined or should call invalidate regions
 // upon invocation. After that's in place, special casing here will not be
 // needed.
 void MallocChecker::checkPostStmt(const BlockExpr *BE,
                                   CheckerContext &C) const {
 
   // Scan the BlockDecRefExprs for any object the retain count checker
   // may be tracking.

test/Analysis/inner-pointer.cpp

void deref_after_swap() {
   const char *c;
   std::string s1, s2;
   c = s1.data(); // expected-note {{Dangling inner pointer obtained here}}
   s1.swap(s2);   // expected-note {{Inner pointer invalidated by call to 'swap'}}
   consume(c);    // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
+struct S {
+  std::string to_string() { return s; }
+private:
+  std::string s;
+};
+
+const char *escape_via_return_temp() {
+  S x;
+  return x.to_string().c_str(); // expected-note {{Dangling inner pointer obtained here}}
+  // expected-note@-1 {{Inner pointer invalidated by call to destructor}}
+  // expected-warning@-2 {{Use of memory after it is freed}}
+  // expected-note@-3 {{Use of memory after it is freed}}
+}
+
+const char *escape_via_return_local() {
+  std::string s;
+  return s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+                    // expected-note@-1 {{Inner pointer invalidated by call to destructor}}
+} // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+
 void deref_after_scope_ok(bool cond) {
   const char *c, *d;
   std::string s;
   {
     c = s.c_str();
     d = s.data();
   }
   if (cond)
     consume(c); // no-warning
   else
     consume(d); // no-warning
 }

test/Analysis/malloc-free-after-return.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete -analyzer-output=text -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+struct S {
+  S() : Data(new int) {}
+  ~S() { delete Data; }
+  int *getData() { return Data; }
+
+private:
+  int *Data;
+};
+
+int *freeAfterReturnTemp() {
+  return S().getData(); // expected-warning {{Use of memory after it is freed}}
+}
+
+int *freeAfterReturnLocal() {
+  S X;
+  return X.getData();
+} // expected-warning {{Use of memory after it is freed}}