Bug 1536825. Fix the interaction of ignoreTrivials and typechecks in MOZ_CAN_RUN_SCRIPT analysis. r=andi

We need to typecheck the trivials too, not just the final thing after trivials
are stripped, because casts are trivials.

Differential Revision: https://phabricator.services.mozilla.com/D24186

--HG--
extra : moz-landing-system : lando

build/clang-plugin/CanRunScriptChecker.cpp

@@ -81,16 +81,18 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
   auto LocalKnownLive = anyOf(KnownLiveSmartPtr, MozKnownLiveCall);
 
   auto InvalidArg =
-      // We want to find any expression,
-      ignoreTrivials(expr(
-          // which has a refcounted pointer type,
-          anyOf(
-            hasType(Refcounted),
-            hasType(pointsTo(Refcounted)),
-            hasType(references(Refcounted)),
-            hasType(isSmartPtrToRefCounted())
-          ),
-          // and which is not this,
+      ignoreTrivialsConditional(
+        // We want to consider things if there is anything refcounted involved,
+        // including in any of the trivials that we otherwise strip off.
+        anyOf(
+          hasType(Refcounted),
+          hasType(pointsTo(Refcounted)),
+          hasType(references(Refcounted)),
+          hasType(isSmartPtrToRefCounted())
+        ),
+        // We want to find any expression,
+        expr(
+          // which is not this,
           unless(cxxThisExpr()),
           // and which is not a stack smart ptr
           unless(KnownLiveSmartPtr),
@@ -111,6 +113,8 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
           // and which is not a default arg with value nullptr, since those are
           // always safe.
           unless(cxxDefaultArgExpr(isNullDefaultArg())),
+          // and which is not a literal nullptr
+          unless(cxxNullPtrLiteralExpr()),
           // and which is not a dereference of a parameter of the parent
           // function (including "this"),
           unless(

build/clang-plugin/CustomMatchers.h

@@ -205,6 +205,31 @@ AST_MATCHER_P(Expr, ignoreTrivials, internal::Matcher<Expr>, InnerMatcher) {
   return InnerMatcher.matches(*IgnoreTrivials(&Node), Finder, Builder);
 }
 
+// Takes two matchers: the first one is a condition; the second is a matcher to be
+// applied once we are done unwrapping trivials.  While the condition does not match
+// and we're looking at a trivial, will keep unwrapping the trivial and trying again.
+// Once the condition matches, we will go ahead and unwrap all trivials and apply the
+// inner matcher to the result.
+//
+// The expected use here is if we want to condition a match on some typecheck but
+// apply the match to only non-trivials, because there are trivials (e.g. casts) that
+// can change types.
+AST_MATCHER_P2(Expr, ignoreTrivialsConditional,
+               internal::Matcher<Expr>, Condition,
+               internal::Matcher<Expr>, InnerMatcher) {
+  const Expr *node = &Node;
+  while (true) {
+    if (Condition.matches(*node, Finder, Builder)) {
+      return InnerMatcher.matches(*IgnoreTrivials(node), Finder, Builder);
+    }
+    const Expr *newNode = MaybeSkipOneTrivial(node);
+    if (newNode == node) {
+      return false;
+    }
+    node = newNode;
+  }
+}
+
 // We can't call this "isImplicit" since it clashes with an existing matcher in
 // clang.
 AST_MATCHER(CXXConstructorDecl, isMarkedImplicit) {

build/clang-plugin/Utils.h

@@ -280,32 +280,51 @@ inline bool typeIsRefPtr(QualType Q) {
 // The method defined in clang for ignoring implicit nodes doesn't work with
 // some AST trees. To get around this, we define our own implementation of
 // IgnoreTrivials.
-inline const Stmt *IgnoreTrivials(const Stmt *s) {
-  while (true) {
+inline const Stmt *MaybeSkipOneTrivial(const Stmt *s) {
     if (!s) {
       return nullptr;
-    } else if (auto *ewc = dyn_cast<ExprWithCleanups>(s)) {
-      s = ewc->getSubExpr();
-    } else if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s)) {
-      s = mte->GetTemporaryExpr();
-    } else if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s)) {
-      s = bte->getSubExpr();
-    } else if (auto *ce = dyn_cast<CastExpr>(s)) {
-      s = ce->getSubExpr();
-    } else if (auto *pe = dyn_cast<ParenExpr>(s)) {
-      s = pe->getSubExpr();
-    } else {
-      break;
     }
+    if (auto *ewc = dyn_cast<ExprWithCleanups>(s)) {
+      return ewc->getSubExpr();
+    }
+    if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s)) {
+      return mte->GetTemporaryExpr();
+    }
+    if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s)) {
+      return bte->getSubExpr();
+    }
+    if (auto *ce = dyn_cast<CastExpr>(s)) {
+      s = ce->getSubExpr();
+    }
+    if (auto *pe = dyn_cast<ParenExpr>(s)) {
+      s = pe->getSubExpr();
+    }
+    // Not a trivial.
+    return s;
+}
+
+inline const Stmt *IgnoreTrivials(const Stmt *s) {
+  while (true) {
+    const Stmt* newS = MaybeSkipOneTrivial(s);
+    if (newS == s) {
+      return newS;
+    }
+    s = newS;
   }
 
-  return s;
+  // Unreachable
+  return nullptr;
 }
 
 inline const Expr *IgnoreTrivials(const Expr *e) {
   return cast_or_null<Expr>(IgnoreTrivials(static_cast<const Stmt *>(e)));
 }
 
+// Returns the input if the input is not a trivial.
+inline const Expr *MaybeSkipOneTrivial(const Expr *e) {
+  return cast_or_null<Expr>(MaybeSkipOneTrivial(static_cast<const Stmt *>(e)));
+}
+
 const FieldDecl *getBaseRefCntMember(QualType T);
 
 inline const FieldDecl *getBaseRefCntMember(const CXXRecordDecl *D) {

build/clang-plugin/tests/TestCanRunScript.cpp

@@ -36,6 +36,14 @@ struct RefCountedBase {
   virtual void method_test3() { // expected-note {{caller function declared here}}
     test(); // expected-error {{functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT}}
   }
+
+  MOZ_CAN_RUN_SCRIPT void method_test4() {
+    method_test();
+  }
+
+  MOZ_CAN_RUN_SCRIPT void method_test5() {
+    this->method_test();
+  }
 };
 
 MOZ_CAN_RUN_SCRIPT void testLambda() {
@@ -144,6 +152,11 @@ void test5_b() {
   test5();
 }
 
+MOZ_CAN_RUN_SCRIPT void test6() {
+  void* x = new RefCountedBase();
+  test2((RefCountedBase*)x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+}
+
 MOZ_CAN_RUN_SCRIPT void test_ref(const RefCountedBase&) {
 
 }
@@ -192,6 +205,16 @@ MOZ_CAN_RUN_SCRIPT void test_ref_8() {
   test_ref(MOZ_KnownLive(ref));
 }
 
+MOZ_CAN_RUN_SCRIPT void test_ref_9() {
+  void* x = new RefCountedBase();
+  test_ref(*(RefCountedBase*)x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+}
+
+MOZ_CAN_RUN_SCRIPT void test_ref_10() {
+  void* x = new RefCountedBase();
+  test_ref((RefCountedBase&)*x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}} expected-warning {{ISO C++ does not allow indirection on operand of type 'void *'}}
+}
+
 MOZ_CAN_RUN_SCRIPT void test_maybe() {
   mozilla::Maybe<RefCountedBase*> unsafe;
   unsafe.emplace(new RefCountedBase);