Bug 1536719. Fix handling of member method calls in the MOZ_CAN_RUN_SCRIPT analysis. r=andi

The old code for member method calls did the following:

 1) Find the member method calls.
 2) Look at their "this" expression.
 3) If the "this" is an operator call, check for any of the arguments of the
    operator call being invalid.
 4) Otherwise (if not an operator call) check for the "this" value being
    invalid.

This wasn't right, because the "is invalid" check checks the type and only
considers refcounted things.  So if the code looked something like
"foo[i]->call_method()", we would look at the types of "foo" and "i" and
determine that none of those are refcounted types so there is nothing invalid
here (since "foo" is some sort of array type and "i" is an integer).  The new
setup just checks whether the "this" value is invalid, which does the type
check on the "this" value itself; in the "foo[i]->call_method()" case on
"foo[i]".  We then adjust the exclusions in InvalidArg to consider operator->
on known-live things valid, to allow the thing that we were really trying to
accomplish with the "check for an operator call" bits:
"stackRefPtr->some_method()".

The test coverage being added for the made-up TArray type is meant to catch
things like the geolocation issue that was being hidden by the buggy behavior.
I'm not using nsTArray itself because some header included by nsTArray.h
tries to define operator new/delete bits inline and that triggers warnings that
then cause a clang-plugin test failure, because they're unexpected.

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

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

build/clang-plugin/CanRunScriptChecker.cpp

@@ -67,7 +67,14 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
   //    reference until "this" gets destroyed.
   auto KnownLiveSmartPtr = anyOf(StackSmartPtr, ConstMemberOfThisSmartPtr);
   auto MozKnownLiveCall =
-    callExpr(callee(functionDecl(hasName("MOZ_KnownLive"))));
+    ignoreTrivials(callExpr(callee(functionDecl(hasName("MOZ_KnownLive")))));
+
+  // A matcher that matches some cases that are known live due to local
+  // information (as in, not relying on the rest of this analysius to guarantee
+  // their liveness).  There's some conceptual overlap with the set of unless()
+  // clauses in InvalidArg here, but for our purposes this limited set of cases
+  // is fine.
+  auto LocalKnownLive = anyOf(KnownLiveSmartPtr, MozKnownLiveCall);
 
   auto InvalidArg =
       // We want to find any expression,
@@ -85,14 +92,14 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
           unless(KnownLiveSmartPtr),
           // and which is not a method call on a stack smart ptr,
           unless(cxxMemberCallExpr(on(KnownLiveSmartPtr))),
-          // and which is not calling operator* on a stack smart ptr.
+          // and which is not calling operator* or operator-> on a thing that is
+          // already known to be live.
           unless(
-            allOf(
-              cxxOperatorCallExpr(hasOverloadedOperatorName("*")),
-              callExpr(allOf(
-                hasAnyArgument(KnownLiveSmartPtr),
-                argumentCountIs(1)
-              ))
+            cxxOperatorCallExpr(
+              anyOf(hasOverloadedOperatorName("*"),
+                    hasOverloadedOperatorName("->")),
+              hasAnyArgument(LocalKnownLive),
+              argumentCountIs(1)
             )
           ),
           // and which is not a parameter of the parent function,
@@ -141,6 +148,9 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
           ),
           expr().bind("invalidArg")));
 
