Make SplitZipStep use proguard mapping on builds with obfuscation. Summary: Previously, we had some hacks in place to make zip splitting work properly on builds with obfuscated class names, but they weren't scalable. Now the split-zip step pulls in the proguard class name mapping and uses that to apply primary dex policies more accurately.
diff --git a/src/com/facebook/buck/android/AndroidBinaryRule.java b/src/com/facebook/buck/android/AndroidBinaryRule.java index 74d9fad..e802db5 100644 --- a/src/com/facebook/buck/android/AndroidBinaryRule.java +++ b/src/com/facebook/buck/android/AndroidBinaryRule.java
@@ -1041,6 +1041,11 @@ final Optional<String> secondaryInputsDir; if (shouldSplitDex()) { + Optional<Path> proguardMappingFile = Optional.absent(); + if (packageType.isBuildWithObfuscation()) { + proguardMappingFile = Optional.of(getPathForProGuardDirectory().resolve("mapping.txt")); + } + // DexLibLoader expects that metadata.txt and secondary jar files are under this dir // in assets. String magicSecondaryDexSubdir = "assets/secondary-program-dex-jars"; @@ -1073,6 +1078,7 @@ primaryJarPath, secondaryZipDir, "secondary-%d.jar", + proguardMappingFile, primaryDexSubstrings, primaryDexClassesFile.transform(sourcePathResolver), dexSplitMode.getDexSplitStrategy(),
diff --git a/src/com/facebook/buck/android/ProguardMapping.java b/src/com/facebook/buck/android/ProguardMapping.java new file mode 100644 index 0000000..2bdc6c0 --- /dev/null +++ b/src/com/facebook/buck/android/ProguardMapping.java
@@ -0,0 +1,50 @@ +/* + * Copyright 2012-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.facebook.buck.android; + +import com.google.common.collect.ImmutableMap; + +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Parser for ProGuard-generated mapping files. Currently only handles class mapping. + */ +public class ProguardMapping { + private static Pattern CLASS_LINE_PATTERN = Pattern.compile("([\\w.$]+) -> ([\\w.$]+):"); + + public static Map<String, String> readClassMapping(Iterable<String> lines) { + ImmutableMap.Builder<String, String> classMappingBuilder = ImmutableMap.builder(); + + for (String line : lines) { + if (line.charAt(0) == ' ') { + // This is a member mapping, which we don't handle yet. + continue; + } + + Matcher matcher = CLASS_LINE_PATTERN.matcher(line); + if (!matcher.matches()) { + throw new IllegalArgumentException("Invalid line in proguard mapping: " + line); + } + + classMappingBuilder.put(matcher.group(1), matcher.group(2)); + } + + return classMappingBuilder.build(); + } +}
diff --git a/src/com/facebook/buck/android/SplitZipStep.java b/src/com/facebook/buck/android/SplitZipStep.java index d3b85d1..637b77b 100644 --- a/src/com/facebook/buck/android/SplitZipStep.java +++ b/src/com/facebook/buck/android/SplitZipStep.java
@@ -26,11 +26,13 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Charsets; import com.google.common.base.Function; +import com.google.common.base.Functions; import com.google.common.base.Joiner; import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.hash.Hashing; import com.google.common.io.Files; @@ -41,12 +43,15 @@ import java.nio.file.Path; import java.util.Collection; import java.util.Collections; +import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import javax.annotation.Nullable; + /** * Split zipping tool designed to divide input code blobs into a set of output jar files such that * none will exceed the DexOpt LinearAlloc limit or the dx method limit when passed through @@ -71,6 +76,7 @@ private final String primaryJarPath; private final String secondaryJarDir; private final String secondaryJarPattern; + private final Optional<Path> proguardMappingFile; private final ImmutableSet<String> primaryDexSubstrings; private final Optional<Path> primaryDexClassesFile; private final ZipSplitter.DexSplitStrategy dexSplitStrategy; @@ -101,6 +107,7 @@ String primaryJarPath, String secondaryJarDir, String secondaryJarPattern, + Optional<Path> proguardMappingFile, Set<String> primaryDexSubstrings, Optional<Path> primaryDexClassesFile, ZipSplitter.DexSplitStrategy dexSplitStrategy, @@ -113,6 +120,7 @@ this.primaryJarPath = Preconditions.checkNotNull(primaryJarPath); this.secondaryJarDir = Preconditions.checkNotNull(secondaryJarDir); this.secondaryJarPattern = Preconditions.checkNotNull(secondaryJarPattern); + this.proguardMappingFile = Preconditions.checkNotNull(proguardMappingFile); this.primaryDexSubstrings = ImmutableSet.copyOf(primaryDexSubstrings); this.primaryDexClassesFile = Preconditions.checkNotNull(primaryDexClassesFile); this.dexSplitStrategy = Preconditions.checkNotNull(dexSplitStrategy); @@ -168,6 +176,7 @@ @VisibleForTesting Predicate<String> createRequiredInPrimaryZipPredicate(ExecutionContext context) throws IOException { + final Function<String, String> deobfuscate = createProguardDeobfuscator(context); final ImmutableSet<String> primaryDexClassNames = getPrimaryDexClassNames(context); return new Predicate<String>() { @@ -181,12 +190,16 @@ return true; } - if (primaryDexClassNames.contains(classFileName)) { + // Drop the ".class" suffix and deobfuscate the class name before we apply our checks. + String internalClassName = Preconditions.checkNotNull( + deobfuscate.apply(classFileName.replaceAll("\\.class$", ""))); + + if (primaryDexClassNames.contains(internalClassName)) { return true; } for (String substr : SplitZipStep.this.primaryDexSubstrings) { - if (classFileName.contains(substr)) { + if (internalClassName.contains(substr)) { return true; } } @@ -195,6 +208,10 @@ }; } + /** + * Construct a {@link Set} of internal class names that should go into the primary dex. + * @return the Set. + */ private ImmutableSet<String> getPrimaryDexClassNames(ExecutionContext context) throws IOException { if (!primaryDexClassesFile.isPresent()) { @@ -218,16 +235,40 @@ return !line.isEmpty() && !(line.charAt(0) == '#'); } }) - .transform(new Function<String, String>() { - @Override - public String apply(String line) { - // Append a ".class" so the input file can just contain the internal class name. - return line + ".class"; - } - }) .toSet(); } + /** + * Create a {@link Function} that will deobfuscate internal class names for this build. + * @return the Function. + */ + private Function<String, String> createProguardDeobfuscator(ExecutionContext context) + throws IOException { + if (!proguardMappingFile.isPresent()) { + return Functions.identity(); + } + + Map<String, String> rawProguardMap = ProguardMapping.readClassMapping( + context.getProjectFilesystem().readLines(proguardMappingFile.get())); + + ImmutableMap.Builder<String, String> internalNameBuilder = ImmutableMap.builder(); + for (Map.Entry<String, String> entry : rawProguardMap.entrySet()) { + internalNameBuilder.put( + entry.getValue().replace('.', '/'), + entry.getKey().replace('.', '/')); + } + final Map<String, String> deobfuscator = internalNameBuilder.build(); + + return new Function<String, String>() { + @Nullable + @Override + public String apply(@Nullable String input) { + return deobfuscator.get(input); + } + }; + + } + @VisibleForTesting static void writeMetaList( BufferedWriter writer,
diff --git a/test/com/facebook/buck/android/ProguardMappingTest.java b/test/com/facebook/buck/android/ProguardMappingTest.java new file mode 100644 index 0000000..4bf900d --- /dev/null +++ b/test/com/facebook/buck/android/ProguardMappingTest.java
@@ -0,0 +1,48 @@ +/* + * Copyright 2012-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.facebook.buck.android; + +import static org.junit.Assert.assertEquals; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + +import org.junit.Test; + +import java.util.Map; + +public class ProguardMappingTest { + @Test + public void testBasicParse() { + Map<String, String> mapping = ProguardMapping.readClassMapping(ImmutableList.of( + "foo.bar.Baz -> foo.bar.a:", + " member -> x", + "foo.bar.Baz$Qux -> foo.bar.Baz$Qux:" + )); + assertEquals(mapping, ImmutableMap.of( + "foo.bar.Baz", "foo.bar.a", + "foo.bar.Baz$Qux", "foo.bar.Baz$Qux" + )); + } + + @Test(expected = IllegalArgumentException.class) + public void testInternalNameError() { + ProguardMapping.readClassMapping(ImmutableList.of( + "foo/bar/Baz -> foo/bar/a:" + )); + } +}
diff --git a/test/com/facebook/buck/android/SplitZipStepTest.java b/test/com/facebook/buck/android/SplitZipStepTest.java index bc3dd00..13a5b9c 100644 --- a/test/com/facebook/buck/android/SplitZipStepTest.java +++ b/test/com/facebook/buck/android/SplitZipStepTest.java
@@ -101,6 +101,7 @@ /* primaryJarPath */ "", /* secondaryJarDir */ "", /* secondaryJarPattern */ "", + /* proguardMappingFile */ Optional.<Path>absent(), /* primaryDexSubstrings */ ImmutableSet.of("List"), Optional.of(primaryDexClassesFile), ZipSplitter.DexSplitStrategy.MAXIMIZE_PRIMARY_DEX_SIZE, @@ -152,6 +153,79 @@ } @Test + public void testRequiredInPrimaryZipPredicateWithProguard() throws IOException { + Path proguardMappingFile = Paths.get("the/mapping.txt"); + Path primaryDexClassesFile = Paths.get("the/manifest.txt"); + SplitZipStep splitZipStep = new SplitZipStep( + /* inputPathsToSplit */ ImmutableSet.<String>of(), + /* secondaryJarMetaPath */ "", + /* primaryJarPath */ "", + /* secondaryJarDir */ "", + /* secondaryJarPattern */ "", + /* proguardMappingFile */ Optional.of(proguardMappingFile), + /* primaryDexSubstrings */ ImmutableSet.of("/primary/", "x/"), + Optional.of(primaryDexClassesFile), + ZipSplitter.DexSplitStrategy.MAXIMIZE_PRIMARY_DEX_SIZE, + DexStore.JAR, + /* pathToReportDir */ "", + /* useLinearAllocSplitDex */ true, + /* linearAllocHardLimit */ 4 * 1024 * 1024); + List<String> linesInMappingFile = ImmutableList.of( + "foo.bar.MappedPrimary -> foo.bar.a:", + "foo.bar.MappedSecondary -> foo.bar.b:", + "foo.bar.UnmappedPrimary -> foo.bar.UnmappedPrimary:", + "foo.bar.UnmappedSecondary -> foo.bar.UnmappedSecondary:", + "foo.primary.MappedPackage -> x.a:", + "foo.secondary.MappedPackage -> x.b:", + "foo.primary.UnmappedPackage -> foo.primary.UnmappedPackage:" + ); + List<String> linesInManifestFile = ImmutableList.of( + // Actual primary dex classes. + "foo/bar/MappedPrimary", + "foo/bar/UnmappedPrimary", + // Red herrings! + "foo/bar/b", + "x/b" + ); + + ProjectFilesystem projectFilesystem = EasyMock.createMock(ProjectFilesystem.class); + EasyMock.expect(projectFilesystem.readLines(primaryDexClassesFile)) + .andReturn(linesInManifestFile); + EasyMock.expect(projectFilesystem.readLines(proguardMappingFile)) + .andReturn(linesInMappingFile); + ExecutionContext context = EasyMock.createMock(ExecutionContext.class); + EasyMock.expect(context.getProjectFilesystem()).andReturn(projectFilesystem).anyTimes(); + EasyMock.replay(projectFilesystem, context); + + Predicate<String> requiredInPrimaryZipPredicate = splitZipStep + .createRequiredInPrimaryZipPredicate(context); + assertTrue( + "Mapped class from primary list should be in primary.", + requiredInPrimaryZipPredicate.apply("foo/bar/a.class")); + assertTrue( + "Unmapped class from primary list should be in primary.", + requiredInPrimaryZipPredicate.apply("foo/bar/UnmappedPrimary.class")); + assertTrue( + "Mapped class from substring should be in primary.", + requiredInPrimaryZipPredicate.apply("x/a.class")); + assertTrue( + "Unmapped class from substring should be in primary.", + requiredInPrimaryZipPredicate.apply("foo/primary/UnmappedPackage.class")); + assertFalse( + "Mapped class with obfuscated name match should not be in primary.", + requiredInPrimaryZipPredicate.apply("foo/bar/b.class")); + assertFalse( + "Unmapped class name should not randomly be in primary.", + requiredInPrimaryZipPredicate.apply("foo/bar/UnmappedSecondary.class")); + assertFalse( + "Map class with obfuscated name substring should not be in primary.", + requiredInPrimaryZipPredicate.apply("x/b.class")); + + EasyMock.verify(projectFilesystem, context); + } + + + @Test public void testClassFilePattern() { assertTrue(SplitZipStep.classFilePattern.matcher( "com/facebook/orca/threads/ParticipantInfo.class").matches());