[analyzer] WIP: MallocChecker: Release temporary CXXNewExpr

[Charusso]

Summary

-

[Charusso]

This is heavily WIP as sometimes we have to release a new after we return it or a constructor did something with that. The direction is okay?

[NoQ]

In such cases i recommend starting with writing down a test. Like in TDD: first test, then code.

The general direction doesn't seem reasonable to me; it introduces some pattern-matching for a specific scenario, but it's unclear why is this scenario a problem on its own. We might eventually do something similar, but I recommend fully debugging the false positive - i.e., understanding what exactly is wrong with it, before picking a suppression mechanism.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1119–1122

I think it's clearly too early for marking the pointer as released. I.e., what about:

auto x = std::shared_ptr(new int); // the pointer is marked as released
use(x.get()); // use-after-free???

[Charusso]

The seen error solved by D63720.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

struct ReallocPair {
   bool operator==(const ReallocPair &X) const {
     return ReallocatedSym == X.ReallocatedSym &&
            Kind == X.Kind;
   }
 };
 
 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,
-                                     eval::Assume>
-{
+class MallocChecker
+    : public Checker<check::DeadSymbols, check::PointerEscape,
+                     check::ConstPointerEscape, check::PreStmt<ReturnStmt>,
+                     check::EndFunction, check::PreCall,
+                     check::PostStmt<CallExpr>, check::PostStmt<CXXNewExpr>,
+                     check::PostStmt<CXXBindTemporaryExpr>, check::NewAllocator,
+                     check::PreStmt<CXXDeleteExpr>, check::PostStmt<BlockExpr>,
+                     check::PostObjCMessage, check::Location, eval::Assume> {
 public:
   MallocChecker()
       : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
         II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr),
         II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),
         II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr),
         II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
         II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr),
public:
   DefaultBool IsOptimistic;
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckName CheckNames[CK_NumCheckKinds];
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
   void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
+  void checkPostStmt(const CXXBindTemporaryExpr *BTE, 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;
public:
 
     inline bool isReleased(const RefState *S, const RefState *SPrev,
                            const Stmt *Stmt) {
       // Did not track -> released. Other state (allocated) -> released.
       // The statement associated with the release might be missing.
       bool IsReleased = (S && S->isReleased()) &&
                         (!SPrev || !SPrev->isReleased());
       assert(!IsReleased ||
-             (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt))) ||
+             (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt) ||
+                       isa<CXXBindTemporaryExpr>(Stmt))) ||
              (!Stmt && S->getAllocationFamily() == AF_InnerBuffer));
       return IsReleased;
     }
 
     inline bool isRelinquished(const RefState *S, const RefState *SPrev,
                                const Stmt *Stmt) {
       // Did not track -> relinquished. Other state (allocated) -> relinquished.
       return (Stmt && (isa<CallExpr>(Stmt) || isa<ObjCMessageExpr>(Stmt) ||

 }
 
 void MallocChecker::checkNewAllocator(const CXXNewExpr *NE, SVal Target,
                                       CheckerContext &C) const {
   if (!C.wasInlined)
     processNewAllocation(NE, C, Target);
 }
 
+void MallocChecker::checkPostStmt(const CXXBindTemporaryExpr *BTE,
+                                  CheckerContext &C) const {
+  if (const auto *CE = dyn_cast<CXXConstructExpr>(BTE->getSubExpr())) {
+    if (CE->getNumArgs() == 0)
+      return;
+
+    // If we catch a 'CXXNewExpr' set it is released as it is temporary.
+    if (const auto *NE = dyn_cast<CXXNewExpr>(CE->getArg(0))) {
+      ProgramStateRef State = C.getState();
+      Optional<SVal> RetVal = C.getSVal(NE);
+
+      SymbolRef Sym = RetVal->getAsLocSymbol();
+      assert(Sym);
+
+      if (const RefState *RS = State->get<RegionState>(Sym))
+        if (RS->isReleased())
+          return;
+
+      // Set the symbol's state to Released.
+      State = State->set<RegionState>(
+          Sym, RefState::getReleased(NE->isArray() ? AF_CXXNewArray : AF_CXXNew,
+                                     NE));
+
+      C.addTransition(State);
+    }
+  }
+}
+
 // Sets the extent value of the MemRegion allocated by
 // new expression NE to its size in Bytes.
 //
 ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
                                              const CXXNewExpr *NE,
                                              ProgramStateRef State,
                                              SVal Target) {
   if (!State)