[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

159

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

24–26

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.

153

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

 //===--- AllocationState.h ------------------------------------- *- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H
 #define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H
 
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 
 namespace clang {
 namespace ento {
 
 namespace allocation_state {
 
 ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym,
                              const Expr *Origin);
 
+/// This function provides an additional visitor that augments the bug report
+/// with information relevant to memory errors caused by the misuse of
+/// AF_InternalBuffer symbols.
+std::unique_ptr<BugReporterVisitor> getDanglingBufferBRVisitor(SymbolRef Sym);
+
 } // end namespace allocation_state
 
 } // end namespace ento
 } // end namespace clang
 
 #endif

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

 //=== DanglingInternalBufferChecker.cpp ---------------------------*- C++ -*--//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
-// This file defines a check that marks a raw pointer to a C++ standard library
-// container's inner buffer released when the object is destroyed. This
-// information can be used by MallocChecker to detect use-after-free problems.
+// This file defines a check that marks a raw pointer to a C++ container's
+// inner buffer released when the object is destroyed. This information can
+// be used by MallocChecker to detect use-after-free problems.
 //
 //===----------------------------------------------------------------------===//
 
+#include "AllocationState.h"
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #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> {
+class DanglingInternalBufferChecker
+    : public Checker<check::DeadSymbols, check::PostCall> {
   CallDescription CStrFn;
 
 public:
+  class DanglingBufferBRVisitor : public BugReporterVisitor {
+    SymbolRef PtrToBuf;
+
+  public:
+    DanglingBufferBRVisitor(SymbolRef Sym) : PtrToBuf(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;
+
+    // FIXME: Scan the map once in the visitor's constructor and do a direct
+    // lookup by region.
+    bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
+      RawPtrMapTy Map = State->get<RawPtrMap>();
+      for (const auto Entry : Map) {
+        if (Entry.second == Sym)
+          return true;
+      }
+      return false;
+    }
+  };
+
   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(), PtrToBuf) ||
+      isSymbolTracked(PrevN->getState(), PtrToBuf))
+    return nullptr;
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream OS(Buf);
+  OS << "Pointer to 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

 //RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.DanglingInternalBuffer %s -analyzer-output=text -verify
 
 namespace std {
 
 template< typename CharT >
 class basic_string {
 public:
   ~basic_string();
-  const CharT *c_str();
+  const CharT *c_str() const;
 };
 
 typedef basic_string<char> string;
 typedef basic_string<wchar_t> wstring;
 typedef basic_string<char16_t> u16string;
 typedef basic_string<char32_t> u32string;
 
 } // end namespace std
 
 void consume(const char *) {}
 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 {{Pointer to 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 {{Pointer to 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 {{Pointer to 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 {{Pointer to 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 {{Pointer to 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
 }