From c74cfc4215fc522e2be138f62c64ae3e9c0914f6 Mon Sep 17 00:00:00 2001 From: Reka Kovacs Date: Mon, 30 Jul 2018 15:43:45 +0000 Subject: [PATCH] [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 --- .../Checkers/InnerPointerChecker.cpp | 194 ++++++++++++------ .../StaticAnalyzer/Checkers/MallocChecker.cpp | 5 + clang/test/Analysis/inner-pointer.cpp | 92 ++++++++- 3 files changed, 217 insertions(+), 74 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp index ed877ab34518..29677f737f5c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp +++ b/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(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(&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(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(MR); + C.addTransition(State); + return; + } +} + +void InnerPointerChecker::checkFunctionArguments(const CallEvent &Call, + ProgramStateRef State, + CheckerContext &C) const { + if (const auto *FC = dyn_cast(&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(FC); + unsigned ArgI = isaMemberOpCall ? I+1 : I; + + SVal Arg = FC->getArgSVal(ArgI); + const auto *ArgRegion = + dyn_cast_or_null(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(&Call); - if (!ICall) - return; - - SVal Obj = ICall->getCXXThisVal(); - const auto *ObjRegion = dyn_cast_or_null(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(); - const PtrSet *SetPtr = State->get(ObjRegion); - PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet(); - assert(C.wasInlined || !Set.contains(Sym)); - Set = F.add(Set, Sym); - State = State->set(ObjRegion, Set); - C.addTransition(State); + if (const auto *ICall = dyn_cast(&Call)) { + if (isCalledOnStringObject(ICall)) { + const auto *ObjRegion = dyn_cast_or_null( + 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(); + const PtrSet *SetPtr = State->get(ObjRegion); + PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet(); + assert(C.wasInlined || !Set.contains(Sym)); + Set = F.add(Set, Sym); + + State = State->set(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(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(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; diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 8f07f413e81f..11df35a50be3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2930,6 +2930,11 @@ std::shared_ptr MallocChecker::MallocBugVisitor::VisitNode( OS << MemCallE->getMethodDecl()->getNameAsString(); } else if (const auto *OpCallE = dyn_cast(S)) { OS << OpCallE->getDirectCallee()->getNameAsString(); + } else if (const auto *CallE = dyn_cast(S)) { + auto &CEMgr = BRC.getStateManager().getCallEventManager(); + CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC); + const auto *D = dyn_cast_or_null(Call->getDecl()); + OS << (D ? D->getNameAsString() : "unknown"); } OS << "'"; } diff --git a/clang/test/Analysis/inner-pointer.cpp b/clang/test/Analysis/inner-pointer.cpp index db9bf43109b2..9e5ed380e730 100644 --- a/clang/test/Analysis/inner-pointer.cpp +++ b/clang/test/Analysis/inner-pointer.cpp @@ -38,6 +38,18 @@ typedef basic_string wstring; typedef basic_string u16string; typedef basic_string u32string; +template +void func_ref(T &a); + +template +void func_const_ref(const T &a); + +template +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; + 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}} }