[analyzer] Add support for more invalidating functions in InnerPointerChecker.

According to the standard, pointers referring to the elements of a
`basic_string` may be invalidated if they are used as an argument to
any standard library function taking a reference to non-const
`basic_string` as an argument. This patch makes InnerPointerChecker warn
for these cases.

Differential Revision: https://reviews.llvm.org/D49656

llvm-svn: 338259

clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

@@ -91,37 +91,53 @@ public:
         ReserveFn("reserve"), ResizeFn("resize"),
         ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
 
-  /// Check whether the function called on the container object is a
-  /// member function that potentially invalidates pointers referring
-  /// to the objects's internal buffer.
-  bool mayInvalidateBuffer(const CallEvent &Call) const;
+  /// Check if the object of this member function call is a `basic_string`.
+  bool isCalledOnStringObject(const CXXInstanceCall *ICall) const;
 
-  /// 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.
+  /// Check whether the called member function potentially invalidates
+  /// pointers referring to the container object's inner buffer.
+  bool isInvalidatingMemberFunction(const CallEvent &Call) const;
+
+  /// Mark pointer symbols associated with the given memory region released
+  /// in the program state.
+  void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
+                              const MemRegion *ObjRegion,
+                              CheckerContext &C) const;
+
+  /// Standard library functions that take a non-const `basic_string` argument by
+  /// reference may invalidate its inner pointers. Check for these cases and
+  /// mark the pointers released.
+  void checkFunctionArguments(const CallEvent &Call, ProgramStateRef State,
+                              CheckerContext &C) const;
+
+  /// Record the connection between raw pointers referring to a container
+  /// object's inner buffer and the object's memory region in the program state.
+  /// Mark potentially invalidated pointers released.
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
 
-  /// Clean up the ProgramState map.
+  /// Clean up the program state map.
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 };
 
 } // end anonymous namespace
 
