llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> This checker has a notion of a guardian variable which is a variable and keeps the object pointed to by a raw pointer / reference in an inner scope alive long enough to "guard" it from use-after-free. But such a guardian variable fails to flawed to keep the object alive if it ever gets mutated within the scope of a raw pointer / reference. This PR fixes this bug by introducing a new AST visitor class, GuardianVisitor, which traverses the compound statements of a guarded variable (raw pointer / reference) and looks for any operator=, move constructor, or calls to "swap", "leakRef", or "releaseNonNull" functions. --- Full diff: https://github.com/llvm/llvm-project/pull/113859.diff 3 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+67-5) - (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+33-1) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+59) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 5cdf047738abcb..5f5fb6e2bf1f87 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -48,6 +48,65 @@ bool isRefcountedStringsHack(const VarDecl *V) { return false; } +struct GuardianVisitor : public RecursiveASTVisitor<GuardianVisitor> { + using Base = RecursiveASTVisitor<GuardianVisitor>; + + const VarDecl *Guardian { nullptr }; + +public: + explicit GuardianVisitor(const VarDecl *Guardian) + : Guardian(Guardian) { + assert(Guardian); + } + + bool VisitBinaryOperator(const BinaryOperator *BO) { + if (BO->isAssignmentOp()) { + if (auto *VarRef = dyn_cast<DeclRefExpr>(BO->getLHS())) { + if (VarRef->getDecl() == Guardian) + return false; + } + } + return true; + } + + bool VisitCXXConstructExpr(const CXXConstructExpr* CE) { + if (auto *Ctor = CE->getConstructor()) { + if (Ctor->isMoveConstructor() && CE->getNumArgs() == 1) { + auto *Arg = CE->getArg(0)->IgnoreParenCasts(); + if (auto *VarRef = dyn_cast<DeclRefExpr>(Arg)) { + if (VarRef->getDecl() == Guardian) + return false; + } + } + } + return true; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* MCE) { + auto MethodName = safeGetName(MCE->getMethodDecl()); + if (MethodName == "swap" || MethodName == "leakRef" || + MethodName == "releaseNonNull") { + auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts(); + if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) { + if (VarRef->getDecl() == Guardian) + return false; + } + } + return true; + } + + bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* OCE) { + if (OCE->isAssignmentOp() && OCE->getNumArgs() == 2) { + auto *ThisArg = OCE->getArg(0)->IgnoreParenCasts(); + if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) { + if (VarRef->getDecl() == Guardian) + return false; + } + } + return true; + } +}; + bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded, const VarDecl *MaybeGuardian) { assert(Guarded); @@ -81,7 +140,7 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded, // We need to skip the first CompoundStmt to avoid situation when guardian is // defined in the same scope as guarded variable. - bool HaveSkippedFirstCompoundStmt = false; + const CompoundStmt *FirstCompondStmt = nullptr; for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded); !guardedVarAncestors.empty(); guardedVarAncestors = ctx.getParents( @@ -90,12 +149,15 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded, ) { for (auto &guardedVarAncestor : guardedVarAncestors) { if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) { - if (!HaveSkippedFirstCompoundStmt) { - HaveSkippedFirstCompoundStmt = true; + if (!FirstCompondStmt) { + FirstCompondStmt = CStmtAncestor; continue; } - if (CStmtAncestor == guardiansClosestCompStmtAncestor) - return true; + if (CStmtAncestor == guardiansClosestCompStmtAncestor) { + GuardianVisitor guardianVisitor(MaybeGuardian); + auto *GuardedScope = const_cast<CompoundStmt *>(FirstCompondStmt); + return guardianVisitor.TraverseCompoundStmt(GuardedScope); + } } } } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index 8d8a90f0afae0e..82c79c97a83de6 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -49,7 +49,23 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra Ref() : t{} {}; Ref(T &t) : t(&RefDerefTraits::ref(t)) { } Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { } + Ref(Ref&& o) : t(o.leakRef()) { } ~Ref() { RefDerefTraits::derefIfNotNull(PtrTraits::exchange(t, nullptr)); } + Ref& operator=(T &t) { + Ref o(t); + swap(o); + return *this; + } + Ref& operator=(Ref &&o) { + Ref m(o); + swap(m); + return *this; + } + void swap(Ref& o) { + typename PtrTraits::StorageType tmp = t; + t = o.t; + o.t = tmp; + } T &get() { return *PtrTraits::unwrap(t); } T *ptr() { return PtrTraits::unwrap(t); } T *operator->() { return PtrTraits::unwrap(t); } @@ -74,11 +90,27 @@ template <typename T> struct RefPtr { if (t) t->deref(); } + Ref<T> releaseNonNull() { + Ref<T> tmp(*t); + if (t) + t->deref(); + t = nullptr; + return tmp; + } + void swap(RefPtr& o) { + T* tmp = t; + t = o.t; + o.t = tmp; + } T *get() { return t; } T *operator->() { return t; } const T *operator->() const { return t; } T &operator*() { return *t; } - RefPtr &operator=(T *) { return *this; } + RefPtr &operator=(T *t) { + RefPtr o(t); + swap(o); + return *this; + } operator bool() const { return t; } }; diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 1c0df42cdda663..7028d7bb0ba160 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -83,6 +83,65 @@ void foo7(RefCountable* obj) { bar.obj->method(); } +void foo8(RefCountable* obj) { + RefPtr<RefCountable> foo; + { + RefCountable *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + foo = nullptr; + bar->method(); + } + RefPtr<RefCountable> baz; + { + RefCountable *bar = baz.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + baz = obj; + bar->method(); + } + foo = nullptr; + { + RefCountable *bar = foo.get(); + // No warning. It's okay to mutate RefPtr in an outer scope. + bar->method(); + } + foo = obj; + { + RefCountable *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + foo.releaseNonNull(); + bar->method(); + } +} + +void foo9(RefCountable& o) { + Ref<RefCountable> guardian(o); + { + RefCountable &bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + guardian = o; // We don't detect that we're setting it to the same value. + bar.method(); + } + { + RefCountable *bar = guardian.ptr(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + Ref<RefCountable> other(*bar); // We don't detect other has the same value as guardian. + guardian.swap(other); + bar->method(); + } + { + RefCountable *bar = guardian.ptr(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + Ref<RefCountable> other(static_cast<Ref<RefCountable>&&>(guardian)); + bar->method(); + } + { + RefCountable *bar = guardian.ptr(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + guardian.leakRef(); + bar->method(); + } +} + } // namespace guardian_scopes namespace auto_keyword { `````````` </details> https://github.com/llvm/llvm-project/pull/113859 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
