From 50fa64d4411a42e0b4f373a84d8d4f5cbf339ea3 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 17 May 2013 02:16:49 +0000 Subject: [PATCH] [analyzer] Don't inline ~shared_ptr. The analyzer can't see the reference count for shared_ptr, so it doesn't know whether a given destruction is going to delete the referenced object. This leads to spurious leak and use-after-free warnings. For now, just ban destructors named '~shared_ptr', which catches std::shared_ptr, std::tr1::shared_ptr, and boost::shared_ptr. PR15987 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@182071 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../StaticAnalyzer/Core/AnalyzerOptions.h | 13 +++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp | 6 ++ .../Core/ExprEngineCallAndReturn.cpp | 24 +++++ test/Analysis/NewDelete-checker-test.cpp | 95 +++++++++++++++++++ test/Analysis/analyzer-config.cpp | 3 +- 5 files changed, 140 insertions(+), 1 deletion(-) diff --git a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h index 2d7e0b4177..2ef802d607 100644 --- a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h +++ b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h @@ -201,6 +201,9 @@ private: /// \sa mayInlineCXXContainerCtorsAndDtors Optional InlineCXXContainerCtorsAndDtors; + /// \sa mayInlineCXXSharedPtrDtor + Optional InlineCXXSharedPtrDtor; + /// \sa mayInlineObjCMethod Optional ObjCInliningMode; @@ -294,6 +297,16 @@ public: /// accepts the values "true" and "false". bool mayInlineCXXContainerCtorsAndDtors(); + /// Returns whether or not the destructor of C++ 'shared_ptr' may be + /// considered for inlining. + /// + /// This covers std::shared_ptr, std::tr1::shared_ptr, and boost::shared_ptr, + /// and indeed any destructor named "~shared_ptr". + /// + /// This is controlled by the 'c++-shared_ptr-inlining' config option, which + /// accepts the values "true" and "false". + bool mayInlineCXXSharedPtrDtor(); + /// Returns whether or not paths that go through null returns should be /// suppressed. /// diff --git a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp index 93d4fcdac5..9dcf58babd 100644 --- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp +++ b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp @@ -140,6 +140,12 @@ bool AnalyzerOptions::mayInlineCXXContainerCtorsAndDtors() { /*Default=*/false); } +bool AnalyzerOptions::mayInlineCXXSharedPtrDtor() { + return getBooleanOption(InlineCXXSharedPtrDtor, + "c++-shared_ptr-inlining", + /*Default=*/false); +} + bool AnalyzerOptions::mayInlineObjCMethod() { return getBooleanOption(ObjCInliningMode, diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 06570a4b4a..b76649e6fc 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -717,6 +717,21 @@ static bool isContainerCtorOrDtor(const ASTContext &Ctx, return isContainerClass(Ctx, RD); } +/// Returns true if the given function is the destructor of a class named +/// "shared_ptr". +static bool isCXXSharedPtrDtor(const FunctionDecl *FD) { + const CXXDestructorDecl *Dtor = dyn_cast(FD); + if (!Dtor) + return false; + + const CXXRecordDecl *RD = Dtor->getParent(); + if (const IdentifierInfo *II = RD->getDeclName().getAsIdentifierInfo()) + if (II->isStr("shared_ptr")) + return true; + + return false; +} + /// Returns true if the function in \p CalleeADC may be inlined in general. /// /// This checks static properties of the function, such as its signature and @@ -749,6 +764,15 @@ static bool mayInlineDecl(const CallEvent &Call, AnalysisDeclContext *CalleeADC, if (!Ctx.getSourceManager().isFromMainFile(FD->getLocation())) if (isContainerCtorOrDtor(Ctx, FD)) return false; + + // Conditionally control the inlining of the destructor of C++ shared_ptr. + // We don't currently do a good job modeling shared_ptr because we can't + // see the reference count, so treating as opaque is probably the best + // idea. + if (!Opts.mayInlineCXXSharedPtrDtor()) + if (isCXXSharedPtrDtor(FD)) + return false; + } } diff --git a/test/Analysis/NewDelete-checker-test.cpp b/test/Analysis/NewDelete-checker-test.cpp index 5d134bc36e..83796357b4 100644 --- a/test/Analysis/NewDelete-checker-test.cpp +++ b/test/Analysis/NewDelete-checker-test.cpp @@ -206,3 +206,98 @@ void testConstEscapePlacementNew() { void *y = new (x) int; escapeVoidPtr(y); } // no-warning + + +namespace reference_count { + class control_block { + unsigned count; + public: + control_block() : count(0) {} + void retain() { ++count; } + int release() { return --count; } + }; + + template + class shared_ptr { + T *p; + control_block *control; + + public: + shared_ptr() : p(0), control(0) {} + explicit shared_ptr(T *p) : p(p), control(new control_block) { + control->retain(); + } + shared_ptr(shared_ptr &other) : p(other.p), control(other.control) { + if (control) + control->retain(); + } + ~shared_ptr() { + if (control && control->release() == 0) { + delete p; + delete control; + } + }; + + T &operator *() { + return *p; + }; + + void swap(shared_ptr &other) { + T *tmp = p; + p = other.p; + other.p = tmp; + + control_block *ctrlTmp = control; + control = other.control; + other.control = ctrlTmp; + } + }; + + void testSingle() { + shared_ptr a(new int); + *a = 1; + } + + void testDouble() { + shared_ptr a(new int); + shared_ptr b = a; + *a = 1; + } + + void testInvalidated() { + shared_ptr a(new int); + shared_ptr b = a; + *a = 1; + + extern void use(shared_ptr &); + use(b); + } + + void testNestedScope() { + shared_ptr a(new int); + { + shared_ptr b = a; + } + *a = 1; + } + + void testSwap() { + shared_ptr a(new int); + shared_ptr b; + shared_ptr c = a; + shared_ptr(c).swap(b); + } + + void testUseAfterFree() { + int *p = new int; + { + shared_ptr a(p); + shared_ptr b = a; + } + + // FIXME: We should get a warning here, but we don't because we've + // conservatively modeled ~shared_ptr. + *p = 1; + } +} + diff --git a/test/Analysis/analyzer-config.cpp b/test/Analysis/analyzer-config.cpp index bf18a5eb3e..521344a511 100644 --- a/test/Analysis/analyzer-config.cpp +++ b/test/Analysis/analyzer-config.cpp @@ -13,6 +13,7 @@ public: // CHECK: [config] // CHECK-NEXT: c++-container-inlining = false // CHECK-NEXT: c++-inlining = destructors +// CHECK-NEXT: c++-shared_ptr-inlining = false // CHECK-NEXT: c++-stdlib-inlining = true // CHECK-NEXT: c++-template-inlining = true // CHECK-NEXT: cfg-conditional-static-initializers = true @@ -28,4 +29,4 @@ public: // CHECK-NEXT: mode = deep // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: [stats] -// CHECK-NEXT: num-entries = 16 +// CHECK-NEXT: num-entries = 17 -- 2.40.0