From f6b8d5e269cf64bda483af464ad98af7277728a6 Mon Sep 17 00:00:00 2001 From: kmb Date: Thu, 12 Oct 2017 01:21:26 +0200 Subject: add option to singlejar to double-check correct default and static interface method desugaring for Android. RELNOTES: none PiperOrigin-RevId: 171891682 --- WORKSPACE | 6 ++ src/tools/singlejar/BUILD | 5 +- src/tools/singlejar/combiners.cc | 137 ++++++++++++++++++++++++++++++++++ src/tools/singlejar/combiners.h | 60 +++++++++++++++ src/tools/singlejar/combiners_test.cc | 96 ++++++++++++++++++++---- src/tools/singlejar/options.cc | 3 +- src/tools/singlejar/options.h | 4 +- src/tools/singlejar/options_test.cc | 3 + src/tools/singlejar/output_jar.cc | 13 ++++ 9 files changed, 310 insertions(+), 17 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index f9af7af640..a86032d907 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -78,6 +78,12 @@ new_local_repository( path = "./third_party/protobuf/3.4.0/", ) +new_local_repository( + name = "com_google_protobuf_cc", + build_file = "./third_party/protobuf/3.4.0/BUILD", + path = "./third_party/protobuf/3.4.0/", +) + new_local_repository( name = "com_google_protobuf_java", build_file = "./third_party/protobuf/3.4.0/com_google_protobuf_java.BUILD", diff --git a/src/tools/singlejar/BUILD b/src/tools/singlejar/BUILD index 35b0c50e3b..e986ab06c6 100644 --- a/src/tools/singlejar/BUILD +++ b/src/tools/singlejar/BUILD @@ -285,7 +285,10 @@ cc_library( ":zip_headers", ], hdrs = ["combiners.h"], - deps = ["//third_party/zlib"], + deps = [ + "//src/main/protobuf:desugar_deps_cc_proto", + "//third_party/zlib", + ], ) cc_library( diff --git a/src/tools/singlejar/combiners.cc b/src/tools/singlejar/combiners.cc index 8fd2a0fdfe..aaddd79aaa 100644 --- a/src/tools/singlejar/combiners.cc +++ b/src/tools/singlejar/combiners.cc @@ -14,6 +14,7 @@ #include "src/tools/singlejar/combiners.h" #include "src/tools/singlejar/diag.h" +#include "src/main/protobuf/desugar_deps.pb.h" Combiner::~Combiner() {} @@ -174,3 +175,139 @@ PropertyCombiner::~PropertyCombiner() {} bool PropertyCombiner::Merge(const CDH *cdh, const LH *lh) { return false; // This should not be called. } + +bool Java8DesugarDepsChecker::Merge(const CDH *cdh, const LH *lh) { + // Throw away anything previously read, no need to concatenate + buffer_.reset(new TransientBytes()); + if (Z_NO_COMPRESSION == lh->compression_method()) { + buffer_->ReadEntryContents(lh); + } else if (Z_DEFLATED == lh->compression_method()) { + if (!inflater_.get()) { + inflater_.reset(new Inflater()); + } + buffer_->DecompressEntryContents(cdh, lh, inflater_.get()); + } else { + errx(2, "META-INF/desugar_deps is neither stored nor deflated"); + } + + // TODO(kmb): Wrap buffer_ as ZeroCopyInputStream to avoid copying out. + // Note we only copy one file at a time, so overhead should be modest. + uint32_t checksum; + const size_t data_size = buffer_->data_size(); + uint8_t *buf = reinterpret_cast(malloc(data_size)); + buffer_->CopyOut(reinterpret_cast(buf), &checksum); + buffer_.reset(); // release buffer eagerly + + bazel::tools::desugar::DesugarDepsInfo deps_info; + google::protobuf::io::CodedInputStream content(buf, data_size); + if (!deps_info.ParseFromCodedStream(&content)) { + errx(2, "META-INF/desugar_deps: unable to parse"); + } + if (!content.ConsumedEntireMessage()) { + errx(2, "META-INF/desugar_deps: unexpected trailing content"); + } + free(buf); + + for (const auto &assume_present : deps_info.assume_present()) { + // This means we need file named .class in the output. Remember + // the first origin of this requirement for error messages, drop others. + needed_deps_.emplace(assume_present.target().binary_name() + ".class", + assume_present.origin().binary_name()); + } + + for (const auto &missing : deps_info.missing_interface()) { + // Remember the first origin of this requirement for error messages, drop + // subsequent ones. + missing_interfaces_.emplace(missing.target().binary_name(), + missing.origin().binary_name()); + } + + for (const auto &extends : deps_info.interface_with_supertypes()) { + // Remember interface hierarchy the first time we see this interface, drop + // subsequent ones for consistency with how singlejar will keep the first + // occurrence of the file defining the interface. We'll lazily derive + // whether missing_interfaces_ inherit default methods with this data later. + if (extends.extended_interface_size() > 0) { + std::vector extended; + extended.reserve(extends.extended_interface_size()); + for (const auto &itf : extends.extended_interface()) { + extended.push_back(itf.binary_name()); + } + extended_interfaces_.emplace(extends.origin().binary_name(), + std::move(extended)); + } + } + + for (const auto &companion : deps_info.interface_with_companion()) { + // Only remember interfaces that definitely have default methods for now. + // For all other interfaces we'll transitively check extended interfaces + // in HasDefaultMethods. + if (companion.num_default_methods() > 0) { + has_default_methods_[companion.origin().binary_name()] = true; + } + } + return true; +} + +void *Java8DesugarDepsChecker::OutputEntry(bool compress) { + if (verbose_) { + fprintf(stderr, "Needed deps: %lu\n", needed_deps_.size()); + fprintf(stderr, "Interfaces to check: %lu\n", missing_interfaces_.size()); + fprintf(stderr, "Sub-interfaces: %lu\n", extended_interfaces_.size()); + fprintf(stderr, "Interfaces w/ default methods: %lu\n", + has_default_methods_.size()); + } + for (auto needed : needed_deps_) { + if (verbose_) { + fprintf(stderr, "Looking for %s\n", needed.first.c_str()); + } + if (!known_member_(needed.first)) { + if (fail_on_error_) { + errx(2, "%s referenced by %s but not found. Is the former defined in " + "a neverlink library?", + needed.first.c_str(), needed.second.c_str()); + } else { + error_ = true; + } + } + } + + for (auto missing : missing_interfaces_) { + if (verbose_) { + fprintf(stderr, "Checking %s\n", missing.first.c_str()); + } + if (HasDefaultMethods(missing.first)) { + if (fail_on_error_) { + errx(2, "%s needed on the classpath for desugaring %s. Please add the " + "missing dependency to the target containing the latter.", + missing.first.c_str(), missing.second.c_str()); + } else { + error_ = true; + } + } + } + + // We don't want these files in the output, just check them for consistency + return nullptr; +} + +bool Java8DesugarDepsChecker::HasDefaultMethods( + const std::string &interface_name) { + auto cached = has_default_methods_.find(interface_name); + if (cached != has_default_methods_.end()) { + return cached->second; + } + + // Prime with false in case there's a cycle. We'll update with the true value + // (ignoring the cycle) below. + has_default_methods_.emplace(interface_name, false); + + for (const std::string &extended : extended_interfaces_[interface_name]) { + if (HasDefaultMethods(extended)) { + has_default_methods_[interface_name] = true; + return true; + } + } + has_default_methods_[interface_name] = false; + return false; +} diff --git a/src/tools/singlejar/combiners.h b/src/tools/singlejar/combiners.h index 98c2df47c5..f7d4cc7be9 100644 --- a/src/tools/singlejar/combiners.h +++ b/src/tools/singlejar/combiners.h @@ -15,8 +15,11 @@ #ifndef SRC_TOOLS_SINGLEJAR_COMBINERS_H_ #define SRC_TOOLS_SINGLEJAR_COMBINERS_H_ 1 +#include #include #include +#include +#include #include "src/tools/singlejar/transient_bytes.h" #include "src/tools/singlejar/zip_headers.h" @@ -133,4 +136,61 @@ class PropertyCombiner : public Concatenator { } }; +// Combiner that checks META-INF/desugar_deps files (b/65645388) to ensure +// correct bytecode desugaring, specifically of default and static interface +// methods, across an entire binary. Two checks are performed: +// 1. Make sure that any dependency assumed by the desugaring process is in +// fact part of the binary. This protects against ill-advised uses of +// neverlink, where a library is only on the compile-time classpath but not +// the runtime classpath. +// 2. To paper over incomplete classpaths during desugaring (b/65211436), check +// that interfaces that couldn't be found don't declare or inherit default +// methods. Desugar emits extra metadata to avoid us having to open up and +// parse .class files for this purpose. +class Java8DesugarDepsChecker : public Combiner { + public: + Java8DesugarDepsChecker(std::function known_member, + bool verbose) + : Java8DesugarDepsChecker(std::move(known_member), verbose, true) {} + ~Java8DesugarDepsChecker() override {} + + bool Merge(const CDH *cdh, const LH *lh) override; + + void *OutputEntry(bool compress) override; + + private: + Java8DesugarDepsChecker(std::function known_member, + bool verbose, bool fail_on_error) + : known_member_(std::move(known_member)), + verbose_(verbose), + fail_on_error_(fail_on_error), + error_(false) {} + /// Computes and caches whether the given interface has default methods. + /// \param interface_name interface name as it would appear in bytecode, e.g., + /// "java/lang/Runnable" + bool HasDefaultMethods(const std::string &interface_name); + + const std::function known_member_; + const bool verbose_; + const bool fail_on_error_; // For testing + + std::unique_ptr buffer_; + std::unique_ptr inflater_; + /// Reverse mapping from needed dependencies to one of the users. + std::map needed_deps_; + /// Reverse mapping from missing interfaces to one of the classes that missed + /// them. + std::map missing_interfaces_; + std::unordered_map > + extended_interfaces_; + /// Cache of interfaces known to definitely define or inherit default methods + /// or definitely not define and not inherit default methods. Merge() + /// populates initial entries and HasDefaultMethods() adds to the cache as + /// needed. + std::unordered_map has_default_methods_; + bool error_; + + friend class CombinersTest; +}; + #endif // SRC_TOOLS_SINGLEJAR_COMBINERS_H_ diff --git a/src/tools/singlejar/combiners_test.cc b/src/tools/singlejar/combiners_test.cc index b4358ffa68..effdf265be 100644 --- a/src/tools/singlejar/combiners_test.cc +++ b/src/tools/singlejar/combiners_test.cc @@ -19,9 +19,6 @@ #include "src/tools/singlejar/zlib_interface.h" #include "gtest/gtest.h" -namespace { -using std::string; - static const char kTag1Contents[] = "Contents1"; static const char kTag2Contents[] = "Contents2"; static const char kCombinedXmlContents[] = @@ -52,6 +49,67 @@ class CombinersTest : public ::testing::Test { } return true; } + + static void TestJava8DesugarDepsChecker_HasDefaultMethods() { + Java8DesugarDepsChecker checker([](const std::string &) { return false; }, + /*verbose=*/false); + checker.has_default_methods_["a"] = true; + checker.extended_interfaces_["c"] = {"b", "a"}; + + // Induce cycle (shouldn't happen but make sure we don't crash) + checker.extended_interfaces_["d"] = {"e"}; + checker.extended_interfaces_["e"] = {"d", "a"}; + + EXPECT_TRUE(checker.HasDefaultMethods("a")); + EXPECT_FALSE(checker.HasDefaultMethods("b")); + EXPECT_TRUE(checker.HasDefaultMethods("c")); // Transitivly through a + EXPECT_TRUE(checker.HasDefaultMethods("d")); // Transitivly through a + EXPECT_FALSE(checker.error_); + } + + static void TestJava8DesugarDepsChecker_OutputEntry() { + bool checkedA = false; + Java8DesugarDepsChecker checker( + [&checkedA](const std::string &binary_name) { + checkedA = true; + return binary_name == "a$$CC.class"; + }, + /*verbose=*/false); + checker.has_default_methods_["a"] = true; + checker.extended_interfaces_["b"] = {"c", "d"}; + checker.extended_interfaces_["c"] = {"e"}; + checker.needed_deps_["a$$CC.class"] = "f"; + checker.missing_interfaces_["b"] = "g"; + EXPECT_EQ(nullptr, checker.OutputEntry(/*compress=*/true)); + EXPECT_TRUE(checkedA); + + // Make sure we checked b and its extended interfaces for default methods + EXPECT_FALSE(checker.has_default_methods_.at("b")); // should be cached + EXPECT_FALSE(checker.has_default_methods_.at("c")); // should be cached + EXPECT_FALSE(checker.has_default_methods_.at("d")); // should be cached + EXPECT_FALSE(checker.has_default_methods_.at("e")); // should be cached + EXPECT_FALSE(checker.error_); + } + + static void TestJava8DesugarDepsChecker_NeededDepMissing() { + Java8DesugarDepsChecker checker([](const std::string &) { return false; }, + /*verbose=*/false, + /*fail_on_error=*/false); + checker.needed_deps_["a$$CC.class"] = "b"; + EXPECT_EQ(nullptr, checker.OutputEntry(/*compress=*/true)); + EXPECT_TRUE(checker.error_); + } + + static void TestJava8DesugarDepsChecker_MissedDefaultMethods() { + Java8DesugarDepsChecker checker([](const std::string &) { return true; }, + /*verbose=*/false, + /*fail_on_error=*/false); + checker.has_default_methods_["b"] = true; + checker.extended_interfaces_["a"] = {"b", "a"}; + checker.missing_interfaces_["a"] = "g"; + EXPECT_EQ(nullptr, checker.OutputEntry(/*compress=*/true)); + EXPECT_TRUE(checker.error_); + } }; // Test Concatenator. @@ -87,7 +145,7 @@ TEST_F(CombinersTest, ConcatenatorSmall) { ASSERT_EQ(Z_STREAM_END, inflater.Inflate((buffer), sizeof(buffer))); EXPECT_EQ(kPoison, buffer[original_size]); EXPECT_EQ(kConcatenatedContents, - string(reinterpret_cast(buffer), original_size)); + std::string(reinterpret_cast(buffer), original_size)); free(reinterpret_cast(entry)); // And if we just copy instead of compress: @@ -98,8 +156,9 @@ TEST_F(CombinersTest, ConcatenatorSmall) { original_size = entry->uncompressed_file_size(); compressed_size = entry->compressed_file_size(); EXPECT_EQ(compressed_size, original_size); - EXPECT_EQ(kConcatenatedContents, - string(reinterpret_cast(entry->data()), original_size)); + EXPECT_EQ( + kConcatenatedContents, + std::string(reinterpret_cast(entry->data()), original_size)); EXPECT_TRUE(entry->file_name_is("concat")); EXPECT_EQ(0, entry->extra_fields_length()); free(reinterpret_cast(entry)); @@ -175,7 +234,7 @@ TEST_F(CombinersTest, XmlCombiner) { ASSERT_EQ(Z_STREAM_END, inflater.Inflate((buffer), sizeof(buffer))); EXPECT_EQ(kPoison, buffer[original_size]); EXPECT_EQ(kCombinedXmlContents, - string(reinterpret_cast(buffer), original_size)); + std::string(reinterpret_cast(buffer), original_size)); free(reinterpret_cast(entry)); // And for the combiner that just copies out: @@ -186,8 +245,9 @@ TEST_F(CombinersTest, XmlCombiner) { original_size = entry->uncompressed_file_size(); compressed_size = entry->compressed_file_size(); EXPECT_EQ(compressed_size, original_size); - EXPECT_EQ(kCombinedXmlContents, - string(reinterpret_cast(entry->data()), original_size)); + EXPECT_EQ( + kCombinedXmlContents, + std::string(reinterpret_cast(entry->data()), original_size)); EXPECT_TRUE(entry->file_name_is("combined2.xml")); EXPECT_EQ(0, entry->extra_fields_length()); free(reinterpret_cast(entry)); @@ -200,7 +260,8 @@ TEST_F(CombinersTest, PropertyCombiner) { "name_str=value_str\n"; PropertyCombiner property_combiner("properties"); property_combiner.AddProperty("name", "value"); - property_combiner.AddProperty(string("name_str"), string("value_str")); + property_combiner.AddProperty(std::string("name_str"), + std::string("value_str")); // Merge should not be called. ASSERT_FALSE(property_combiner.Merge(nullptr, nullptr)); @@ -225,7 +286,7 @@ TEST_F(CombinersTest, PropertyCombiner) { ASSERT_EQ(Z_STREAM_END, inflater.Inflate((buffer), sizeof(buffer))); EXPECT_EQ(kPoison, buffer[original_size]); EXPECT_EQ(kProperties, - string(reinterpret_cast(buffer), original_size)); + std::string(reinterpret_cast(buffer), original_size)); free(reinterpret_cast(entry)); // Create output, verify Local Header contents. @@ -236,11 +297,18 @@ TEST_F(CombinersTest, PropertyCombiner) { original_size = entry->uncompressed_file_size(); compressed_size = entry->compressed_file_size(); EXPECT_EQ(compressed_size, original_size); - EXPECT_EQ(kProperties, - string(reinterpret_cast(entry->data()), original_size)); + EXPECT_EQ( + kProperties, + std::string(reinterpret_cast(entry->data()), original_size)); EXPECT_EQ("properties", entry->file_name_string()); EXPECT_EQ(0, entry->extra_fields_length()); free(reinterpret_cast(entry)); } -} // namespace +TEST_F(CombinersTest, Java8DesugarDepsChecker) { + // Tests are instance methods of CombinersTest to avoid gUnit dep in .h file. + TestJava8DesugarDepsChecker_HasDefaultMethods(); + TestJava8DesugarDepsChecker_OutputEntry(); + TestJava8DesugarDepsChecker_NeededDepMissing(); + TestJava8DesugarDepsChecker_MissedDefaultMethods(); +} diff --git a/src/tools/singlejar/options.cc b/src/tools/singlejar/options.cc index 11484c4124..86c35fe142 100644 --- a/src/tools/singlejar/options.cc +++ b/src/tools/singlejar/options.cc @@ -38,7 +38,8 @@ void Options::ParseCommandLine(int argc, const char * const argv[]) { tokens.MatchAndSet("--verbose", &verbose) || tokens.MatchAndSet("--warn_duplicate_resources", &warn_duplicate_resources) || - tokens.MatchAndSet("--nocompress_suffixes", &nocompress_suffixes)) { + tokens.MatchAndSet("--nocompress_suffixes", &nocompress_suffixes) || + tokens.MatchAndSet("--check_desugar_deps", &check_desugar_deps)) { continue; } else if (tokens.MatchAndSet("--build_info_file", &optarg)) { build_info_files.push_back(optarg); diff --git a/src/tools/singlejar/options.h b/src/tools/singlejar/options.h index cc01cd8547..c228fc6892 100644 --- a/src/tools/singlejar/options.h +++ b/src/tools/singlejar/options.h @@ -29,7 +29,8 @@ class Options { no_duplicate_classes(false), preserve_compression(false), verbose(false), - warn_duplicate_resources(false) {} + warn_duplicate_resources(false), + check_desugar_deps(false) {} // Parses command line arguments into the fields of this instance. void ParseCommandLine(int argc, const char * const argv[]); @@ -53,6 +54,7 @@ class Options { bool preserve_compression; bool verbose; bool warn_duplicate_resources; + bool check_desugar_deps; }; #endif // THIRD_PARTY_BAZEL_SRC_TOOLS_SINGLEJAR_OPTIONS_H_ diff --git a/src/tools/singlejar/options_test.cc b/src/tools/singlejar/options_test.cc index 14e1d4e5c3..a16c134161 100644 --- a/src/tools/singlejar/options_test.cc +++ b/src/tools/singlejar/options_test.cc @@ -36,6 +36,7 @@ TEST(OptionsTest, Flags1) { EXPECT_FALSE(options.preserve_compression); EXPECT_FALSE(options.verbose); EXPECT_FALSE(options.warn_duplicate_resources); + EXPECT_FALSE(options.check_desugar_deps); EXPECT_EQ("output_jar", options.output_jar); } @@ -43,6 +44,7 @@ TEST(OptionsTest, Flags2) { const char *args[] = {"--dont_change_compression", "--verbose", "--warn_duplicate_resources", + "--check_desugar_deps", "--output", "output_jar"}; Options options; options.ParseCommandLine(arraysize(args), args); @@ -54,6 +56,7 @@ TEST(OptionsTest, Flags2) { ASSERT_TRUE(options.preserve_compression); ASSERT_TRUE(options.verbose); ASSERT_TRUE(options.warn_duplicate_resources); + ASSERT_TRUE(options.check_desugar_deps); } TEST(OptionsTest, SingleOptargs) { diff --git a/src/tools/singlejar/output_jar.cc b/src/tools/singlejar/output_jar.cc index 7fd97abb6e..3674d27aee 100644 --- a/src/tools/singlejar/output_jar.cc +++ b/src/tools/singlejar/output_jar.cc @@ -89,6 +89,17 @@ int OutputJar::Doit(Options *options) { EntryInfo{&build_properties_}); } + // Process or drop Java 8 desugaring metadata, see b/65645388. We don't want + // or need these files afterwards so make sure we drop them either way. + Combiner *desugar_checker = options_->check_desugar_deps + ? new Java8DesugarDepsChecker( + [this](const std::string &filename) { + return !NewEntry(filename); + }, + options_->verbose) + : (Combiner *)new NullCombiner(); + ExtraCombiner("META-INF/desugar_deps", desugar_checker); + build_properties_.AddProperty("build.target", options_->output_jar.c_str()); if (options_->verbose) { fprintf(stderr, "combined_file_name=%s\n", options_->output_jar.c_str()); @@ -352,6 +363,8 @@ bool OutputJar::AddJar(int jar_path_index) { auto &entry_info = got.first->second; // Handle special entries (the ones that have a combiner). if (entry_info.combiner_ != nullptr) { + // TODO(kmb,asmundak): Should be checking Merge() return value but fails + // for build-data.properties when merging deploy jars into deploy jars. entry_info.combiner_->Merge(jar_entry, lh); continue; } -- cgit v1.2.3