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

[rnkovacs]

Summary

According to the standard, pointers referring to the elements of a basic_string sequence may also 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 as well.

[xazax.hun]

Some comments, mostly nits inline.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

131–132

Nit: This return is redundant.

194–198

Nit: no need for braces here.

200

Nit: redundant return.

210

Shouldn't we also check if the function is a standard library function? Or do we assume that user functions also invalidate the strings?

211

I am not sure if we always have a Decl here, I am afraid this might return null sometimes. Please add a test case with a function pointer (received as an argument in a top level function).

220

Nit: redundant return.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2934

I think getDirectCallee might fail and return nullptr. One more reason to test function pointers :)

[NoQ]

I've just one thing to add.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

131–132

Because of how easy it is to accidentally split the state, i'm on a brink of declaring return after addTransition a good practice.

205–217

I believe that this should also go into PostCall. Symbols aren't released until some point within the call.

210

That's right, it's an important thing to check.

[xazax.hun]

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

205–217

Oh, I see. But is it guaranteed that the symbols are not garbage collected until post call? Also, the environment will always contain all the bindings for the arguments?

[rnkovacs]

Addressed comments & added two test cases for function pointers.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

211

Um, yes, I'd already seen a crash here on a project before I saw your comment. Thanks!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2934

You're right. Also, it needed a bit more effort to dig up the function pointer's name. Or should I go further and somehow find out the name of the function it points to?

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2934

Also, it needed a bit more effort to dig up the function pointer's name.

I think it should work out of the box; Call.getDecl() is smart enough to show the right decl when a pointer to a concrete function is being called on the current path.

[NoQ]

test/Analysis/inner-pointer.cpp

41–42

Without a definition for this thing, we won't be able to test much. I suggest you to take a pointer to a function directly, i.e. define a std::swap<T>(x, y) somewhere and take a &std::swap<std::string> directly and call it (hope it actually works this way).

[NoQ]

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

207–208

We need tests for operators here, due to the problem that i recently encountered in D49627: there's different numbering for arguments and parameters within in-class operators. Namely, this-argument is counted as an argument (shifting other argument indices by 1) but not as a parameter.

207–208

(i.e., tests and most likely some actual code branch)

[rnkovacs]

Fix note for function pointers & handle argument counting in member operator calls.
I also refactored the code a little, because after moving things from checkPreCall to checkPostCall, the structure was a bit confusing.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

207–208

I did the coding part, but for the tests, I'm struggling to find an example of a member operator in the STL that takes a non-const string reference as an argument. Could you perhaps help me out here?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2934

That worked, thanks!

test/Analysis/inner-pointer.cpp

41–42

I think I am missing something. This is meant to test that the checker recognizes standard library function calls taking a non-const reference to a string as an argument. At L314, func_ref is called with a string argument, and the checker seems to do all the things it should after the current patch, emitting the new kind of note and such.

I can surely add the std::swap<T>(x, y) definition too, but then the analyzer will see that the pointer was reallocated at the assignment inside the function, and place the note there. I think that would test the previous string-member-function patch more than this one.

[xazax.hun]

Small comments inline.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

149–161

Nit: maybe using FunctionDecl::parameters and range based for loop is sligtly cleaner.

167

Nit: maybe using isa + bool is cleaner here?

167–193

While I cannot recall examples in the STL the number of arguments and parameters might differ. We might have ellipsis in the parameters and this way we may pass more arguments. Since we do not have the parameter type for this case, I think it is ok to not handle it. But it might be worth to have a comment. The other case, when we have default arguments. I do not really know how the analyzer behaves with default arguments but maybe it would be worth to add a test case to ensure we do not crash on that?

[rnkovacs]

Tiny bit more re-structuring.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

149–161

Given that I am manipulating indices below, I feel that actually the plain for loop is a bit simpler here, what do you think?

[xazax.hun]

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

149–161

Indeed, I am fine with the old style loop too.

[NoQ]

Looks great!

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

167–193

The analyzer behaves pretty badly with default arguments; we don't really model them unless they are like plain integer literals. But that's not an issue here because a default parameter/argument is still both a parameter and an argument (where the argument expression takes the form of CXXDefaultArgExpr).

207–208

Mmm mmmmm right, dunno. I thought operator>> would be our main example, but it's apparently non-member for a string, even though it is defined as a member for primitive types.

I guess let's skip the test until we find an example. We did our best to work around potential future problems, that's good.

test/Analysis/inner-pointer.cpp

