[analyzer] Highlight c_str() call in DanglingInternalBuffer checker

[rnkovacs]

Summary

Add a bug visitor to DanglingInternalBuffer checker that places a note at the point where the dangling pointer was obtained.
The visitor is handed over to MallocChecker and attached to the report there.

[xazax.hun]

Regarding the visitor:
Maybe rather than looking at the AST, we should check the states, when we started to track the returned symbol?

Using your current design you need to check for the AST twice. Once in the visitor and once in the check.

Also, I wonder if this always give you the right note. Consider the following example:

void deref_after_scope_char() {
  const char *c;
  {
    std::string s;
    c = s.c_str();
  }
  std::string s;
  const char *c2 = s.c_str();
  consume(c); 
}

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

161

Why not !=?

[rnkovacs]

Um, sorry, I totally forgot about that. Added your case to the tests.

[xazax.hun]

Looks better, thanks!

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

65

Maybe early return here?

[NoQ]

lib/StaticAnalyzer/Checkers/AllocationState.h

23–25

I think we should start commenting this stuff up. Like, "This function provides an additional visitor that augments the bug report with information relevant to memory errors caused by misuse of AF_InternalBuffer symbols".

[NoQ]

Looks good tho!

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

63–64

Interesting, so we don't have access to the region with which the symbol is associated, so we have to scan the whole map.

Probably we can scan the map only once (eg., in the visitor's consturctor if we also supply the program state) and then do a direct lookup by region?

Because it's a premature optimization, i'm in favor of a FIXME.

155

Maybe "pointer to dangling buffer".

[rnkovacs]

Thanks! Addressed comments.

[xazax.hun]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

41

I am fine with this as is, but I prefer self documenting code in general. Naming this variable PtrToBuf or something like that would conway the same information and render the comment redundant.

[rnkovacs]

Fixed variable name inside the visitor.
I also clang-formatted the file, sorry for any line number shifting.

[rnkovacs]

Fixed the constness of c_str() in the test file.

lib/StaticAnalyzer/Checkers/AllocationState.h

 namespace clang {
 namespace ento {
 
 namespace allocation_state {
 
 ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym,
                              const Expr *Origin);
 
+std::unique_ptr<BugReporterVisitor>
+getDanglingBufferBRVisitor(SymbolRef Sym);
+
 } // end namespace allocation_state
 
 } // end namespace ento
 } // end namespace clang
 
 #endif

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "AllocationState.h"
 
 using namespace clang;
 using namespace ento;
 
+// 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)
+
 namespace {
 
 class DanglingInternalBufferChecker : public Checker<check::DeadSymbols,
                                                      check::PostCall> {
   CallDescription CStrFn;
 
 public:
+  class DanglingBufferBRVisitor
+      : public BugReporterVisitorImpl<DanglingBufferBRVisitor> {
+    // Tracked pointer to a buffer.
+    SymbolRef Sym;
+
+  public:
+    DanglingBufferBRVisitor(SymbolRef Sym) : Sym(Sym) {}
+
+    static void *getTag() {
+      static int Tag = 0;
+      return &Tag;
+    }
+
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      ID.AddPointer(getTag());
+    }
+
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                   const ExplodedNode *PrevN,
+                                                   BugReporterContext &BRC,
+                                                   BugReport &BR) override;
+
+    bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
+      bool Found = false;
+      RawPtrMapTy Map = State->get<RawPtrMap>();
+      for (const auto Entry : Map) {
+        if (Entry.second == Sym)
+          Found = true;
+      }
+      return Found;
+    }
+
+  };
+
   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,
                                                   CheckerContext &C) const {
   const auto *ICall = dyn_cast<CXXInstanceCall>(&Call);
   if (!ICall)
     return;
 
   SVal Obj = ICall->getCXXThisVal();
   const auto *TypedR = dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion());
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);
 }
 
+std::shared_ptr<PathDiagnosticPiece>
+DanglingInternalBufferChecker::DanglingBufferBRVisitor::VisitNode(
+    const ExplodedNode *N, const ExplodedNode *PrevN,
+    BugReporterContext &BRC, BugReport &BR) {
+
+  if (!isSymbolTracked(N->getState(), Sym) ||
+      isSymbolTracked(PrevN->getState(), Sym))
+    return nullptr;
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream OS(Buf);
+  OS << "Dangling buffer was obtained here";
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                             N->getLocationContext());
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(),
+                                                    true, nullptr);
+}
+
+namespace clang {
+namespace ento {
+namespace allocation_state {
+
+std::unique_ptr<BugReporterVisitor>
+getDanglingBufferBRVisitor(SymbolRef Sym) {
+  return llvm::make_unique<DanglingInternalBufferChecker::DanglingBufferBRVisitor>(Sym);
+}
+
+} // end namespace allocation_state
+} // end namespace ento
+} // end namespace clang
+
 void ento::registerDanglingInternalBufferChecker(CheckerManager &Mgr) {
   registerNewDeleteChecker(Mgr);
   Mgr.registerChecker<DanglingInternalBufferChecker>();
 }

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (!BT_UseFree[*CheckKind])
           CheckNames[*CheckKind], "Use-after-free", categories::MemoryError));
 
     auto R = llvm::make_unique<BugReport>(*BT_UseFree[*CheckKind],
                                          "Use of memory after it is freed", N);
 
     R->markInteresting(Sym);
     R->addRange(Range);
     R->addVisitor(llvm::make_unique<MallocBugVisitor>(Sym));
+
+    const RefState *RS = C.getState()->get<RegionState>(Sym);
+    if (RS->getAllocationFamily() == AF_InternalBuffer)
+      R->addVisitor(allocation_state::getDanglingBufferBRVisitor(Sym));
+
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::ReportDoubleFree(CheckerContext &C, SourceRange Range,
                                      bool Released, SymbolRef Sym,
                                      SymbolRef PrevSym) const {
 

test/Analysis/dangling-internal-buffer.cpp

 void consume(const wchar_t *) {}
 void consume(const char16_t *) {}
 void consume(const char32_t *) {}
 
 void deref_after_scope_char() {
   const char *c;
   {
     std::string s;
-    c = s.c_str();
+    c = s.c_str(); // expected-note {{Dangling buffer was obtained here}}
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
+  consume(c); // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_char2() {
+  const char *c;
+  {
+    std::string s;
+    c = s.c_str(); // expected-note {{Dangling buffer was obtained here}}
   } // expected-note {{Internal buffer is released because the object was destroyed}}
+  std::string s;
+  const char *c2 = s.c_str();
   consume(c); // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
 void deref_after_scope_wchar_t() {
   const wchar_t *w;
   {
     std::wstring ws;
-    w = ws.c_str();
+    w = ws.c_str(); // expected-note {{Dangling buffer was obtained here}}
   } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(w); // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
 void deref_after_scope_char16_t() {
   const char16_t *c16;
   {
     std::u16string s16;
-    c16 = s16.c_str();
+    c16 = s16.c_str(); // expected-note {{Dangling buffer was obtained here}}
   } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(c16); // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
 void deref_after_scope_char32_t() {
   const char32_t *c32;
   {
     std::u32string s32;
-    c32 = s32.c_str();
+    c32 = s32.c_str(); // expected-note {{Dangling buffer was obtained here}}
   } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(c32); // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
 void deref_after_scope_ok() {
   const char *c;
   std::string s;
   {
     c = s.c_str();
   }
   consume(c); // no-warning
 }