https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91143
>From d3a5518192fdc2ee121ef960a91a10ec1e3fffc2 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Sun, 5 May 2024 13:40:10 -0700 Subject: [PATCH] [analyzer] Support determining origins in a conditional operator in WebKit checkers. This PR adds the support for determining the origin of a pointer in a conditional operator. Because such an expression can have two distinct origins each of which needs to be visited, this PR refactors tryToFindPtrOrigin to take a callback instead of returning a pair. The callback is called for the second operand and the third operand of the conditioanl operator (i.e. E2 and E3 in E1 ? E2 : E3). Also treat nullptr and integer literal as safe pointer origins in the local variable checker. --- .../Checkers/WebKit/ASTUtils.cpp | 23 ++++-- .../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 11 ++- .../WebKit/UncountedCallArgsChecker.cpp | 36 +++++---- .../WebKit/UncountedLocalVarsChecker.cpp | 73 +++++++++++-------- .../Analysis/Checkers/WebKit/call-args.cpp | 14 ++++ .../Checkers/WebKit/uncounted-local-vars.cpp | 18 +++++ 6 files changed, 113 insertions(+), 62 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 5c49eecacc0f2..f81db0e67d835 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -16,8 +16,9 @@ namespace clang { -std::pair<const Expr *, bool> -tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { +bool tryToFindPtrOrigin( + const Expr *E, bool StopAtFirstRefCountedObj, + std::function<bool(const clang::Expr *, bool)> callback) { while (E) { if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) { E = tempExpr->getSubExpr(); @@ -31,12 +32,18 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { E = tempExpr->getSubExpr(); continue; } + if (auto *Expr = dyn_cast<ConditionalOperator>(E)) { + return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj, + callback) && + tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj, + callback); + } if (auto *cast = dyn_cast<CastExpr>(E)) { if (StopAtFirstRefCountedObj) { if (auto *ConversionFunc = dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) { if (isCtorOfRefCounted(ConversionFunc)) - return {E, true}; + return callback(E, true); } } // FIXME: This can give false "origin" that would lead to false negatives @@ -51,7 +58,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { if (IsGetterOfRefCt && *IsGetterOfRefCt) { E = memberCall->getImplicitObjectArgument(); if (StopAtFirstRefCountedObj) { - return {E, true}; + return callback(E, true); } continue; } @@ -68,17 +75,17 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { if (auto *callee = call->getDirectCallee()) { if (isCtorOfRefCounted(callee)) { if (StopAtFirstRefCountedObj) - return {E, true}; + return callback(E, true); E = call->getArg(0); continue; } if (isReturnValueRefCounted(callee)) - return {E, true}; + return callback(E, true); if (isSingleton(callee)) - return {E, true}; + return callback(E, true); if (isPtrConversion(callee)) { E = call->getArg(0); @@ -95,7 +102,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { break; } // Some other expression. - return {E, false}; + return callback(E, false); } bool isASafeCallArg(const Expr *E) { diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h index e35ea4ef05dd1..e972924e0c523 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h @@ -13,6 +13,7 @@ #include "llvm/ADT/APInt.h" #include "llvm/Support/Casting.h" +#include <functional> #include <string> #include <utility> @@ -48,10 +49,12 @@ class Expr; /// represents ref-counted object during the traversal we return relevant /// sub-expression and true. /// -/// \returns subexpression that we traversed to and if \p -/// StopAtFirstRefCountedObj is true we also return whether we stopped early. -std::pair<const clang::Expr *, bool> -tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj); +/// Calls \p callback with the subexpression that we traversed to and if \p +/// StopAtFirstRefCountedObj is true we also specify whether we stopped early. +/// Returns false if any of calls to callbacks returned false. Otherwise true. +bool tryToFindPtrOrigin( + const clang::Expr *E, bool StopAtFirstRefCountedObj, + std::function<bool(const clang::Expr *, bool)> callback); /// For \p E referring to a ref-countable/-counted pointer/reference we return /// whether it's a safe call argument. Examples: function parameter or diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 9a178a690ff24..704c082a4d1d6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -126,25 +126,23 @@ class UncountedCallArgsChecker } bool isPtrOriginSafe(const Expr *Arg) const { - std::pair<const clang::Expr *, bool> ArgOrigin = - tryToFindPtrOrigin(Arg, true); - - // Temporary ref-counted object created as part of the call argument - // would outlive the call. - if (ArgOrigin.second) - return true; - - if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) { - // foo(nullptr) - return true; - } - if (isa<IntegerLiteral>(ArgOrigin.first)) { - // FIXME: Check the value. - // foo(NULL) - return true; - } - - return isASafeCallArg(ArgOrigin.first); + return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true, + [](const clang::Expr *ArgOrigin, bool IsSafe) { + if (IsSafe) + return true; + if (isa<CXXNullPtrLiteralExpr>(ArgOrigin)) { + // foo(nullptr) + return true; + } + if (isa<IntegerLiteral>(ArgOrigin)) { + // FIXME: Check the value. + // foo(NULL) + return true; + } + if (isASafeCallArg(ArgOrigin)) + return true; + return false; + }); } bool shouldSkipCall(const CallExpr *CE) const { diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 98a73810b7afc..0d9710a5e2d83 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -188,39 +188,50 @@ class UncountedLocalVarsChecker if (!InitExpr) return; // FIXME: later on we might warn on uninitialized vars too - const clang::Expr *const InitArgOrigin = - tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false) - .first; - if (!InitArgOrigin) + if (tryToFindPtrOrigin( + InitExpr, /*StopAtFirstRefCountedObj=*/false, + [&](const clang::Expr *InitArgOrigin, bool IsSafe) { + if (!InitArgOrigin) + return true; + + if (isa<CXXThisExpr>(InitArgOrigin)) + return true; + + if (isa<CXXNullPtrLiteralExpr>(InitArgOrigin)) + return true; + + if (isa<IntegerLiteral>(InitArgOrigin)) + return true; + + if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) { + if (auto *MaybeGuardian = + dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) { + const auto *MaybeGuardianArgType = + MaybeGuardian->getType().getTypePtr(); + if (MaybeGuardianArgType) { + const CXXRecordDecl *const MaybeGuardianArgCXXRecord = + MaybeGuardianArgType->getAsCXXRecordDecl(); + if (MaybeGuardianArgCXXRecord) { + if (MaybeGuardian->isLocalVarDecl() && + (isRefCounted(MaybeGuardianArgCXXRecord) || + isRefcountedStringsHack(MaybeGuardian)) && + isGuardedScopeEmbeddedInGuardianScope( + V, MaybeGuardian)) + return true; + } + } + + // Parameters are guaranteed to be safe for the duration of + // the call by another checker. + if (isa<ParmVarDecl>(MaybeGuardian)) + return true; + } + } + + return false; + })) return; - if (isa<CXXThisExpr>(InitArgOrigin)) - return; - - if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) { - if (auto *MaybeGuardian = - dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) { - const auto *MaybeGuardianArgType = - MaybeGuardian->getType().getTypePtr(); - if (MaybeGuardianArgType) { - const CXXRecordDecl *const MaybeGuardianArgCXXRecord = - MaybeGuardianArgType->getAsCXXRecordDecl(); - if (MaybeGuardianArgCXXRecord) { - if (MaybeGuardian->isLocalVarDecl() && - (isRefCounted(MaybeGuardianArgCXXRecord) || - isRefcountedStringsHack(MaybeGuardian)) && - isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) - return; - } - } - - // Parameters are guaranteed to be safe for the duration of the call - // by another checker. - if (isa<ParmVarDecl>(MaybeGuardian)) - return; - } - } - reportBug(V); } } diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index 45d900d4ba880..e1bee8a23a250 100644 --- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -344,3 +344,17 @@ namespace cxx_member_operator_call { // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} } } + +namespace call_with_ptr_on_ref { + Ref<RefCountable> provideProtected(); + void bar(RefCountable* bad); + bool baz(); + void foo(bool v) { + bar(v ? nullptr : provideProtected().ptr()); + bar(baz() ? provideProtected().ptr() : nullptr); + bar(v ? provide() : provideProtected().ptr()); + // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} + bar(v ? provideProtected().ptr() : provide()); + // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} + } +} diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 8da1dc557a5a3..632a82eb0d8d1 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -198,3 +198,21 @@ void system_header() { } } // ignore_system_headers + +namespace conditional_op { +RefCountable *provide_ref_ctnbl(); +bool bar(); + +void foo() { + RefCountable *a = bar() ? nullptr : provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + RefPtr<RefCountable> b = provide_ref_ctnbl(); + { + RefCountable* c = bar() ? nullptr : b.get(); + c->method(); + RefCountable* d = bar() ? b.get() : nullptr; + d->method(); + } +} + +} // namespace conditional_op _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits