Validate that all referenced resources are public or internal Also remove an extraneous log+rethrow, in line with https://github.com/bazelbuild/bazel/commit/8425ddbd1df38976d22a206baf33a344c2412b79. PiperOrigin-RevId: 291050943
diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java index d060cc8..0259b17 100644 --- a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions; import com.google.devtools.build.android.aapt2.CompiledResources; import com.google.devtools.build.android.aapt2.PackagedResources; +import com.google.devtools.build.android.aapt2.ProtoApk; import com.google.devtools.build.android.aapt2.ResourceCompiler; import com.google.devtools.build.android.aapt2.ResourceLinker; import com.google.devtools.build.android.aapt2.StaticLibrary; @@ -441,7 +442,14 @@ .includeGeneratedLocales(aaptConfigOptions.generatePseudoLocale) .includeOnlyConfigs(aaptConfigOptions.resourceConfigs) .link(compiled); - profiler.recordEndOf("link"); + profiler.recordEndOf("link").startTask("validate"); + + ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( + ProtoApk.readFrom(packagedResources.proto()).getManifest(), + compiled, + compiledResourceDeps); + + profiler.recordEndOf("validate"); if (options.packagePath != null) { copy(packagedResources.apk(), options.packagePath);
diff --git a/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java b/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java index 597d5a3..8e878e2 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java
@@ -14,13 +14,17 @@ // Copyright 2017 The Bazel Authors. All rights reserved. package com.google.devtools.build.android; -import static java.util.logging.Level.SEVERE; - +import com.android.aapt.Resources.Reference; +import com.android.aapt.Resources.XmlNode; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions; import com.google.devtools.build.android.aapt2.CompiledResources; import com.google.devtools.build.android.aapt2.ResourceLinker; import com.google.devtools.build.android.aapt2.StaticLibrary; +import com.google.devtools.build.android.resources.Visibility; +import com.google.devtools.build.android.xml.ProtoXmlUtils; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -30,9 +34,8 @@ import java.nio.file.FileSystems; import java.nio.file.Path; import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.logging.Logger; -import java.util.stream.Collectors; /** Performs resource validation and static linking for compiled android resources. */ public class ValidateAndLinkResourcesAction { @@ -148,9 +151,6 @@ public Path sourceJarOut; } - private static final Logger logger = - Logger.getLogger(ValidateAndLinkResourcesAction.class.getName()); - public static void main(String[] args) throws Exception { OptionsParser optionsParser = OptionsParser.builder() @@ -182,14 +182,25 @@ .writeDummyManifestForAapt( scopedTmp.getPath().resolve("manifest-aapt-dummy"), options.packageForR)); - profiler.recordEndOf("manifest").startTask("link"); + List<CompiledResources> deps = + options.compiledDeps.stream() + .map(CompiledResources::from) + .collect(ImmutableList.toImmutableList()); + profiler.recordEndOf("manifest").startTask("validate"); + + // TODO(b/146663858): distinguish direct/transitive deps for "strict deps". + // TODO(b/128711690): validate AndroidManifest.xml + checkVisibilityOfResourceReferences( + /*androidManifest=*/ XmlNode.getDefaultInstance(), resources, deps); + + profiler.recordEndOf("validate").startTask("link"); ResourceLinker.create(aapt2Options.aapt2, executorService, scopedTmp.getPath()) .profileUsing(profiler) + // NB: these names are really confusing. + // .dependencies is meant for linking in android.jar + // .include is meant for regular dependencies .dependencies(Optional.ofNullable(options.deprecatedLibraries).orElse(options.libraries)) - .include( - options.compiledDeps.stream() - .map(CompiledResources::from) - .collect(Collectors.toList())) + .include(deps) .buildVersion(aapt2Options.buildToolsVersion) .outputAsProto(aapt2Options.resourceTableAsProto) .linkStatically(resources) @@ -197,9 +208,71 @@ .copySourceJarTo(options.sourceJarOut) .copyRTxtTo(options.rTxtOut); profiler.recordEndOf("link"); - } catch (Exception e) { - logger.log(SEVERE, "Error while validating and linking resources", e); - throw e; + } + } + + /** + * Validates that resources referenced from AndroidManifest.xml and res/ are visible. + * + * @param androidManifest AndroidManifest in protobuf format; while {@link CompiledResources} also + * contains a manifest, aapt2 requires it to be regular XML and only converts it to protobuf + * after "linking". + * @param compiled resources of a compilation unit (i.e. an android_library rule) + * @param deps resources from the transitive closure of the rule's "deps" attribute + */ + static void checkVisibilityOfResourceReferences( + XmlNode androidManifest, CompiledResources compiled, List<CompiledResources> deps) { + ImmutableSet<String> privateResourceNames = + deps.stream() + .flatMap( + cr -> + AndroidCompiledDataDeserializer.create( + /*includeFileContentsForValidation=*/ false) + .read(DependencyInfo.UNKNOWN, cr.getZip()) + .entrySet() + .stream()) + .filter(entry -> entry.getValue().getVisibility() == Visibility.PRIVATE) + .map(entry -> ((FullyQualifiedName) entry.getKey()).asQualifiedReference()) + .collect(ImmutableSet.toImmutableSet()); + + StringBuilder errorMessage = new StringBuilder(); + { + List<String> referencedPrivateResources = + ProtoXmlUtils.getAllResourceReferences(androidManifest).stream() + .map(Reference::getName) + .filter(privateResourceNames::contains) + .collect(ImmutableList.toImmutableList()); + if (!referencedPrivateResources.isEmpty()) { + errorMessage + .append("AndroidManifest.xml references external private resources ") + .append(referencedPrivateResources) + .append('\n'); + } + } + + for (Map.Entry<DataKey, DataResource> resource : + AndroidCompiledDataDeserializer.create(/*includeFileContentsForValidation=*/ true) + .read(DependencyInfo.UNKNOWN, compiled.getZip()) + .entrySet()) { + List<String> referencedPrivateResources = + resource.getValue().getReferencedResources().stream() + .map(Reference::getName) + .filter(privateResourceNames::contains) + .collect(ImmutableList.toImmutableList()); + + if (!referencedPrivateResources.isEmpty()) { + errorMessage + .append(resource.getKey().toPrettyString()) + .append(" (defined in ") + .append(resource.getValue().source().getPath()) + .append(") references external private resources ") + .append(referencedPrivateResources) + .append('\n'); + } + } + + if (errorMessage.length() != 0) { + throw new UserException(errorMessage.toString()); } } }
diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/ProtoApk.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/ProtoApk.java index 46c1572..7853e8e 100644 --- a/src/tools/android/java/com/google/devtools/build/android/aapt2/ProtoApk.java +++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/ProtoApk.java
@@ -43,6 +43,7 @@ import com.google.common.xml.XmlEscapers; import com.google.devtools.build.android.AndroidResourceOutputs.UniqueZipBuilder; import com.google.protobuf.ByteString; +import com.google.protobuf.ExtensionRegistry; import java.io.Closeable; import java.io.IOException; import java.io.InputStream; @@ -122,7 +123,9 @@ final UniqueZipBuilder dstZip = UniqueZipBuilder.createFor(destination)) { final ResourceTable.Builder dstTableBuilder = ResourceTable.newBuilder(); final ResourceTable resourceTable = - ResourceTable.parseFrom(Files.newInputStream(apkFileSystem.getPath(RESOURCE_TABLE))); + ResourceTable.parseFrom( + Files.newInputStream(apkFileSystem.getPath(RESOURCE_TABLE)), + ExtensionRegistry.getEmptyRegistry()); dstTableBuilder.setSourcePool(resourceTable.getSourcePool()); for (Package pkg : resourceTable.getPackageList()) { Package dstPkg = copyPackage(resourceFilter, dstZip, pkg); @@ -211,11 +214,16 @@ } } + public XmlNode getManifest() throws IOException { + try (InputStream in = Files.newInputStream(apkFileSystem.getPath(MANIFEST))) { + return XmlNode.parseFrom(in, ExtensionRegistry.getEmptyRegistry()); + } + } + /** Copy manifest as xml to an external directory. */ public Path writeManifestAsXmlTo(Path directory) { - try (InputStream in = Files.newInputStream(apkFileSystem.getPath(MANIFEST)); - XmlWriter out = XmlWriter.openNew(Files.createDirectories(directory).resolve(MANIFEST))) { - out.write(XmlNode.parseFrom(in)); + try (XmlWriter out = XmlWriter.openNew(Files.createDirectories(directory).resolve(MANIFEST))) { + out.write(getManifest()); return directory.resolve(MANIFEST); } catch (IOException e) { throw new ProtoApkException(e); @@ -359,7 +367,9 @@ // visit resource table and associated files. final ResourceTable resourceTable = - ResourceTable.parseFrom(Files.newInputStream(apkFileSystem.getPath(RESOURCE_TABLE))); + ResourceTable.parseFrom( + Files.newInputStream(apkFileSystem.getPath(RESOURCE_TABLE)), + ExtensionRegistry.getEmptyRegistry()); final List<String> sourcePool = resourceTable.hasSourcePool() @@ -580,7 +590,7 @@ } try (InputStream in = Files.newInputStream(path)) { - visit(XmlNode.parseFrom(in), visitor); + visit(XmlNode.parseFrom(in, ExtensionRegistry.getEmptyRegistry()), visitor); } catch (IOException e) { throw new RuntimeException(e); }