llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/91143.diff 6 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+13-8) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+6-4) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+17-19) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+38-29) - (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+14) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+18) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index b36fa95bc73f3e..f2e531837ac848 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -16,8 +16,8 @@ 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(); @@ -27,12 +27,17 @@ 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 @@ -47,7 +52,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { if (IsGetterOfRefCt && *IsGetterOfRefCt) { E = memberCall->getImplicitObjectArgument(); if (StopAtFirstRefCountedObj) { - return {E, true}; + return callback(E, true); } continue; } @@ -64,17 +69,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); @@ -91,7 +96,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 e35ea4ef05dd17..cfcc046dc9d36c 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,11 @@ 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 0f40ecc7ba3000..cf98e2d2133ab8 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 6036ad58cf253c..1d1dfa12e96df6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -188,38 +188,47 @@ 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) - 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; + 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; } } - // Parameters are guaranteed to be safe for the duration of the call - // by another checker. - if (isa<ParmVarDecl>(MaybeGuardian)) - return; - } - } + return false; + })) + return; reportBug(V); } diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index 2a4b6bb1f1063a..5d864a90f6da6b 100644 --- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -333,3 +333,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 00673e91f471ea..00cea6a3b58945 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -187,3 +187,21 @@ void bar() { } } // namespace ignore_for_if + +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 \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/91143 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
