From ad175defe2b626d78a616373b8798821178abb98 Mon Sep 17 00:00:00 2001 From: Alice Sayutina Date: Mon, 5 Feb 2024 08:42:58 +0100 Subject: [PATCH] Jarjar shouldn't mangle classes when no meaningful transformation applied (#52) Fixes #47 In case big module and all it dependent jars are going through jarjar shading, there will probably be dependent jars which are not actually modified by shading. However because they go through jarjar, it unpacks, revisits, and repacks the class back resulting a technically different class from before shading (even class size can be different from original). This upsets classpath checkers (i.e. checking that runtime classpath has at most 1 unique class version). I propose to add a change so that jarjar can detect whether the meaningful transformation to class file has happened and if not, the class is unmodified. This pull request introduces following new concepts: - TracingRemapper: extension of Remapper with capability of tracking if the change was applied; - RemappingJarProcessor: extension of JarProcessor focusing on TracingRemapper-based processors. It assumes ownership of TracingRemapper, and will verify if the mentioned remapper did any transformation. --- .../com/eed3si9n/jarjar/JJProcessor.scala | 3 +- .../com/eed3si9n/jarjar/MainProcessor.java | 4 +- .../jarjar/MethodSignatureProcessor.java | 21 ++++++---- .../com/eed3si9n/jarjar/PackageRemapper.java | 21 +++++++++- .../com/eed3si9n/jarjar/TracingRemapper.java | 15 +++++++ .../com/eed3si9n/jarjar/util/EntryStruct.java | 9 ++++ .../eed3si9n/jarjar/util/JarTransformer.java | 15 +++++-- .../jarjar/util/JarTransformerChain.java | 16 ++++---- .../util/RemappingClassTransformer.java | 13 ++++-- .../jarjar/util/RemappingJarProcessor.java | 41 +++++++++++++++++++ .../com/eed3si9n/jarjar/GenericsTest.java | 5 ++- .../eed3si9n/jarjar/MethodRewriterTest.java | 2 +- .../com/eed3si9n/jarjar/UnmodifiedTest.java | 35 ++++++++++++++++ .../eed3si9n/jarjar/VersionedClassTest.java | 15 ++++--- 14 files changed, 177 insertions(+), 38 deletions(-) create mode 100644 jarjar/src/main/java/com/eed3si9n/jarjar/TracingRemapper.java create mode 100644 jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingJarProcessor.java create mode 100644 jarjar/src/test/java/com/eed3si9n/jarjar/UnmodifiedTest.java diff --git a/core/src/main/scala/com/eed3si9n/jarjar/JJProcessor.scala b/core/src/main/scala/com/eed3si9n/jarjar/JJProcessor.scala index 71c5c449..fbaf7f16 100644 --- a/core/src/main/scala/com/eed3si9n/jarjar/JJProcessor.scala +++ b/core/src/main/scala/com/eed3si9n/jarjar/JJProcessor.scala @@ -52,7 +52,8 @@ class JJProcessor( processors += new ZapProcessor(zapList.asJava) processors += misplacedClassProcessor processors += new JarTransformerChain( - Array[RemappingClassTransformer](new RemappingClassTransformer(pr)) + Array[RemappingClassTransformer](new RemappingClassTransformer()), + pr ) val renamer: String => Option[String] = { diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/MainProcessor.java b/jarjar/src/main/java/com/eed3si9n/jarjar/MainProcessor.java index e6c3bdf2..dc6cf395 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/MainProcessor.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/MainProcessor.java @@ -74,8 +74,8 @@ public MainProcessor(List patterns, boolean verbose, boolean ski processors.add(new ZapProcessor(zapList)); processors.add(misplacedClassProcessor); processors.add(new JarTransformerChain(new RemappingClassTransformer[] { - new RemappingClassTransformer(pr) - })); + new RemappingClassTransformer() + }, pr)); processors.add(new MethodSignatureProcessor(pr)); processors.add(new ResourceProcessor(pr)); chain = new JarProcessorChain(processors.toArray(new JarProcessor[processors.size()])); diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/MethodSignatureProcessor.java b/jarjar/src/main/java/com/eed3si9n/jarjar/MethodSignatureProcessor.java index 7f4cffb5..8eba9796 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/MethodSignatureProcessor.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/MethodSignatureProcessor.java @@ -4,6 +4,8 @@ import java.util.Arrays; import java.util.HashSet; import java.util.Set; + +import com.eed3si9n.jarjar.util.RemappingJarProcessor; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; @@ -20,7 +22,7 @@ * being renamed. Method signatures are more difficult to detect so this class keeps track of which * methods definitely take method signatures and remaps those explicitly. */ -public class MethodSignatureProcessor implements JarProcessor { +public class MethodSignatureProcessor extends RemappingJarProcessor { /** * List of method names which take a method signature as their parameter. @@ -30,14 +32,12 @@ public class MethodSignatureProcessor implements JarProcessor { private static final Set METHOD_NAMES_WITH_PARAMS_TO_REWRITE = new HashSet(Arrays.asList("getImplMethodSignature")); - private final Remapper remapper; - - public MethodSignatureProcessor(Remapper remapper) { - this.remapper = remapper; + public MethodSignatureProcessor(TracingRemapper remapper) { + super(remapper); } @Override - public boolean process(final EntryStruct struct) throws IOException { + public boolean processImpl(EntryStruct struct, Remapper remapper) throws IOException { if (!struct.name.endsWith(".class") || struct.skipTransform) { return true; } @@ -50,12 +50,14 @@ public boolean process(final EntryStruct struct) throws IOException { e.printStackTrace(); return true; } - reader.accept(new MethodSignatureRemapperClassVisitor(classWriter), ClassReader.EXPAND_FRAMES); + reader.accept(new MethodSignatureRemapperClassVisitor(classWriter, remapper), ClassReader.EXPAND_FRAMES); struct.data = classWriter.toByteArray(); return true; } - private class MethodSignatureRemapperClassVisitor extends ClassVisitor { + private static class MethodSignatureRemapperClassVisitor extends ClassVisitor { + + private final Remapper remapper; private class MethodSignatureRemapperMethodVisitor extends MethodVisitor { @@ -96,8 +98,9 @@ public void visitLdcInsn(Object value) { } } - public MethodSignatureRemapperClassVisitor(ClassVisitor classVisitor) { + public MethodSignatureRemapperClassVisitor(ClassVisitor classVisitor, Remapper remapper) { super(Opcodes.ASM9, classVisitor); + this.remapper = remapper; } @Override diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/PackageRemapper.java b/jarjar/src/main/java/com/eed3si9n/jarjar/PackageRemapper.java index 3af7081b..0efaf1c7 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/PackageRemapper.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/PackageRemapper.java @@ -21,7 +21,7 @@ import java.util.*; import java.util.regex.Pattern; -class PackageRemapper extends Remapper +class PackageRemapper extends TracingRemapper { private static final String RESOURCE_SUFFIX = "RESOURCE"; @@ -29,16 +29,25 @@ class PackageRemapper extends Remapper = Pattern.compile("\\[L[\\p{javaJavaIdentifierPart}\\.]+?;"); private final List wildcards; + private final List ruleList; private final Map typeCache = new HashMap(); private final Map pathCache = new HashMap(); private final Map valueCache = new HashMap(); private final boolean verbose; + private boolean modified = false; + public PackageRemapper(List ruleList, boolean verbose) { this.verbose = verbose; + this.ruleList = ruleList; wildcards = PatternElement.createWildcards(ruleList); } + @Override + public TracingRemapper copy() { + return new PackageRemapper(this.ruleList, this.verbose); + } + // also used by KeepProcessor static boolean isArrayForName(String value) { return ARRAY_FOR_NAME_PATTERN.matcher(value).matches(); @@ -130,9 +139,17 @@ public String mapInnerClassName( private String replaceHelper(String value) { for (Wildcard wildcard : wildcards) { String test = wildcard.replace(value); - if (test != null) + if (test != null) { + if (!test.equals(value)) + this.modified = true; return test; + } } return value; } + + @Override + public boolean hasChanges() { + return modified; + } } diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/TracingRemapper.java b/jarjar/src/main/java/com/eed3si9n/jarjar/TracingRemapper.java new file mode 100644 index 00000000..8b6f6a2e --- /dev/null +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/TracingRemapper.java @@ -0,0 +1,15 @@ +package com.eed3si9n.jarjar; + +import org.objectweb.asm.commons.Remapper; + +public abstract class TracingRemapper extends Remapper { + /** + * Obtain the fresh copy of this object. + */ + public abstract TracingRemapper copy(); + + /** + * Check if this instance remapped something already. + */ + public abstract boolean hasChanges(); +} diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/util/EntryStruct.java b/jarjar/src/main/java/com/eed3si9n/jarjar/util/EntryStruct.java index 23d470bc..8e2c029d 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/util/EntryStruct.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/util/EntryStruct.java @@ -21,4 +21,13 @@ public class EntryStruct { public String name; public long time; public boolean skipTransform; + + public EntryStruct copy() { + EntryStruct result = new EntryStruct(); + result.data = data; + result.name = name; + result.time = time; + result.skipTransform = skipTransform; + return result; + } } diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformer.java b/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformer.java index 936774aa..c13ee26a 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformer.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformer.java @@ -19,13 +19,20 @@ import java.io.*; import static com.eed3si9n.jarjar.misplaced.MisplacedClassProcessor.VERSIONED_CLASS_FOLDER; + +import com.eed3si9n.jarjar.TracingRemapper; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.commons.Remapper; + +abstract public class JarTransformer extends RemappingJarProcessor { -abstract public class JarTransformer implements JarProcessor { + public JarTransformer(TracingRemapper remapper) { + super(remapper); + } - public boolean process(EntryStruct struct) throws IOException { + public boolean processImpl(EntryStruct struct, Remapper remapper) throws IOException { if (struct.name.endsWith(".class") && !struct.skipTransform) { ClassReader reader; try { @@ -38,7 +45,7 @@ public boolean process(EntryStruct struct) throws IOException { GetNameClassWriter w = new GetNameClassWriter(ClassWriter.COMPUTE_MAXS); try { - reader.accept(transform(w), ClassReader.EXPAND_FRAMES); + reader.accept(transform(w, remapper), ClassReader.EXPAND_FRAMES); } catch (RuntimeException e) { throw new IOException("Unable to transform " + struct.name, e); } @@ -51,7 +58,7 @@ public boolean process(EntryStruct struct) throws IOException { return true; } - abstract protected ClassVisitor transform(ClassVisitor v); + abstract protected ClassVisitor transform(ClassVisitor v, Remapper remapper); private static String pathFromName(String className) { return className.replace('.', '/') + ".class"; diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformerChain.java b/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformerChain.java index 352fb232..5eb5301c 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformerChain.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/util/JarTransformerChain.java @@ -16,21 +16,23 @@ package com.eed3si9n.jarjar.util; +import com.eed3si9n.jarjar.TracingRemapper; import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.commons.Remapper; public class JarTransformerChain extends JarTransformer { private final RemappingClassTransformer[] chain; - public JarTransformerChain(RemappingClassTransformer[] chain) { + public JarTransformerChain(RemappingClassTransformer[] chain, TracingRemapper remapper) { + super(remapper); this.chain = chain.clone(); - for (int i = chain.length - 1; i > 0; i--) { - chain[i - 1].setTarget(chain[i]); - } } - protected ClassVisitor transform(ClassVisitor v) { - chain[chain.length - 1].setTarget(v); - return chain[0]; + protected ClassVisitor transform(ClassVisitor parent, Remapper remapper) { + for (int i = chain.length - 1; i >= 0; i--) { + parent = chain[i].update(remapper, parent); + } + return parent; } } diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingClassTransformer.java b/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingClassTransformer.java index e7b20240..f844c4bf 100644 --- a/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingClassTransformer.java +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingClassTransformer.java @@ -24,11 +24,16 @@ public class RemappingClassTransformer extends ClassRemapper { - public RemappingClassTransformer(Remapper pr) { + public RemappingClassTransformer() { + this(null, null); + } + + public RemappingClassTransformer(Remapper pr, ClassVisitor parent) { super(new EmptyClassVisitor(), pr); + this.cv = parent; } - - public void setTarget(ClassVisitor target) { - cv = target; + + public RemappingClassTransformer update(Remapper pr, ClassVisitor target) { + return new RemappingClassTransformer(pr, target); } } diff --git a/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingJarProcessor.java b/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingJarProcessor.java new file mode 100644 index 00000000..288fa308 --- /dev/null +++ b/jarjar/src/main/java/com/eed3si9n/jarjar/util/RemappingJarProcessor.java @@ -0,0 +1,41 @@ +package com.eed3si9n.jarjar.util; + + +import com.eed3si9n.jarjar.TracingRemapper; +import org.objectweb.asm.commons.Remapper; + +import java.io.IOException; + +/** + * Extension of JarProcessor for building processors around TracingRemapper. + * + * This class will track if TracingRemapper recorded doing any changes, and if it didn't, + * it will make sure that the underlying bytes are unmodified. + * + * This helps to avoid bytecode modifications when there was no actual changes + * to do. + */ +public abstract class RemappingJarProcessor implements JarProcessor { + private TracingRemapper remapper; + + public RemappingJarProcessor(TracingRemapper remapper) { + this.remapper = remapper; + } + + public final boolean process(EntryStruct struct) throws IOException { + TracingRemapper structRemapper = remapper.copy(); + EntryStruct origStruct = struct.copy(); + + if (!processImpl(struct, structRemapper)) { + return false; + } + + if (!structRemapper.hasChanges()) { + struct.data = origStruct.data; + } + + return true; + } + + protected abstract boolean processImpl(EntryStruct struct, Remapper remapper) throws IOException; +} \ No newline at end of file diff --git a/jarjar/src/test/java/com/eed3si9n/jarjar/GenericsTest.java b/jarjar/src/test/java/com/eed3si9n/jarjar/GenericsTest.java index 2cfb02fd..4f640983 100644 --- a/jarjar/src/test/java/com/eed3si9n/jarjar/GenericsTest.java +++ b/jarjar/src/test/java/com/eed3si9n/jarjar/GenericsTest.java @@ -32,8 +32,9 @@ public void testTransform() throws Exception { Rule rule = new Rule(); rule.setPattern("java.lang.String"); rule.setResult("com.tonicsystems.String"); - RemappingClassTransformer t = new RemappingClassTransformer(new PackageRemapper(Arrays.asList(rule), false)); - t.setTarget(new EmptyClassVisitor()); + RemappingClassTransformer t = new RemappingClassTransformer( + new PackageRemapper(Arrays.asList(rule), false), new EmptyClassVisitor() + ); ClassReader reader = new ClassReader(getClass().getResourceAsStream("/Generics.class")); reader.accept(t, 0); } diff --git a/jarjar/src/test/java/com/eed3si9n/jarjar/MethodRewriterTest.java b/jarjar/src/test/java/com/eed3si9n/jarjar/MethodRewriterTest.java index 6b9e07a2..df03978c 100644 --- a/jarjar/src/test/java/com/eed3si9n/jarjar/MethodRewriterTest.java +++ b/jarjar/src/test/java/com/eed3si9n/jarjar/MethodRewriterTest.java @@ -46,7 +46,7 @@ public MethodVisitor visitMethod( } } - private static byte[] readInputStream(InputStream inputStream) throws IOException { + protected static byte[] readInputStream(InputStream inputStream) throws IOException { byte[] buf = new byte[0x2000]; ByteArrayOutputStream baos = new ByteArrayOutputStream(); IoUtil.pipe(inputStream, baos, buf); diff --git a/jarjar/src/test/java/com/eed3si9n/jarjar/UnmodifiedTest.java b/jarjar/src/test/java/com/eed3si9n/jarjar/UnmodifiedTest.java new file mode 100644 index 00000000..d32d8a8f --- /dev/null +++ b/jarjar/src/test/java/com/eed3si9n/jarjar/UnmodifiedTest.java @@ -0,0 +1,35 @@ +package com.eed3si9n.jarjar; + +import com.eed3si9n.jarjar.util.EntryStruct; +import org.junit.Assert; +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +import static com.eed3si9n.jarjar.MethodRewriterTest.readInputStream; + +public class UnmodifiedTest { + @Test + public void testNotModified() throws Exception { + Rule rule = new Rule(); + rule.setPattern("com.abc"); + rule.setResult("com.def"); + + MainProcessor mp = new MainProcessor(Arrays.asList(rule), false, false, "move"); + + EntryStruct entryStruct = new EntryStruct(); + entryStruct.name = "BigtableIO$Write.class"; + entryStruct.skipTransform = false; + entryStruct.time = 0; + entryStruct.data = + readInputStream( + getClass().getResourceAsStream("/com/eed3si9n/jarjar/BigtableIO$Write.class")); + + EntryStruct orig = entryStruct.copy(); + + mp.process(entryStruct); + + Assert.assertEquals(entryStruct.data, orig.data); + } +} \ No newline at end of file diff --git a/jarjar/src/test/java/com/eed3si9n/jarjar/VersionedClassTest.java b/jarjar/src/test/java/com/eed3si9n/jarjar/VersionedClassTest.java index 93e53ce1..36110bdf 100644 --- a/jarjar/src/test/java/com/eed3si9n/jarjar/VersionedClassTest.java +++ b/jarjar/src/test/java/com/eed3si9n/jarjar/VersionedClassTest.java @@ -9,6 +9,7 @@ import org.junit.Test; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.commons.ClassRemapper; +import org.objectweb.asm.commons.Remapper; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -37,13 +38,15 @@ public void testVersionPrefixIsPreserved() throws IOException { String expectedName = Example.class.getName().replace("jarjar", "jarjarabrams"); struct.name = MisplacedClassProcessor.VERSIONED_CLASS_FOLDER + "9/" + toClassPath(originalName); struct.data = getClassBytes(originalName); - JarTransformer transformer = new JarTransformer() { + + Rule rule = new Rule(); + rule.setPattern(originalName); + rule.setResult(expectedName); + PackageRemapper remapper = new PackageRemapper(Arrays.asList(rule), false); + + JarTransformer transformer = new JarTransformer(remapper) { @Override - protected ClassVisitor transform(ClassVisitor v) { - Rule rule = new Rule(); - rule.setPattern(originalName); - rule.setResult(expectedName); - PackageRemapper remapper = new PackageRemapper(Arrays.asList(rule), false); + protected ClassVisitor transform(ClassVisitor v, Remapper remapper) { return new ClassRemapper(v, remapper); } };