41–42

Mm, aha, ok, i misunderstood this test. I thought you're trying to define something like std::function and try to see if calling a function pointer wrapped into that thing works. No idea why i was thinking that, sry.

[rnkovacs]

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

167–193

Hm, it seems that passing objects of non-trivial types like std::string to a variadic function is implementation defined, and for clang 6.0, this means that it does not compile (does compile with gcc 8.1 though, example).

Added a test for the default parameter case.

[NoQ]

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

167–193

Yup, that's correct; i noticed it a few days ago too in https://reviews.llvm.org/D49443#1170831.

You can disable the warning/error with -Wno-non-pod-varargs for the purposes of testing, but you anyway won't be able to obtain the AST you'll want to test, so i don't think this requires a test.

[xazax.hun]

LGTM!

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

public:
 
   InnerPointerChecker()
       : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
         CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
         PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
         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;
-
-  /// 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;
+
+  /// 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::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)
       return true;
     return false;
   }
   return (isa<CXXDestructorCall>(Call) || Call.isCalled(AppendFn) ||
           Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
           Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
           Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
           Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
           Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
           Call.isCalled(SwapFn));
 }
 
-void InnerPointerChecker::checkPostCall(const CallEvent &Call,
+void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
+                                                 ProgramStateRef State,
+                                                 const MemRegion *MR,
                                                  CheckerContext &C) const {
-  const auto *ICall = dyn_cast<CXXInstanceCall>(&Call);
-  if (!ICall)
+  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;
+  }
+}
+
+// [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 {
+  ProgramStateRef State = C.getState();
 
+  if (const auto *ICall = dyn_cast<CXXInstanceCall>(&Call)) {
     SVal Obj = ICall->getCXXThisVal();
-  const auto *ObjRegion = dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion());
+    const auto *ObjRegion =
+        dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion());
     if (!ObjRegion)
       return;
 
-  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
-  if (TypeDecl->getName() != "basic_string")
+    QualType ObjTy = ObjRegion->getValueType();
+    if (ObjTy.isNull())
+      return;
+    if (ObjTy->getAsCXXRecordDecl()->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);
       }
       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);
+    // Check [string.require] / second point.
+    if (isInvalidatingMemberFunction(Call))
+      markPtrSymbolsReleased(Call, State, ObjRegion, C);
+
+  // Check [string.require] / first point.
+  else 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;
+      SVal Arg = FC->getArgSVal(I);
+      const auto *ArgRegion =
+          dyn_cast_or_null<TypedValueRegion>(Arg.getAsRegion());
+      if (!ArgRegion)
+        continue;
+      markPtrSymbolsReleased(Call, State, ArgRegion, C);
     }
   }
 }
 
 void InnerPointerChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                            CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
   RawPtrMapTy RPM = State->get<RawPtrMap>();
   for (const auto Entry : RPM) {
     if (!SymReaper.isLiveRegion(Entry.first)) {
void InnerPointerChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   C.addTransition(State);
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 InnerPointerChecker::InnerPointerBRVisitor::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;
 

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (isAllocated(RS, RSPrev, S)) {
             OS << "destructor";
           } else {
             OS << "'";
             const Stmt *S = RS->getStmt();
             if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
               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)) {
+              const auto *D = dyn_cast_or_null<NamedDecl>(CallE->getCalleeDecl());
+              OS << (D ? D->getNameAsString() : "unknown");
             }
             OS << "'";
           }
           Msg = OS.str();
           break;
         }
         case AF_None:
           llvm_unreachable("Unhandled allocation family!");

test/Analysis/inner-pointer.cpp

public:
   void swap(basic_string &other);
 };
 
 typedef basic_string<char> string;
 typedef basic_string<wchar_t> wstring;
 typedef basic_string<char16_t> u16string;
 typedef basic_string<char32_t> u32string;
 
+template< class T >
+void func_ref(T& a);
+
+template< class T >
+void func_const_ref(const T& a);
+
+template< class T >
+void func_value(T a);
+
 } // 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(bool cond) {
void deref_after_swap() {
   const char *c;
   std::string s1, s2;
   c = s1.data(); // expected-note {{Dangling inner pointer obtained here}}
   s1.swap(s2);   // expected-note {{Inner pointer invalidated by call to 'swap'}}
   consume(c);    // expected-warning {{Use of memory after it is freed}}
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
+void STL_func_ref() {
+  const char *c;
+  std::string s;
+  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_ptr'}}
+  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 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
 }