[analyzer] Clean up the program state map of DanglingInternalBufferChecker

[rnkovacs]

Summary

Symbols are cleaned up from the program state map when they go out of scope.
Memory regions are cleaned up when the corresponding object is destroyed, and
additionally in checkDeadSymbols in case destructor modeling was incomplete.

[xazax.hun]

LG!

[NoQ]

Yep, great stuff!

I guess, please add a comment on incomplete destructor support in the CFG and/or analyzer core that may cause the region to be never cleaned up. Or perform an extra cleanup pass - not sure if it's worth it, but it should be easy and safe.

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

97

It's kinda weird that something that's not a const MemRegion * is called "Region".

I'd rather use a neutral term like "Item".

[rnkovacs]

Fixed naming and added an extra pass for regions left behind by incomplete destructors.

[NoQ]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

92–94

This optimization is in fact incorrect due to an old bug that i didn't yet get to fixing: D18860. The proposed patch would most likely remove the check anyway, because the set of dead symbols is not well-defined. So i think we shouldn't add it.

98

For the same reason (D18860), it should be more correct to use !isLive(). Otherwise you may miss some symbols.

100–103

I mildly advocate for braces here because that's as many as three lines to wrap.

[rnkovacs]

Addressed comments.

[NoQ]

Great, thanks!

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "AllocationState.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
-class DanglingInternalBufferChecker : public Checker<check::PostCall> {
+class DanglingInternalBufferChecker : public Checker<check::DeadSymbols,
+                                                     check::PostCall> {
   CallDescription CStrFn;
 
 public:
   DanglingInternalBufferChecker() : CStrFn("c_str") {}
 
   /// Record the connection between the symbol returned by c_str() and the
   /// corresponding string object region in the ProgramState. Mark the symbol
   /// released if the string object is destroyed.
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+  /// Clean up the ProgramState map.
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 };
 
 } // end anonymous namespace
 
 // FIXME: c_str() may be called on a string object many times, so it should
 // have a list of symbols associated with it.
 REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
 
void DanglingInternalBufferChecker::checkPostCall(const CallEvent &Call,
   }
 
   if (isa<CXXDestructorCall>(ICall)) {
     if (State->contains<RawPtrMap>(TypedR)) {
       const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
       // FIXME: What if Origin is null?
       const Expr *Origin = Call.getOriginExpr();
       State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
+      State = State->remove<RawPtrMap>(TypedR);
       C.addTransition(State);
       return;
     }
   }
 }
 
+void DanglingInternalBufferChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+                                                     CheckerContext &C) const {
+  if (!SymReaper.hasDeadSymbols())
+    return;
+
+  ProgramStateRef State = C.getState();
+  RawPtrMapTy RPM = State->get<RawPtrMap>();
+  for (const auto Entry : RPM) {
+    if (SymReaper.isDead(Entry.second))
+      State = State->remove<RawPtrMap>(Entry.first);
+    if (!SymReaper.isLiveRegion(Entry.first))
+      // Due to incomplete destructor support, some dead regions might still
+      // remain in the program state map. Clean them up.
+      State = State->remove<RawPtrMap>(Entry.first);
+  }
+
+  C.addTransition(State);
+}
+
 void ento::registerDanglingInternalBufferChecker(CheckerManager &Mgr) {
   registerNewDeleteChecker(Mgr);
   Mgr.registerChecker<DanglingInternalBufferChecker>();
 }