From fc6faa19589062d5a94bb3faf5b4d058317f820a Mon Sep 17 00:00:00 2001 From: William Sanville Date: Tue, 7 May 2024 10:43:16 -0700 Subject: [PATCH] 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 --- libredex/ClassChecker.cpp | 137 ++++++++++++++++++++++++----- libredex/ClassChecker.h | 4 +- libredex/ClassHierarchy.cpp | 7 +- libredex/ClassHierarchy.h | 7 ++ test/unit/ClassCheckerTest.cpp | 154 +++++++++++++++++++++++++++++++++ 5 files changed, 286 insertions(+), 23 deletions(-) diff --git a/libredex/ClassChecker.cpp b/libredex/ClassChecker.cpp index ed99488a2e3..5bc66107df9 100644 --- a/libredex/ClassChecker.cpp +++ b/libredex/ClassChecker.cpp @@ -5,40 +5,135 @@ * LICENSE file in the root directory of this source tree. */ +#include + #include "ClassChecker.h" +#include "ClassHierarchy.h" +#include "DexClass.h" +#include "Show.h" +#include "Timer.h" +#include "TypeUtil.h" #include "Walkers.h" +namespace { +template +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; + +template +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 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 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 index 60d24e1c50c..ef307facc0e 100644 --- a/libredex/ClassChecker.h +++ b/libredex/ClassChecker.h @@ -27,5 +27,7 @@ class ClassChecker { private: bool m_good; - ConcurrentSet failed_classes; + ConcurrentSet m_failed_classes; + // Methods which are incorrectly overriding final methods on a super. + ConcurrentSet m_failed_methods; }; diff --git a/libredex/ClassHierarchy.cpp b/libredex/ClassHierarchy.cpp index bfd3b44f528..601e303fd14 100644 --- 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 index e5fca9a8bd2..76aee6feaa5 100644 --- a/libredex/ClassHierarchy.h +++ b/libredex/ClassHierarchy.h @@ -19,6 +19,13 @@ using TypeSet = std::set; */ using ClassHierarchy = std::unordered_map; +/** + * 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 index c4c601e7f63..ef0f4e25e5d 100644 --- 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()); +}