-// [string.require]
-//
-// "References, pointers, and iterators referring to the elements of a
-// basic_string sequence may be invalidated by the following uses of that
-// basic_string object:
-//
-// -- TODO: As an argument to any standard library function taking a reference
-// to non-const basic_string as an argument. For example, as an argument to
-// non-member functions swap(), operator>>(), and getline(), or as an argument
-// to basic_string::swap().
-//
-// -- Calling non-const member functions, except operator[], at, front, back,
-// begin, rbegin, end, and rend."
-//
-bool InnerPointerChecker::mayInvalidateBuffer(const CallEvent &Call) const {
+bool InnerPointerChecker::isCalledOnStringObject(
+        const CXXInstanceCall *ICall) const {
+  const auto *ObjRegion =
+    dyn_cast_or_null<TypedValueRegion>(ICall->getCXXThisVal().getAsRegion());
+  if (!ObjRegion)
+    return false;
+
+  QualType ObjTy = ObjRegion->getValueType();
+  if (ObjTy.isNull() ||
+      ObjTy->getAsCXXRecordDecl()->getName() != "basic_string")
+    return false;
+
+  return true;
+}
+
+bool InnerPointerChecker::isInvalidatingMemberFunction(
+        const CallEvent &Call) const {
   if (const auto *MemOpCall = dyn_cast<CXXMemberOperatorCall>(&Call)) {
     OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
     if (Opc == OO_Equal || Opc == OO_PlusEqual)
@@ -137,53 +153,104 @@ bool InnerPointerChecker::mayInvalidateBuffer(const CallEvent &Call) const {
           Call.isCalled(SwapFn));
 }
 
+void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
+                                                 ProgramStateRef State,
+                                                 const MemRegion *MR,
+                                                 CheckerContext &C) const {
+  if (const PtrSet *PS = State->get<RawPtrMap>(MR)) {
+    const Expr *Origin = Call.getOriginExpr();
+    for (const auto Symbol : *PS) {
+      // NOTE: `Origin` may be null, and will be stored so in the symbol's
+      // `RefState` in MallocChecker's `RegionState` program state map.
+      State = allocation_state::markReleased(State, Symbol, Origin);
+    }
+    State = State->remove<RawPtrMap>(MR);
+    C.addTransition(State);
+    return;
+  }
+}
+
+void InnerPointerChecker::checkFunctionArguments(const CallEvent &Call,
+                                                 ProgramStateRef State,
+                                                 CheckerContext &C) const {
+  if (const auto *FC = dyn_cast<AnyFunctionCall>(&Call)) {
+    const FunctionDecl *FD = FC->getDecl();
+    if (!FD || !FD->isInStdNamespace())
+      return;
+
+    for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+      QualType ParamTy = FD->getParamDecl(I)->getType();
+      if (!ParamTy->isReferenceType() ||
+          ParamTy->getPointeeType().isConstQualified())
+        continue;
+
+      // In case of member operator calls, `this` is counted as an
+      // argument but not as a parameter.
+      bool isaMemberOpCall = isa<CXXMemberOperatorCall>(FC);
+      unsigned ArgI = isaMemberOpCall ? I+1 : I;
+
+      SVal Arg = FC->getArgSVal(ArgI);
+      const auto *ArgRegion =
+          dyn_cast_or_null<TypedValueRegion>(Arg.getAsRegion());
+      if (!ArgRegion)
+        continue;
+
+      markPtrSymbolsReleased(Call, State, ArgRegion, C);
+    }
+  }
+}
+
+// [string.require]
+//
+// "References, pointers, and iterators referring to the elements of a
+// basic_string sequence may be invalidated by the following uses of that
+// basic_string object:
+//
+// -- As an argument to any standard library function taking a reference
+// to non-const basic_string as an argument. For example, as an argument to
+// non-member functions swap(), operator>>(), and getline(), or as an argument
+// to basic_string::swap().
+//
+// -- Calling non-const member functions, except operator[], at, front, back,
+// begin, rbegin, end, and rend."
+
 void InnerPointerChecker::checkPostCall(const CallEvent &Call,
                                         CheckerContext &C) const {
-  const auto *ICall = dyn_cast<CXXInstanceCall>(&Call);
-  if (!ICall)
-    return;
-
-  SVal Obj = ICall->getCXXThisVal();
-  const auto *ObjRegion = dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion());
-  if (!ObjRegion)
-    return;
-
-  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
-  if (TypeDecl->getName() != "basic_string")
-    return;
-
   ProgramStateRef State = C.getState();
 
-  if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-    SVal RawPtr = Call.getReturnValue();
-    if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
-      // Start tracking this raw pointer by adding it to the set of symbols
-      // associated with this container object in the program state map.
-      PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
-      const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion);
-      PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
-      assert(C.wasInlined || !Set.contains(Sym));
-      Set = F.add(Set, Sym);
-      State = State->set<RawPtrMap>(ObjRegion, Set);
-      C.addTransition(State);
+  if (const auto *ICall = dyn_cast<CXXInstanceCall>(&Call)) {
+    if (isCalledOnStringObject(ICall)) {
+      const auto *ObjRegion = dyn_cast_or_null<TypedValueRegion>(
+              ICall->getCXXThisVal().getAsRegion());
+
+      if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
+        SVal RawPtr = Call.getReturnValue();
+        if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
+          // Start tracking this raw pointer by adding it to the set of symbols
+          // associated with this container object in the program state map.
+
+          PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
+          const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion);
+          PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
+          assert(C.wasInlined || !Set.contains(Sym));
+          Set = F.add(Set, Sym);
+
+          State = State->set<RawPtrMap>(ObjRegion, Set);
+          C.addTransition(State);
+        }
+        return;
+      }
+
+      // Check [string.require] / second point.
+      if (isInvalidatingMemberFunction(Call)) {
+        markPtrSymbolsReleased(Call, State, ObjRegion, C);
+        return;
+      }
     }
-    return;
   }
 