+  // A matcher which will mark the first invalid argument it finds invalid, but
+  // will always match, even if it finds no invalid arguments, so it doesn't
+  // preclude other matchers from running and maybe finding invalid args.
   auto OptionalInvalidExplicitArg = anyOf(
       // We want to find any argument which is invalid.
       hasAnyArgument(InvalidArg),
@@ -159,15 +169,11 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
               cxxMemberCallExpr(
                   // which optionally has an invalid arg,
                   OptionalInvalidExplicitArg,
-                  // or which optionally has an invalid implicit this argument,
+                  // or which optionally has an invalid this argument,
                   anyOf(
-                      // which derefs into an invalid arg,
-                      on(cxxOperatorCallExpr(
-                          anyOf(hasAnyArgument(InvalidArg), anything()))),
-                      // or is an invalid arg.
-                      on(InvalidArg),
-
-                      anything()),
+                    on(InvalidArg),
+                    anything()
+                  ),
                   expr().bind("callExpr")),
               // or a regular call expression,
               callExpr(

build/clang-plugin/tests/TestCanRunScript.cpp

@@ -193,10 +193,9 @@ MOZ_CAN_RUN_SCRIPT void test_ref_8() {
 }
 
 MOZ_CAN_RUN_SCRIPT void test_maybe() {
-  // FIXME(emilio): This should generate an error, but it's pre-existing!
   mozilla::Maybe<RefCountedBase*> unsafe;
   unsafe.emplace(new RefCountedBase);
-  (*unsafe)->method_test();
+  (*unsafe)->method_test(); // 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_maybe_2() {
@@ -347,3 +346,32 @@ struct DisallowConstNonRefPtrMemberArgs {
     test2(mRefCounted); // 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)}}
   }
 };
+
+template<typename T>
+struct TArray {
+  TArray() {
+    mArray[0] = new RefCountedBase();
+  }
+  T& operator[](unsigned int index) { return mArray[index]; }
+  T mArray[1];
+};
+
+struct DisallowRawTArrayElement {
+  TArray<RefCountedBase*> mArray;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    mArray[0]->method_test(); // 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 bar() {
+    test2(mArray[0]); // 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)}}
+  }
+};
+
+struct DisallowRefPtrTArrayElement {
+  TArray<RefPtr<RefCountedBase>> mArray;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    mArray[0]->method_test(); // 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 bar() {
+    test2(mArray[0]); // 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)}}
+  }
+};

dom/geolocation/nsGeolocation.cpp

@@ -624,8 +624,12 @@ nsGeolocationService::Update(nsIDOMGeoPosition* aSomewhere) {
 
 NS_IMETHODIMP
 nsGeolocationService::NotifyError(uint16_t aErrorCode) {
-  for (uint32_t i = 0; i < mGeolocators.Length(); i++) {
-    mGeolocators[i]->NotifyError(aErrorCode);
+  // nsTArray doesn't have a constructors that takes a different-type TArray.
+  nsTArray<RefPtr<Geolocation>> geolocators;
+  geolocators.AppendElements(mGeolocators);
+  for (uint32_t i = 0; i < geolocators.Length(); i++) {
+    // MOZ_KnownLive because the stack array above keeps it alive.
+    MOZ_KnownLive(geolocators[i])->NotifyError(aErrorCode);
   }
   return NS_OK;
 }

dom/u2f/U2F.cpp

@@ -516,13 +516,15 @@ void U2F::RejectTransaction(const nsresult& aError) {
   if (transaction.HasRegisterCallback()) {
     RegisterResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(code));
-    ExecuteCallback(response, transaction.GetRegisterCallback());
+    // MOZ_KnownLive because "transaction" lives on the stack.
+    ExecuteCallback(response, MOZ_KnownLive(transaction.GetRegisterCallback()));
   }
 
   if (transaction.HasSignCallback()) {
     SignResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(code));
-    ExecuteCallback(response, transaction.GetSignCallback());
+    // MOZ_KnownLive because "transaction" lives on the stack.
+    ExecuteCallback(response, MOZ_KnownLive(transaction.GetSignCallback()));
   }
 }
 

xpcom/threads/nsProxyRelease.h

@@ -216,7 +216,7 @@ inline NS_HIDDEN_(void)
  * an nsMainThreadPtrHandle<T> rather than an nsCOMPtr<T>.
  */
 template <class T>
-class nsMainThreadPtrHolder final {
+class MOZ_IS_SMARTPTR_TO_REFCOUNTED nsMainThreadPtrHolder final {
  public:
   // We can only acquire a pointer on the main thread. We to fail fast for
   // threading bugs, so by default we assert if our pointer is used or acquired
@@ -310,7 +310,7 @@ class nsMainThreadPtrHolder final {
 };
 
 template <class T>
-class nsMainThreadPtrHandle {
+class MOZ_IS_SMARTPTR_TO_REFCOUNTED nsMainThreadPtrHandle {
   RefPtr<nsMainThreadPtrHolder<T>> mPtr;
 
  public: