From: Devin Coughlin Date: Sun, 7 Feb 2016 16:55:44 +0000 (+0000) Subject: [analyzer] Invalidate destination of std::copy() and std::copy_backward(). X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8b5ad43a09275893b6255d46bcf6c19042f96b25;p=clang [analyzer] Invalidate destination of std::copy() and std::copy_backward(). Now that the libcpp implementations of these methods has a branch that doesn't call memmove(), the analyzer needs to invalidate the destination for these methods explicitly. rdar://problem/23575656 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@260043 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/AnalysisContext.h b/include/clang/Analysis/AnalysisContext.h index 931190e43a..4324a0352e 100644 --- a/include/clang/Analysis/AnalysisContext.h +++ b/include/clang/Analysis/AnalysisContext.h @@ -201,6 +201,10 @@ public: } return static_cast(data); } + + /// Returns true if the root namespace of the given declaration is the 'std' + /// C++ namespace. + static bool isInStdNamespace(const Decl *D); private: ManagedAnalysis *&getAnalysisImpl(const void* tag); diff --git a/lib/Analysis/AnalysisDeclContext.cpp b/lib/Analysis/AnalysisDeclContext.cpp index 52c7f26136..94f753ed91 100644 --- a/lib/Analysis/AnalysisDeclContext.cpp +++ b/lib/Analysis/AnalysisDeclContext.cpp @@ -317,6 +317,21 @@ AnalysisDeclContext::getBlockInvocationContext(const LocationContext *parent, BD, ContextData); } +bool AnalysisDeclContext::isInStdNamespace(const Decl *D) { + const DeclContext *DC = D->getDeclContext()->getEnclosingNamespaceContext(); + const NamespaceDecl *ND = dyn_cast(DC); + if (!ND) + return false; + + while (const DeclContext *Parent = ND->getParent()) { + if (!isa(Parent)) + break; + ND = cast(Parent); + } + + return ND->isStdNamespace(); +} + LocationContextManager & AnalysisDeclContext::getLocationContextManager() { assert(Manager && "Cannot create LocationContexts without an AnalysisDeclContextManager!"); diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 17537445d6..5130dd610a 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -118,6 +118,10 @@ public: void evalStrsep(CheckerContext &C, const CallExpr *CE) const; + void evalStdCopy(CheckerContext &C, const CallExpr *CE) const; + void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const; + void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const; + // Utility methods std::pair static assumeZero(CheckerContext &C, @@ -1950,7 +1954,57 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const { C.addTransition(State); } +// These should probably be moved into a C++ standard library checker. +void CStringChecker::evalStdCopy(CheckerContext &C, const CallExpr *CE) const { + evalStdCopyCommon(C, CE); +} + +void CStringChecker::evalStdCopyBackward(CheckerContext &C, + const CallExpr *CE) const { + evalStdCopyCommon(C, CE); +} + +void CStringChecker::evalStdCopyCommon(CheckerContext &C, + const CallExpr *CE) const { + if (CE->getNumArgs() < 3) + return; + + ProgramStateRef State = C.getState(); + + const LocationContext *LCtx = C.getLocationContext(); + + // template + // _OutputIterator + // copy(_InputIterator __first, _InputIterator __last, + // _OutputIterator __result) + + // Invalidate the destination buffer + const Expr *Dst = CE->getArg(2); + SVal DstVal = State->getSVal(Dst, LCtx); + State = InvalidateBuffer(C, State, Dst, DstVal, /*IsSource=*/false, + /*Size=*/nullptr); + + SValBuilder &SVB = C.getSValBuilder(); + + SVal ResultVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); + State = State->BindExpr(CE, LCtx, ResultVal); + + C.addTransition(State); +} + +static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) { + IdentifierInfo *II = FD->getIdentifier(); + if (!II) + return false; + + if (!AnalysisDeclContext::isInStdNamespace(FD)) + return false; + if (II->getName().equals(Name)) + return true; + + return false; +} //===----------------------------------------------------------------------===// // The driver method, and other Checker callbacks. //===----------------------------------------------------------------------===// @@ -1999,6 +2053,10 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { evalFunction = &CStringChecker::evalBcopy; else if (C.isCLibraryFunction(FDecl, "bcmp")) evalFunction = &CStringChecker::evalMemcmp; + else if (isCPPStdLibraryFunction(FDecl, "copy")) + evalFunction = &CStringChecker::evalStdCopy; + else if (isCPPStdLibraryFunction(FDecl, "copy_backward")) + evalFunction = &CStringChecker::evalStdCopyBackward; // If the callee isn't a string function, let another checker handle it. if (!evalFunction) diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index ae5cd546f8..859a2220e0 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1540,20 +1540,6 @@ ConditionBRVisitor::VisitTrueTest(const Expr *Cond, return event; } - -// FIXME: Copied from ExprEngineCallAndReturn.cpp. -static bool isInStdNamespace(const Decl *D) { - const DeclContext *DC = D->getDeclContext()->getEnclosingNamespaceContext(); - const NamespaceDecl *ND = dyn_cast(DC); - if (!ND) - return false; - - while (const NamespaceDecl *Parent = dyn_cast(ND->getParent())) - ND = Parent; - - return ND->isStdNamespace(); -} - std::unique_ptr LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, const ExplodedNode *N, @@ -1564,7 +1550,7 @@ LikelyFalsePositiveSuppressionBRVisitor::getEndPath(BugReporterContext &BRC, AnalyzerOptions &Options = Eng.getAnalysisManager().options; const Decl *D = N->getLocationContext()->getDecl(); - if (isInStdNamespace(D)) { + if (AnalysisDeclContext::isInStdNamespace(D)) { // Skip reports within the 'std' namespace. Although these can sometimes be // the user's fault, we currently don't report them very well, and // Note that this will not help for any other data structure libraries, like diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 74cc8d2ccb..2c24dc1353 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -382,21 +382,6 @@ void ExprEngine::examineStackFrames(const Decl *D, const LocationContext *LCtx, } -static bool IsInStdNamespace(const FunctionDecl *FD) { - const DeclContext *DC = FD->getEnclosingNamespaceContext(); - const NamespaceDecl *ND = dyn_cast(DC); - if (!ND) - return false; - - while (const DeclContext *Parent = ND->getParent()) { - if (!isa(Parent)) - break; - ND = cast(Parent); - } - - return ND->isStdNamespace(); -} - // The GDM component containing the dynamic dispatch bifurcation info. When // the exact type of the receiver is not known, we want to explore both paths - // one on which we do inline it and the other one on which we don't. This is @@ -761,7 +746,7 @@ static bool mayInlineDecl(AnalysisDeclContext *CalleeADC, // Conditionally control the inlining of C++ standard library functions. if (!Opts.mayInlineCXXStandardLibrary()) if (Ctx.getSourceManager().isInSystemHeader(FD->getLocation())) - if (IsInStdNamespace(FD)) + if (AnalysisDeclContext::isInStdNamespace(FD)) return false; // Conditionally control the inlining of methods on objects that look diff --git a/test/Analysis/Inputs/system-header-simulator-cxx.h b/test/Analysis/Inputs/system-header-simulator-cxx.h index f9049c3ae9..f6b970088e 100644 --- a/test/Analysis/Inputs/system-header-simulator-cxx.h +++ b/test/Analysis/Inputs/system-header-simulator-cxx.h @@ -7,6 +7,9 @@ typedef unsigned char uint8_t; +typedef __typeof__(sizeof(int)) size_t; +void *memmove(void *s1, const void *s2, size_t n); + namespace std { template struct pair { @@ -104,11 +107,120 @@ namespace std { const _E* end() const {return __begin_ + __size_;} }; + template struct enable_if {}; + template struct enable_if {typedef _Tp type;}; + + template + struct integral_constant + { + static const _Tp value = __v; + typedef _Tp value_type; + typedef integral_constant type; + + operator value_type() const {return value;} + + value_type operator ()() const {return value;} + }; + + template + const _Tp integral_constant<_Tp, __v>::value; + + template + struct is_trivially_assignable + : integral_constant + { + }; + + typedef integral_constant true_type; + typedef integral_constant false_type; + + template struct is_const : public false_type {}; + template struct is_const<_Tp const> : public true_type {}; + + template struct is_reference : public false_type {}; + template struct is_reference<_Tp&> : public true_type {}; + + template struct is_same : public false_type {}; + template struct is_same<_Tp, _Tp> : public true_type {}; + + template ::value || is_reference<_Tp>::value > + struct __add_const {typedef _Tp type;}; + + template + struct __add_const<_Tp, false> {typedef const _Tp type;}; + + template struct add_const {typedef typename __add_const<_Tp>::type type;}; + + template struct remove_const {typedef _Tp type;}; + template struct remove_const {typedef _Tp type;}; + + template struct add_lvalue_reference {typedef _Tp& type;}; + + template struct is_trivially_copy_assignable + : public is_trivially_assignable::type, + typename add_lvalue_reference::type>::type> {}; + + template + OutputIter __copy(InputIter II, InputIter IE, OutputIter OI) { + while (II != IE) + *OI++ = *II++; + + return OI; + } + + template + inline + typename enable_if + < + is_same::type, _Up>::value && + is_trivially_copy_assignable<_Up>::value, + _Up* + >::type __copy(_Tp* __first, _Tp* __last, _Up* __result) { + size_t __n = __last - __first; + + if (__n > 0) + memmove(__result, __first, __n * sizeof(_Up)); + + return __result + __n; + } + template OutputIter copy(InputIter II, InputIter IE, OutputIter OI) { - while (II != IE) - *OI++ = *II++; - return OI; + return __copy(II, IE, OI); + } + + template + inline + _OutputIterator + __copy_backward(_BidirectionalIterator __first, _BidirectionalIterator __last, + _OutputIterator __result) + { + while (__first != __last) + *--__result = *--__last; + return __result; + } + + template + inline + typename enable_if + < + is_same::type, _Up>::value && + is_trivially_copy_assignable<_Up>::value, + _Up* + >::type __copy_backward(_Tp* __first, _Tp* __last, _Up* __result) { + size_t __n = __last - __first; + + if (__n > 0) + { + __result -= __n; + memmove(__result, __first, __n * sizeof(_Up)); + } + return __result; + } + + template + OutputIter copy_backward(InputIter II, InputIter IE, OutputIter OI) { + return __copy_backward(II, IE, OI); } struct input_iterator_tag { }; diff --git a/test/Analysis/bstring.cpp b/test/Analysis/bstring.cpp new file mode 100644 index 0000000000..0b4e7e94f0 --- /dev/null +++ b/test/Analysis/bstring.cpp @@ -0,0 +1,38 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s + +#include "Inputs/system-header-simulator-cxx.h" +#include "Inputs/system-header-simulator-for-malloc.h" + +void clang_analyzer_eval(int); + +int *testStdCopyInvalidatesBuffer(std::vector v) { + int n = v.size(); + int *buf = (int *)malloc(n * sizeof(int)); + + buf[0] = 66; + + // Call to copy should invalidate buf. + std::copy(v.begin(), v.end(), buf); + + int i = buf[0]; + + clang_analyzer_eval(i == 66); // expected-warning {{UNKNOWN}} + + return buf; +} + +int *testStdCopyBackwardInvalidatesBuffer(std::vector v) { + int n = v.size(); + int *buf = (int *)malloc(n * sizeof(int)); + + buf[0] = 66; + + // Call to copy_backward should invalidate buf. + std::copy_backward(v.begin(), v.end(), buf + n); + + int i = buf[0]; + + clang_analyzer_eval(i == 66); // expected-warning {{UNKNOWN}} + + return buf; +} diff --git a/test/Analysis/diagnostics/explicit-suppression.cpp b/test/Analysis/diagnostics/explicit-suppression.cpp index 78067573e3..67a47d02b6 100644 --- a/test/Analysis/diagnostics/explicit-suppression.cpp +++ b/test/Analysis/diagnostics/explicit-suppression.cpp @@ -9,9 +9,15 @@ void clang_analyzer_eval(bool); -void testCopyNull(int *I, int *E) { - std::copy(I, E, (int *)0); +class C { + // The virtual function is to make C not trivially copy assignable so that we call the + // variant of std::copy() that does not defer to memmove(). + virtual int f(); +}; + +void testCopyNull(C *I, C *E) { + std::copy(I, E, (C *)0); #ifndef SUPPRESSED - // expected-warning@../Inputs/system-header-simulator-cxx.h:110 {{Dereference of null pointer}} + // expected-warning@../Inputs/system-header-simulator-cxx.h:166 {{Called C++ object pointer is null}} #endif }