Jarjar shouldn't mangle classes when no meaningful transformation app…

…lied (#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.
eed3si9n · Feb 5, 2024 · ad175de · ad175de
1 parent 9d0eb03
commit ad175de
Show file tree

Hide file tree

Showing 14 changed files with 177 additions and 38 deletions.
diff --git 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
@@ -74,8 +74,8 @@ public MainProcessor(List<PatternElement> 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
@@ -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<String> METHOD_NAMES_WITH_PARAMS_TO_REWRITE =
       new HashSet<String>(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
@@ -21,24 +21,33 @@
 import java.util.*;
 import java.util.regex.Pattern;
 
-class PackageRemapper extends Remapper
+class PackageRemapper extends TracingRemapper
 {
     private static final String RESOURCE_SUFFIX = "RESOURCE";
 
     private static final Pattern ARRAY_FOR_NAME_PATTERN
         = Pattern.compile("\\[L[\\p{javaJavaIdentifierPart}\\.]+?;");
 
     private final List<Wildcard> wildcards;
+    private final List<Rule> ruleList;
     private final Map<String, String> typeCache = new HashMap<String, String>();
     private final Map<String, String> pathCache = new HashMap<String, String>();
     private final Map<Object, String> valueCache = new HashMap<Object, String>();
     private final boolean verbose;
+    private boolean modified = false;
+
 
     public PackageRemapper(List<Rule> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
+}
diff --git 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
@@ -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
@@ -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);
+    }
+}
diff --git 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);
             }
         };