[WebKit checkers] Treat passing of a member variable which is capable of CheckedPtr as safe.

[rniwa]

It's safe for a member function of a class or struct to call a function or allocate a local variable with a pointer or a reference to a member variable since "this" pointer, and therefore all its members, will be kept alive by its caller so recognize as such.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

It's safe for a member function of a class or struct to call a function or allocate a local variable with a pointer or a reference to a member variable since "this" pointer, and therefore all its members, will be kept alive by its caller so recognize as such.

---

Full diff: https://github.com/llvm/llvm-project/pull/142485.diff

6 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+15)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+4)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+4)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+3)
• (modified) clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp (+16)
• (modified) clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp (+16)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index f087fc8fa19fd..b6f452e814830 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -231,6 +231,21 @@ bool isConstOwnerPtrMemberExpr(const clang::Expr *E) {
   return isOwnerPtrType(T) && T.isConstQualified();
 }
 
+bool isExprToGetCheckedPtrCapableMember(const clang::Expr *E) {
+  auto *ME = dyn_cast<MemberExpr>(E);
+  if (!ME)
+    return false;
+  auto *D = ME->getMemberDecl();
+  if (!D)
+    return false;
+  auto T = D->getType();
+  auto *CXXRD = T->getAsCXXRecordDecl();
+  if (!CXXRD)
+    return false;
+  auto result = isCheckedPtrCapable(CXXRD);
+  return result && *result;
+}
+
 class EnsureFunctionVisitor
     : public ConstStmtVisitor<EnsureFunctionVisitor, bool> {
 public:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index e2cc7b976adfc..8302bbe3084c2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -69,6 +69,10 @@ bool isASafeCallArg(const clang::Expr *E);
 /// \returns true if E is a MemberExpr accessing a const smart pointer type.
 bool isConstOwnerPtrMemberExpr(const clang::Expr *E);
 
+/// \returns true if E is a MemberExpr accessing a member variable which
+/// supports CheckedPtr.
+bool isExprToGetCheckedPtrCapableMember(const clang::Expr *E);
+
 /// \returns true if E is a CXXMemberCallExpr which returns a const smart
 /// pointer type.
 class EnsureFunctionAnalysis {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 78675e55cf0ba..6bc39ab565041 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -443,6 +443,10 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
     return isRefOrCheckedPtrType(type);
   }
 
+  bool isSafeExpr(const Expr *E) const final {
+    return isExprToGetCheckedPtrCapableMember(E);
+  }
+
   const char *ptrKind() const final { return "unchecked"; }
 };
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index 4fb47703e3984..7cd86a682c0a9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -417,6 +417,9 @@ class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
   bool isSafePtrType(const QualType type) const final {
     return isRefOrCheckedPtrType(type);
   }
+  bool isSafeExpr(const Expr *E) const final {
+    return isExprToGetCheckedPtrCapableMember(E);
+  }
   const char *ptrKind() const final { return "unchecked"; }
 };
 
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
index e24b04dcd3cf9..4ad365f0f937e 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
@@ -32,6 +32,22 @@ static void baz() {
 
 } // namespace call_args_checked
 
+namespace call_args_member {
+
+void consume(CheckedObj&);
+
+struct WrapperObj {
+  CheckedObj checked;
+  CheckedObj& checkedRef;
+  void foo() {
+    consume(checked);
+    consume(checkedRef);
+    // expected-warning@-1{{Call argument is unchecked and unsafe [alpha.webkit.UncheckedCallArgsChecker]}}
+  }
+};
+
+} // namespace call_args_checked
+
 namespace call_args_default {
 
 void someFunction(RefCountableAndCheckable* = makeObj());
diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp
index 3bc75230fcf82..64ab22f74c558 100644
--- a/clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp
@@ -290,6 +290,22 @@ void foo() {
 
 } // namespace local_assignment_to_global
 
+namespace member_var {
+
+  struct WrapperObj {
+    CheckedObj checked;
+    CheckedObj& checkedRef;
+    void foo() {
+      auto *a = &checked;
+      a->method();
+      auto *b = &checkedRef;
+      // expected-warning@-1{{Local variable 'b' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
+      b->method();
+    }
+  };
+
+}
+
 namespace local_refcountable_checkable_object {
 
 RefCountableAndCheckable* provide_obj();

[ziqingluo-90]

do you need to check that the checked expr here is in fact this->checked?
In other words, is the example below valid?

struct WrapperObj {
  CheckedObj checked;

  void foo(WrapperObj *WO) {
    consume(WO->checked); // I see no warn here                               
  }
};

[rniwa]

Yes, that is valid since the caller of foo will keep WO alive for the duration of the function invocation.