Extend ClassChecker to find erroneously overriding methods

Summary: This attempts to point out when a final method has been accidentally overridden in a sub class, following the details at https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.5 Reviewed By: NTillmann Differential Revision: D55454156 fbshipit-source-id: c9b3133c1a5d9797b1ef3111aeabb1d2b6d6bbc7
facebook · May 7, 2024 · fc6faa1 · fc6faa1
1 parent 68342f9
commit fc6faa1
Show file tree

Hide file tree

Showing 5 changed files with 286 additions and 23 deletions.
diff --git a/libredex/ClassChecker.cpp b/libredex/ClassChecker.cpp
@@ -5,40 +5,135 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+#include <mutex>
+
 #include "ClassChecker.h"
+#include "ClassHierarchy.h"
+#include "DexClass.h"
+#include "Show.h"
+#include "Timer.h"
+#include "TypeUtil.h"
 #include "Walkers.h"
 
+namespace {
+template <typename Collection>
+void print_failed_things(const Collection& items,
+                         const size_t print_limit,
+                         std::ostringstream* oss) {
+  size_t counter = 0;
+  for (auto& fail : items) {
+    *oss << show(fail)
+         << " (deobfuscated: " << fail->get_deobfuscated_name_or_empty_copy()
+         << ")\n";
+    counter++;
+    if (counter == print_limit) {
+      if (items.size() > print_limit) {
+        *oss << "...truncated...\n";
+      }
+      break;
+    }
+  }
+}
+
+struct NameAndProto {
+  const DexString* name;
+  const DexProto* proto;
+  explicit NameAndProto(const DexMethod* m)
+      : name(m->get_name()), proto(m->get_proto()) {}
+};
+
+struct compare_name_and_proto {
+  bool operator()(const NameAndProto& l, const NameAndProto& r) const {
+    if (l.name == r.name) {
+      return compare_dexprotos(l.proto, r.proto);
+    }
+    return compare_dexstrings(l.name, r.name);
+  }
+};
+
+using NamedMethodMap =
+    std::map<NameAndProto, const DexMethod*, compare_name_and_proto>;
+
+template <typename Collection>
+bool has_colliding_methods(const DexClass* cls,
+                           const NamedMethodMap& final_methods,
+                           const DexClass* child_cls,
+                           Collection* failures) {
+  bool result{false};
+  for (auto& v : child_cls->get_vmethods()) {
+    NameAndProto np(v);
+    auto search = final_methods.find(np);
+    if (search != final_methods.end()) {
+      // Check package visibility rules per
+      // https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.5
+      auto super_method = search->second;
+      if (is_public(super_method) || is_protected(super_method) ||
+          type::same_package(cls->get_type(), child_cls->get_type())) {
+        result = true;
+        failures->insert(v);
+      }
+    }
+  }
+  return result;
+}
+} // namespace
+
 ClassChecker::ClassChecker() : m_good(true) {}
 
 void ClassChecker::run(const Scope& scope) {
-  walk::parallel::classes(scope, [&](DexClass* cls) {
-    if (is_interface(cls) || is_abstract(cls)) {
-      return;
-    }
-    for (auto* m : cls->get_all_methods()) {
-      if (is_abstract(m)) {
-        m_good = false;
-        failed_classes.insert(cls);
-        return;
+  std::mutex finals_mutex;
+  std::unordered_map<const DexClass*, NamedMethodMap> class_to_final_methods;
+  {
+    Timer t("ClassChecker_walk");
+    walk::parallel::classes(scope, [&](DexClass* cls) {
+      if (!is_interface(cls) && !is_abstract(cls)) {
+        for (auto* m : cls->get_all_methods()) {
+          if (is_abstract(m)) {
+            m_good = false;
+            m_failed_classes.insert(cls);
+            return;
+          }
+        }
+      }
+      if (!is_interface(cls)) {
+        for (auto* m : cls->get_vmethods()) {
+          if (is_final(m)) {
+            std::lock_guard<std::mutex> lock(finals_mutex);
+            NameAndProto np(m);
+            class_to_final_methods[cls].emplace(np, m);
+          }
+        }
+      }
+    });
+  }
+  {
+    Timer t("ClassChecker_hierarchy_traverse");
+    auto hierarchy = build_internal_type_hierarchy(scope);
+    for (auto&& [cls, final_methods] : class_to_final_methods) {
+      auto child_types = get_all_children(hierarchy, cls->get_type());
+      for (const auto& child_type : child_types) {
+        auto child_cls = type_class(child_type);
+        always_assert(child_cls != nullptr);
+        if (has_colliding_methods(
+                cls, final_methods, child_cls, &m_failed_methods)) {
+          m_good = false;
+        }
       }
     }
-  });
+  }
 }
 
 std::ostringstream ClassChecker::print_failed_classes() {
+  constexpr size_t MAX_ITEMS_TO_PRINT = 10;
   std::ostringstream oss;
-  if (failed_classes.size() > 10) {
-    oss << "Too many errors. Only printing out the first 10\n";
+  if (!m_failed_classes.empty()) {
+    oss << "Nonabstract classes with abstract methods:" << std::endl;
+    print_failed_things(m_failed_classes, MAX_ITEMS_TO_PRINT, &oss);
   }
-  int counter = 0;
-  oss << "Nonabstract classes with abstract methods: ";
-  for (DexClass* fail : failed_classes) {
-    oss << fail->get_deobfuscated_name_or_empty_copy() << "\n";
-    counter++;
-    if (counter == 10) {
-      break;
-    }
+  if (!m_failed_methods.empty()) {
+    oss << "Methods incorrectly overriding super class final method:"
+        << std::endl;
+    print_failed_things(m_failed_methods, MAX_ITEMS_TO_PRINT, &oss);
   }
-
   return oss;
 }
diff --git a/libredex/ClassChecker.h b/libredex/ClassChecker.h
@@ -27,5 +27,7 @@ class ClassChecker {
 
  private:
   bool m_good;
-  ConcurrentSet<DexClass*> failed_classes;
+  ConcurrentSet<const DexClass*> m_failed_classes;
+  // Methods which are incorrectly overriding final methods on a super.
+  ConcurrentSet<const DexMethod*> m_failed_methods;
 };
diff --git a/libredex/ClassHierarchy.cpp b/libredex/ClassHierarchy.cpp
@@ -107,13 +107,18 @@ void build_interface_map(InterfaceMap& interfaces,
 
 } // namespace
 
-ClassHierarchy build_type_hierarchy(const Scope& scope) {
+ClassHierarchy build_internal_type_hierarchy(const Scope& scope) {
   ClassHierarchy hierarchy;
   // build the type hierarchy
   for (const auto& cls : scope) {
     if (is_interface(cls)) continue;
     build_class_hierarchy(hierarchy, cls);
   }
+  return hierarchy;
+}
+
+ClassHierarchy build_type_hierarchy(const Scope& scope) {
+  auto hierarchy = build_internal_type_hierarchy(scope);
   build_external_hierarchy(hierarchy);
   return hierarchy;
 }

diff --git a/libredex/ClassHierarchy.h b/libredex/ClassHierarchy.h
@@ -19,6 +19,13 @@ using TypeSet = std::set<const DexType*, dextypes_comparator>;
  */
 using ClassHierarchy = std::unordered_map<const DexType*, TypeSet>;
 
+/**
+ * Given a scope it builds all the parent-children relationships known. Excludes
+ * external classes as keys in the returned map, and will have incomplete
+ * hierarchies that end in external types.
+ */
+ClassHierarchy build_internal_type_hierarchy(const Scope& scope);
+
 /**
  * Given a scope it builds all the parent-children relationship known.
  * The walk stops once a DexClass is not found.

diff --git a/test/unit/ClassCheckerTest.cpp b/test/unit/ClassCheckerTest.cpp
@@ -119,3 +119,157 @@ TEST_F(ClassVCheckerTest, testInterfaceClassWithNonAbstractMethods) {
   checker.run(scope);
   EXPECT_FALSE(checker.fail());
 }
+
+namespace {
+// Make a super class A with a method called foo, of given proto and access, and
+// a subclass B also with a method called foo of given proto and access.
+Scope make_scope_with_a_foo_b_foo(const std::string& a_package,
+                                  DexProto* a_foo_proto,
+                                  DexAccessFlags a_foo_access,
+                                  const std::string& b_package,
+                                  DexProto* b_foo_proto,
+                                  DexAccessFlags b_foo_access) {
+  Scope scope = create_empty_scope();
+
+  DexType* a_type = DexType::make_type(a_package + "A;");
+  DexClass* a_cls =
+      create_internal_class(a_type, type::java_lang_Object(), {}, ACC_PUBLIC);
+
+  auto a_foo = create_empty_method(a_cls, "foo", a_foo_proto, a_foo_access);
+  always_assert(a_foo->is_virtual());
+
+  DexType* b_type = DexType::make_type(b_package + "B;");
+  DexClass* b_cls = create_internal_class(b_type, a_type, {}, ACC_PUBLIC);
+  auto b_foo = create_empty_method(b_cls, "foo", b_foo_proto, b_foo_access);
+  always_assert(b_foo->is_virtual());
+
+  scope.push_back(a_cls);
+  scope.push_back(b_cls);
+  return scope;
+}
+
+Scope make_scope_with_a_foo_b_foo(DexProto* a_foo_proto,
+                                  DexAccessFlags a_foo_access,
+                                  DexProto* b_foo_proto,
+                                  DexAccessFlags b_foo_access) {
+  return make_scope_with_a_foo_b_foo("Lredex/", a_foo_proto, a_foo_access,
+                                     "Lredex/", b_foo_proto, b_foo_access);
+}
+} // namespace
+
+TEST_F(ClassVCheckerTest, testFinalMethodNotInSubclassPasses) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  auto int_return_int = DexProto::make_proto(
+      type::_int(), DexTypeList::make_type_list({type::_int()}));
+  // A and B are in the same package, both defining foo() with different return
+  // value. No problem.
+  auto scope = make_scope_with_a_foo_b_foo(
+      int_return_void, ACC_PUBLIC | ACC_FINAL, int_return_int, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_FALSE(checker.fail());
+}
+
+TEST_F(ClassVCheckerTest, testFinalMethodInSubclassFails) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  // A and B are in the same package, both defining foo() with same proto.
+  // A.foo() is final, which should be disallowed.
+  auto scope = make_scope_with_a_foo_b_foo(
+      int_return_void, ACC_PUBLIC | ACC_FINAL, int_return_void, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_TRUE(checker.fail());
+  std::cerr << checker.print_failed_classes().str() << std::endl;
+}
+
+TEST_F(ClassVCheckerTest, testProtectedFinalMethodInSubclassFails) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  // Same as above, but A.foo() is protected, still should be disallowed.
+  auto scope = make_scope_with_a_foo_b_foo(
+      int_return_void, ACC_PROTECTED | ACC_FINAL, int_return_void, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_TRUE(checker.fail());
+  std::cerr << checker.print_failed_classes().str() << std::endl;
+}
+
+TEST_F(ClassVCheckerTest, testDefaultAccessFinalMethodInSubclassSamePkgFails) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  // Same as above, but A.foo() is default access, still should be disallowed.
+  auto scope = make_scope_with_a_foo_b_foo(int_return_void, ACC_FINAL,
+                                           int_return_void, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_TRUE(checker.fail());
+  std::cerr << checker.print_failed_classes().str() << std::endl;
+}
+
+TEST_F(ClassVCheckerTest, testDefaultAccessFinalMethodInSubclassOtherPkgPass) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  // A.foo() is final, package-private, and B.foo() has same signature but in a
+  // different package, should be fine.
+  auto scope =
+      make_scope_with_a_foo_b_foo("Lredex/", int_return_void, ACC_FINAL,
+                                  "Lother/", int_return_void, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_FALSE(checker.fail());
+}
+
+TEST_F(ClassVCheckerTest, testNonFinalOverrideInSubclassPasses) {
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  // A and B are in the same package, both defining foo() with same proto.
+  // A.foo() is not final, no problem.
+  auto scope = make_scope_with_a_foo_b_foo(int_return_void, ACC_PUBLIC,
+                                           int_return_void, ACC_PUBLIC);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_FALSE(checker.fail());
+}
+
+// This following example would not compile if written as source code
+// (P1203397896) but is represented in a test case for thoroughness.
+TEST_F(ClassVCheckerTest, testVeryFunnyBusiness) {
+  Scope scope = create_empty_scope();
+
+  auto int_return_void = DexProto::make_proto(
+      type::_void(), DexTypeList::make_type_list({type::_int()}));
+  DexAccessFlags def_access{};
+
+  DexType* a_type = DexType::make_type("Lredex/A;");
+  DexClass* a_cls =
+      create_internal_class(a_type, type::java_lang_Object(), {}, ACC_PUBLIC);
+
+  auto a_foo = create_empty_method(a_cls, "foo", int_return_void, ACC_FINAL);
+  always_assert(a_foo->is_virtual());
+
+  DexType* b_type = DexType::make_type("Lother/B;");
+  DexClass* b_cls = create_internal_class(b_type, a_type, {}, ACC_PUBLIC);
+  auto b_foo = create_empty_method(b_cls, "foo", int_return_void, def_access);
+  always_assert(b_foo->is_virtual());
+
+  DexType* c_type = DexType::make_type("Lredex/C;");
+  DexClass* c_cls = create_internal_class(c_type, b_type, {}, ACC_PUBLIC);
+  auto c_foo = create_empty_method(c_cls, "foo", int_return_void, def_access);
+  always_assert(c_foo->is_virtual());
+
+  scope.push_back(a_cls);
+  scope.push_back(b_cls);
+  scope.push_back(c_cls);
+
+  ClassChecker checker;
+  checker.run(scope);
+  EXPECT_TRUE(checker.fail());
+}