Bug 1533617 part 5. Disallow virtual MOZ_CAN_RUN_SCRIPT methods overriding a non-MOZ_CAN_RUN_SCRIPT superclass method. r=andi
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 11 Mar 2019 14:20:27 +0000
changeset 524386 94ae30387e57697e0044cad778459f751319fe11
parent 524385 005ab4b76f8c43b8a9a1dd792786b65d4dd4d05a
child 524387 ea2b6fafbb08e3875b7cd64073539ba46ae10b10
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersandi
bugs1533617
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1533617 part 5. Disallow virtual MOZ_CAN_RUN_SCRIPT methods overriding a non-MOZ_CAN_RUN_SCRIPT superclass method. r=andi This way if a caller calls a method that has a MOZ_CAN_RUN_SCRIPT override, it can detect that it's possibly calling a MOZ_CAN_RUN_SCRIPT thing without having to know about the override. Differential Revision: https://phabricator.services.mozilla.com/D22839
build/clang-plugin/CanRunScriptChecker.cpp
build/clang-plugin/tests/TestCanRunScript.cpp
--- a/build/clang-plugin/CanRunScriptChecker.cpp
+++ b/build/clang-plugin/CanRunScriptChecker.cpp
@@ -83,76 +83,80 @@ void CanRunScriptChecker::registerMatche
 }
 
 void CanRunScriptChecker::onStartOfTranslationUnit() {
   IsFuncSetBuilt = false;
   CanRunScriptFuncs.clear();
 }
 
 namespace {
-/// This class is a callback used internally to match function declarations
-/// with the MOZ_CAN_RUN_SCRIPT annotation, adding these functions and all
-/// the methods they override to the can-run-script function set.
+/// This class is a callback used internally to match function declarations with
+/// the MOZ_CAN_RUN_SCRIPT annotation, adding these functions to the
+/// can-run-script function set and making sure the functions they override (if
+/// any) also have the annotation.
 class FuncSetCallback : public MatchFinder::MatchCallback {
 public:
-  FuncSetCallback(std::unordered_set<const FunctionDecl *> &FuncSet)
-      : CanRunScriptFuncs(FuncSet) {}
+  FuncSetCallback(CanRunScriptChecker& Checker,
+                  std::unordered_set<const FunctionDecl *> &FuncSet)
+      : CanRunScriptFuncs(FuncSet),
+        Checker(Checker) {}
 
   void run(const MatchFinder::MatchResult &Result) override;
 
 private:
-  /// This method recursively adds all the methods overriden by the given
-  /// paremeter.
-  void addAllOverriddenMethodsRecursively(const CXXMethodDecl *Method);
+  /// This method checks the methods overriden by the given parameter.
+  void checkOverriddenMethods(const CXXMethodDecl *Method);
 
   std::unordered_set<const FunctionDecl *> &CanRunScriptFuncs;
+  CanRunScriptChecker &Checker;
 };
 
 void FuncSetCallback::run(const MatchFinder::MatchResult &Result) {
   const FunctionDecl *Func;
   if (auto *Lambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda")) {
     Func = Lambda->getCallOperator();
     if (!Func || !hasCustomAttribute<moz_can_run_script>(Func))
       return;
   } else {
     Func = Result.Nodes.getNodeAs<FunctionDecl>("canRunScriptFunction");
   }
 
   CanRunScriptFuncs.insert(Func);
 
   // If this is a method, we check the methods it overrides.
   if (auto *Method = dyn_cast<CXXMethodDecl>(Func)) {
-    addAllOverriddenMethodsRecursively(Method);
+    checkOverriddenMethods(Method);
   }
 }
 
-void FuncSetCallback::addAllOverriddenMethodsRecursively(
-    const CXXMethodDecl *Method) {
+void FuncSetCallback::checkOverriddenMethods(const CXXMethodDecl *Method) {
   for (auto OverriddenMethod : Method->overridden_methods()) {
-    CanRunScriptFuncs.insert(OverriddenMethod);
+    if (!hasCustomAttribute<moz_can_run_script>(OverriddenMethod)) {
+      const char *ErrorNonCanRunScriptOverridden =
+          "functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions "
+          "that are not marked MOZ_CAN_RUN_SCRIPT";
+      const char* NoteNonCanRunScriptOverridden =
+          "overridden function declared here";
 
-    // If this is not the definition, we also add the definition (if it
-    // exists) to the set.
-    if (!OverriddenMethod->isThisDeclarationADefinition()) {
-      if (auto Def = OverriddenMethod->getDefinition()) {
-        CanRunScriptFuncs.insert(Def);
-      }
+      Checker.diag(Method->getLocation(), ErrorNonCanRunScriptOverridden,
+                   DiagnosticIDs::Error);
+      Checker.diag(OverriddenMethod->getLocation(),
+                   NoteNonCanRunScriptOverridden,
+                   DiagnosticIDs::Note);
     }
-
-    addAllOverriddenMethodsRecursively(OverriddenMethod);
   }
 }
 } // namespace
 
 void CanRunScriptChecker::buildFuncSet(ASTContext *Context) {
   // We create a match finder.
   MatchFinder Finder;
   // We create the callback which will be called when we find a function with
   // a MOZ_CAN_RUN_SCRIPT annotation.
-  FuncSetCallback Callback(CanRunScriptFuncs);
+  FuncSetCallback Callback(*this, CanRunScriptFuncs);
   // We add the matcher to the finder, linking it to our callback.
   Finder.addMatcher(
       functionDecl(hasCanRunScriptAnnotation()).bind("canRunScriptFunction"),
       &Callback);
   Finder.addMatcher(
       lambdaExpr().bind("lambda"),
       &Callback);
   // We start the analysis, given the ASTContext our main checker is in.
--- a/build/clang-plugin/tests/TestCanRunScript.cpp
+++ b/build/clang-plugin/tests/TestCanRunScript.cpp
@@ -28,18 +28,18 @@ struct RefCountedBase {
   MOZ_CAN_RUN_SCRIPT void method_test() {
     test();
   }
 
   MOZ_CAN_RUN_SCRIPT void method_test2() {
     test2(this);
   }
 
-  virtual void method_test3() {
-    test();
+  virtual void method_test3() { // expected-note {{parent 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 testLambda() {
   auto doIt = []() MOZ_CAN_RUN_SCRIPT {
     test();
   };
 
@@ -87,28 +87,28 @@ MOZ_CAN_RUN_SCRIPT void test2_parent7() 
 
 MOZ_CAN_RUN_SCRIPT void test3(int* param) {}
 
 MOZ_CAN_RUN_SCRIPT void test3_parent() {
   test3(new int);
 }
 
 struct RefCountedChild : public RefCountedBase {
-  virtual void method_test3() override;
+  virtual void method_test3() override; // expected-note {{overridden function declared here}} expected-note {{overridden function declared here}}
 };
 
-void RefCountedChild::method_test3() {
-  test();
+void RefCountedChild::method_test3() { // expected-note {{parent 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}}
 }
 
 struct RefCountedSubChild : public RefCountedChild {
-  MOZ_CAN_RUN_SCRIPT void method_test3() override;
+  MOZ_CAN_RUN_SCRIPT void method_test3() override; // expected-error {{functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT}}
 };
 
-void RefCountedSubChild::method_test3() {
+void RefCountedSubChild::method_test3() { // expected-error {{functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT}}
   test();
 }
 
 MOZ_CAN_RUN_SCRIPT void test4() {
   RefPtr<RefCountedBase> refptr1 = new RefCountedChild;
   refptr1->method_test3();
 
   RefPtr<RefCountedBase> refptr2 = new RefCountedSubChild;