[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

2495–2497

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

2508

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,
ProgramStateRef ReallocMemAux(CheckerContext &C, const CallExpr *CE,
                                 bool FreesMemOnFailure,
                                 ProgramStateRef State,
                                 bool SuffixWithN = false) const;
   static SVal evalMulForBufferSize(CheckerContext &C, const Expr *Blocks,
                                    const Expr *BlockBytes);
   static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE,
                                    ProgramStateRef State);
 
-  ///Check if the memory associated with this symbol was released.
+  /// Check if the memory associated with this symbol was released.
   bool isReleased(SymbolRef Sym, CheckerContext &C) const;
 
   bool checkUseAfterFree(SymbolRef Sym, CheckerContext &C, const Stmt *S) const;
 
   void checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
                              const Stmt *S) const;
 
   bool checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const;
 
   /// Check if the function is known free memory, or if it is
   /// "interesting" and should be modeled explicitly.
   ///
   /// \param [out] EscapingSymbol A function might not free memory in general,
   ///   but could be known to free a particular symbol. In this case, false is
   ///   returned and the single escaping symbol is returned through the out
   ///   parameter.
   ///
   /// We assume that pointers do not escape through calls to system functions
   /// not handled by this checker.
   bool mayFreeAnyEscapedMemoryOrIsModeledExplicitly(const CallEvent *Call,
                                    ProgramStateRef State,
                                    SymbolRef &EscapingSymbol) const;
 
-  // Implementation of the checkPointerEscape callabcks.
+  // Implementation of the checkPointerEscape callbacks.
   ProgramStateRef checkPointerEscapeAux(ProgramStateRef State,
                                   const InvalidatedSymbols &Escaped,
                                   const CallEvent *Call,
                                   PointerEscapeKind Kind,
                                   bool(*CheckRefState)(const RefState*)) const;
 
+  // Implementation of the checkPreStmt and checkEndFunction callbacks.
+  void checkEscapeOnReturn(const ReturnStmt *S, CheckerContext &C) const;
+
   ///@{
   /// Tells if a given family/call/symbol is tracked by the current checker.
   /// Sets CheckKind to the kind of the checker responsible for this
   /// family/call/symbol.
   Optional<CheckKind> getCheckIfTracked(AllocationFamily Family,
                                         bool IsALeakCheck = false) const;
   Optional<CheckKind> getCheckIfTracked(CheckerContext &C,
                                         const Stmt *AllocDeallocStmt,
if (ArgSVal.getAs<Loc>()) {
       if (!Sym)
         continue;
       if (checkUseAfterFree(Sym, C, Call.getArgExpr(I)))
         return;
     }
   }
 }
 
-void MallocChecker::checkPreStmt(const ReturnStmt *S, CheckerContext &C) const {
+void MallocChecker::checkPreStmt(const ReturnStmt *S,
+                                 CheckerContext &C) const {
+  checkEscapeOnReturn(S, C);
+}
+
+// In the CFG, automatic destructors come after the return statement.
+// This callback checks for returning memory that is freed by automatic
+// destructors, as those cannot be reached in checkPreStmt().
+void MallocChecker::checkEndFunction(const ReturnStmt *S,
+                                     CheckerContext &C) const {
+  checkEscapeOnReturn(S, C);
+}
+
+void MallocChecker::checkEscapeOnReturn(const ReturnStmt *S,
+                                        CheckerContext &C) const {
+  if (!S)
+    return;
+
   const Expr *E = S->getRetValue();
   if (!E)
     return;
 
   // Check if we are returning a symbol.
   ProgramStateRef State = C.getState();
   SVal RetVal = C.getSVal(E);
   SymbolRef Sym = RetVal.getAsSymbol();
   if (!Sym)
     // If we are returning a field of the allocated struct or an array element,
     // 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);
 }
 
 // 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.
   if (!BE->getBlockDecl()->hasCaptures())
     return;
 
   ProgramStateRef state = C.getState();
   const BlockDataRegion *R =
     cast<BlockDataRegion>(C.getSVal(BE).getAsRegion());
 
   BlockDataRegion::referenced_vars_iterator I = R->referenced_vars_begin(),
                                             E = R->referenced_vars_end();

test/Analysis/inner-pointer.cpp

 void func_default_arg() {
   const char *c;
   std::string s;
   c = s.c_str();     // expected-note {{Dangling inner pointer obtained here}}
   default_arg(3, s); // expected-note {{Inner pointer invalidated by call to 'default_arg'}}
   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}}

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

+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete -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}}