-  if (mayInvalidateBuffer(Call)) {
-    if (const PtrSet *PS = State->get<RawPtrMap>(ObjRegion)) {
-      // Mark all pointer symbols associated with the deleted object released.
-      const Expr *Origin = Call.getOriginExpr();
-      for (const auto Symbol : *PS) {
-        // NOTE: `Origin` may be null, and will be stored so in the symbol's
-        // `RefState` in MallocChecker's `RegionState` program state map.
-        State = allocation_state::markReleased(State, Symbol, Origin);
-      }
-      State = State->remove<RawPtrMap>(ObjRegion);
-      C.addTransition(State);
-      return;
-    }
-  }
+  // Check [string.require] / first point.
+  checkFunctionArguments(Call, State, C);
 }
 
 void InnerPointerChecker::checkDeadSymbols(SymbolReaper &SymReaper,
@@ -216,7 +283,6 @@ InnerPointerChecker::InnerPointerBRVisitor::VisitNode(const ExplodedNode *N,
                                                       const ExplodedNode *PrevN,
                                                       BugReporterContext &BRC,
                                                       BugReport &BR) {
-
   if (!isSymbolTracked(N->getState(), PtrToBuf) ||
       isSymbolTracked(PrevN->getState(), PtrToBuf))
     return nullptr;

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -2930,6 +2930,11 @@ std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
               OS << MemCallE->getMethodDecl()->getNameAsString();
             } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
               OS << OpCallE->getDirectCallee()->getNameAsString();
+            } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
+              auto &CEMgr = BRC.getStateManager().getCallEventManager();
+              CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC);
+              const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl());
+              OS << (D ? D->getNameAsString() : "unknown");
             }
             OS << "'";
           }

clang/test/Analysis/inner-pointer.cpp

@@ -38,6 +38,18 @@ typedef basic_string<wchar_t> wstring;
 typedef basic_string<char16_t> u16string;
 typedef basic_string<char32_t> u32string;
 
+template <typename T>
+void func_ref(T &a);
+
+template <typename T>
+void func_const_ref(const T &a);
+
+template <typename T>
+void func_value(T a);
+
+string my_string = "default";
+void default_arg(int a = 42, string &b = my_string);
+
 } // end namespace std
 
 void consume(const char *) {}
@@ -45,6 +57,10 @@ void consume(const wchar_t *) {}
 void consume(const char16_t *) {}
 void consume(const char32_t *) {}
 
+//=--------------------------------------=//
+//     `std::string` member functions     //
+//=--------------------------------------=//
+
 void deref_after_scope_char(bool cond) {
   const char *c, *d;
   {
@@ -151,6 +167,19 @@ void multiple_symbols(bool cond) {
   }              // expected-note@-1 {{Use of memory after it is freed}}
 }
 
+void deref_after_scope_ok(bool cond) {
+  const char *c, *d;
+  std::string s;
+  {
+    c = s.c_str();
+    d = s.data();
+  }
+  if (cond)
+    consume(c); // no-warning
+  else
+    consume(d); // no-warning
+}
+
 void deref_after_equals() {
   const char *c;
   std::string s = "hello";
@@ -277,15 +306,58 @@ void deref_after_swap() {
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
-void deref_after_scope_ok(bool cond) {
-  const char *c, *d;
+//=---------------------------=//
+//     Other STL functions     //
+//=---------------------------=//
+
+void STL_func_ref() {
+  const char *c;
   std::string s;
-  {
-    c = s.c_str();
-    d = s.data();
-  }
-  if (cond)
-    consume(c); // no-warning
-  else
-    consume(d); // no-warning
+  c = s.c_str();    // expected-note {{Dangling inner pointer obtained here}}
+  std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+  consume(c);       // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  std::func_const_ref(s);
+  consume(c); // no-warning
+}
+
+void STL_func_value() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  std::func_value(s);
+  consume(c); // no-warning
+}
+
+void func_ptr_known() {
+  const char *c;
+  std::string s;
+  void (*func_ptr)(std::string &) = std::func_ref<std::string>;
+  c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+  func_ptr(s);   // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+  consume(c);    // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string &)) {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  func_ptr(s);
+  consume(c); // no-warning
+}
+
+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}}
 }