prefer popping and removing useless pops in UselessPopCleanTransformer

narumii · Sep 7, 2024 · 1dc8834 · 1dc8834
1 parent 524679e
commit 1dc8834
Show file tree

Hide file tree

Showing 12 changed files with 82 additions and 46 deletions.
diff --git a/deobfuscator-api/src/main/java/org/objectweb/asm/tree/analysis/OriginalSourceValue.java b/deobfuscator-api/src/main/java/org/objectweb/asm/tree/analysis/OriginalSourceValue.java
@@ -4,6 +4,9 @@
 import org.objectweb.asm.Type;
 import org.objectweb.asm.tree.AbstractInsnNode;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -81,6 +84,11 @@ public class OriginalSourceValue extends SourceValue {
   @Nullable
   private ConstantValue constantValue = null;
 
+  /**
+   * Children that copied this source value
+   */
+  private final List<OriginalSourceValue> children = new ArrayList<>();
+
   public OriginalSourceValue(int size) {
     this(size, new SmallSet<>());
   }
@@ -118,6 +126,12 @@ public OriginalSourceValue(int size, Set<AbstractInsnNode> insnSet, @Nullable Or
     super(size, insnSet);
     this.copiedFrom = copiedFrom;
     this.originalSource = copiedFrom == null ? this : copiedFrom.originalSource;
+
+    if (copiedFrom != null) {
+      // Add child
+      copiedFrom.addChild(this);
+    }
+
     if (constantValue != null) {
       // If the constant value is present, then use it
       this.constantValue = constantValue;
@@ -162,6 +176,14 @@ public ConstantValue getConstantValue() {
     return constantValue;
   }
 
+  void addChild(OriginalSourceValue child) {
+    this.children.add(child);
+  }
+
+  public List<OriginalSourceValue> getChildren() {
+    return Collections.unmodifiableList(children);
+  }
+
   /**
    * Walk to the last parent value until the predicate returns true.
    *

diff --git a/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/asm/InstructionContext.java b/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/asm/InstructionContext.java
@@ -1,6 +1,8 @@
 package uwu.narumi.deobfuscator.api.asm;
 
+import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.tree.AbstractInsnNode;
+import org.objectweb.asm.tree.InsnNode;
 import org.objectweb.asm.tree.MethodNode;
 import org.objectweb.asm.tree.analysis.Frame;
 import org.objectweb.asm.tree.analysis.OriginalSourceValue;
@@ -27,4 +29,20 @@ public Frame<OriginalSourceValue> frame() {
   public MethodNode methodNode() {
     return this.methodContext.methodNode();
   }
+
+  /**
+   * Adds POP instruction to pop current instruction.
+   *
+   * @param count Stack values count to pop
+   */
+  public void pop(int count) {
+    for (int i = 0; i < count; i++) {
+      int stackValueIdx = frame().getStackSize() - (i + 1);
+      OriginalSourceValue sourceValue = frame().getStack(stackValueIdx);
+
+      // Pop
+      InsnNode popInsn = sourceValue.getSize() == 2 ? new InsnNode(Opcodes.POP2) : new InsnNode(Opcodes.POP);
+      this.methodNode().instructions.insertBefore(this.insn, popInsn);
+    }
+  }
 }
diff --git a/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/helper/AsmHelper.java b/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/helper/AsmHelper.java
@@ -168,21 +168,6 @@ public static Map<AbstractInsnNode, Frame<OriginalSourceValue>> analyzeSource(
     return Collections.unmodifiableMap(frames);
   }
 
-  /**
-   * Removes values from the stack. You can only remove stack values that are one way produced.
-   *
-   * @param count How many values to remove from top
-   */
-  public static void removeValuesFromStack(MethodNode methodNode, Frame<OriginalSourceValue> frame, int count) {
-    for (int i = 0; i < count; i++) {
-      int stackValueIdx = frame.getStackSize() - (i + 1);
-
-      OriginalSourceValue sourceValue = frame.getStack(stackValueIdx);
-      // Remove
-      methodNode.instructions.remove(sourceValue.getProducer());
-    }
-  }
-
   /**
    * Convert constant value to instruction that represents this constant
    *

diff --git a/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/helper/AsmMathHelper.java b/deobfuscator-api/src/main/java/uwu/narumi/deobfuscator/api/helper/AsmMathHelper.java
@@ -130,11 +130,11 @@ public final class AsmMathHelper {
             AbstractInsnNode originalInsn = originalSourceValue.getProducer();
             if (!originalInsn.isString()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(originalInsn.asString().length())
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -152,11 +152,11 @@ public final class AsmMathHelper {
             AbstractInsnNode originalInsn = originalSourceValue.getProducer();
             if (!originalInsn.isString()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(originalInsn.asString().hashCode())
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -175,11 +175,11 @@ public final class AsmMathHelper {
             // Integer#parseInt(String)
             if (!originalInsn.isString()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Integer.parseInt(originalInsn.asString()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -202,14 +202,13 @@ public final class AsmMathHelper {
             // Integer#parseInt(String, int)
             if (!originalFirstInsn.isString() || !originalSecondInsn.isInteger()) return false;
 
+            context.pop(2);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(
                     Integer.parseInt(originalFirstInsn.asString(), originalSecondInsn.asInteger())
                 )
             );
-            context.methodNode().instructions.remove(firstValue.getProducer());
-            context.methodNode().instructions.remove(secondValue.getProducer());
             return true;
           });
 
@@ -228,11 +227,11 @@ public final class AsmMathHelper {
             // Integer#reverse(int)
             if (!originalInsn.isInteger()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Integer.reverse(originalInsn.asInteger()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -251,11 +250,11 @@ public final class AsmMathHelper {
             // Long#reverse(long)
             if (!originalInsn.isLong()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Long.reverse(originalInsn.asLong()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -274,11 +273,11 @@ public final class AsmMathHelper {
             // Float#floatToIntBits(float)
             if (!originalInsn.isFloat()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Float.floatToIntBits(originalInsn.asFloat()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -297,11 +296,11 @@ public final class AsmMathHelper {
             // Float#intBitsToFloat(int)
             if (!originalInsn.isInteger()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Float.intBitsToFloat(originalInsn.asInteger()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -320,11 +319,11 @@ public final class AsmMathHelper {
             // Double#doubleToLongBits(double)
             if (!originalInsn.isDouble()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Double.doubleToLongBits(originalInsn.asDouble()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 
@@ -343,11 +342,11 @@ public final class AsmMathHelper {
             // Double#longBitsToDouble(long)
             if (!originalInsn.isLong()) return false;
 
+            context.pop(1);
             context.methodNode().instructions.set(
                 context.insn(),
                 AsmHelper.getNumber(Double.longBitsToDouble(originalInsn.asLong()))
             );
-            context.methodNode().instructions.remove(sourceValue.getProducer());
             return true;
           });
 

diff --git a/...arumi/deobfuscator/core/other/impl/clean/peephole/PopUnUsedLocalVariablesTransformer.java b/...arumi/deobfuscator/core/other/impl/clean/peephole/PopUnUsedLocalVariablesTransformer.java
@@ -59,6 +59,6 @@ protected void transform(ClassWrapper scope, Context context) throws Exception {
       }
     }));
 
-    LOGGER.info("Removed {} unused local variables", this.getChangesCount());
+    LOGGER.info("Popped {} unused local variables", this.getChangesCount());
   }
 }
diff --git a/...va/uwu/narumi/deobfuscator/core/other/impl/clean/peephole/UselessPopCleanTransformer.java b/...va/uwu/narumi/deobfuscator/core/other/impl/clean/peephole/UselessPopCleanTransformer.java
@@ -9,9 +9,11 @@
 
 import java.util.stream.Stream;
 
-// TODO: Remove pair of DUP and POP
-// TODO: Account for DUP2_X1, DUP2_X2, DUP_X2, etc.
+// TODO: Handle DUP2
 public class UselessPopCleanTransformer extends FramedInstructionsTransformer {
+  public UselessPopCleanTransformer() {
+    this.rerunOnChange = true;
+  }
 
   @Override
   protected Stream<AbstractInsnNode> buildInstructionsStream(Stream<AbstractInsnNode> stream) {
@@ -58,14 +60,28 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
     return shouldRemovePop;
   }
 
+  private boolean isSourceValueRemovable(OriginalSourceValue sourceValue) {
+    if (sourceValue.insns.isEmpty()) {
+      // Nothing to remove. Probably a local variable
+      return false;
+    }
+    if (!sourceValue.getChildren().isEmpty()) {
+      // Other source values depends on this source value
+      // Also there is a small bug https://gitlab.ow2.org/asm/asm/-/merge_requests/414
+      return false;
+    }
+
+    return true;
+  }
+
   /**
    * Checks if all producers of the source value are constants
    */
   private boolean areProducersConstant(OriginalSourceValue sourceValue) {
-    if (sourceValue.insns.isEmpty()) return false;
+    if (!isSourceValueRemovable(sourceValue)) return false;
 
     for (AbstractInsnNode producer : sourceValue.insns) {
-      if (!producer.isConstant()) {
+      if (!(producer.isConstant() || producer.getOpcode() == DUP)) {
         return false;
       }
     }
@@ -76,7 +92,7 @@ private boolean areProducersConstant(OriginalSourceValue sourceValue) {
    * Checks if all producers of the source value are 2-sized values
    */
   private boolean areTwoSizedValues(OriginalSourceValue sourceValue) {
-    if (sourceValue.insns.isEmpty()) return false;
+    if (!isSourceValueRemovable(sourceValue)) return false;
 
     for (AbstractInsnNode producer : sourceValue.insns) {
       if (producer.sizeOnStack() != 2) {

diff --git a/...in/java/uwu/narumi/deobfuscator/core/other/impl/pool/InlineLocalVariablesTransformer.java b/...in/java/uwu/narumi/deobfuscator/core/other/impl/pool/InlineLocalVariablesTransformer.java
@@ -13,10 +13,6 @@
  * Inlines constant local variables
  */
 public class InlineLocalVariablesTransformer extends FramedInstructionsTransformer {
-  public InlineLocalVariablesTransformer() {
-    this.rerunOnChange = true;
-  }
-
   @Override
   protected Stream<AbstractInsnNode> buildInstructionsStream(Stream<AbstractInsnNode> stream) {
     return stream

diff --git a/...in/java/uwu/narumi/deobfuscator/core/other/impl/universal/UniversalNumberTransformer.java b/...in/java/uwu/narumi/deobfuscator/core/other/impl/universal/UniversalNumberTransformer.java
@@ -1,6 +1,7 @@
 package uwu.narumi.deobfuscator.core.other.impl.universal;
 
 import uwu.narumi.deobfuscator.api.transformer.ComposedTransformer;
+import uwu.narumi.deobfuscator.core.other.impl.clean.peephole.UselessPopCleanTransformer;
 import uwu.narumi.deobfuscator.core.other.impl.universal.number.MathBinaryOperationsTransformer;
 import uwu.narumi.deobfuscator.core.other.impl.universal.number.MethodCallsOnLiteralsTransformer;
 import uwu.narumi.deobfuscator.core.other.impl.universal.number.MathUnaryOperationsTransformer;
@@ -13,7 +14,9 @@ public UniversalNumberTransformer() {
     super(
         MethodCallsOnLiteralsTransformer::new,
         MathBinaryOperationsTransformer::new,
-        MathUnaryOperationsTransformer::new
+        MathUnaryOperationsTransformer::new,
+
+        UselessPopCleanTransformer::new
     );
 
     this.rerunOnChange = true;

diff --git a/...wu/narumi/deobfuscator/core/other/impl/universal/flow/CleanRedundantJumpsTransformer.java b/...wu/narumi/deobfuscator/core/other/impl/universal/flow/CleanRedundantJumpsTransformer.java
@@ -4,7 +4,6 @@
 import org.objectweb.asm.tree.MethodNode;
 import uwu.narumi.deobfuscator.api.asm.InstructionContext;
 import uwu.narumi.deobfuscator.api.context.Context;
-import uwu.narumi.deobfuscator.api.helper.AsmHelper;
 import uwu.narumi.deobfuscator.api.helper.AsmMathHelper;
 import uwu.narumi.deobfuscator.api.transformer.FramedInstructionsTransformer;
 
@@ -22,9 +21,9 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
     boolean ifResult = optIfResult.get();
 
     if (AsmMathHelper.isOneValueCondition(jumpInsn.getOpcode())) {
-      AsmHelper.removeValuesFromStack(insnContext.methodNode(), insnContext.frame(), 1);
+      insnContext.pop(1);
     } else if (AsmMathHelper.isTwoValuesCondition(jumpInsn.getOpcode())) {
-      AsmHelper.removeValuesFromStack(insnContext.methodNode(), insnContext.frame(), 2);
+      insnContext.pop(2);
     }
 
     // Replace if with corresponding GOTO or remove it

diff --git a/...narumi/deobfuscator/core/other/impl/universal/flow/CleanRedundantSwitchesTransformer.java b/...narumi/deobfuscator/core/other/impl/universal/flow/CleanRedundantSwitchesTransformer.java
@@ -6,7 +6,6 @@
 import org.objectweb.asm.tree.TableSwitchInsnNode;
 import uwu.narumi.deobfuscator.api.asm.InstructionContext;
 import uwu.narumi.deobfuscator.api.context.Context;
-import uwu.narumi.deobfuscator.api.helper.AsmHelper;
 import uwu.narumi.deobfuscator.api.helper.AsmMathHelper;
 import uwu.narumi.deobfuscator.api.transformer.FramedInstructionsTransformer;
 
@@ -27,7 +26,7 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
 
       LabelNode predictedJump = optPredictedJump.get();
       // Remove value from stack
-      AsmHelper.removeValuesFromStack(insnContext.methodNode(), insnContext.frame(), 1);
+      insnContext.pop(1);
       // Replace lookup switch with predicted jump
       insnContext.methodNode().instructions.set(lookupSwitchInsn, new JumpInsnNode(GOTO, predictedJump));
     } else if (insnContext.insn().getOpcode() == TABLESWITCH) {
@@ -38,7 +37,7 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
 
       LabelNode predictedJump = optPredictedJump.get();
       // Remove value from stack
-      AsmHelper.removeValuesFromStack(insnContext.methodNode(), insnContext.frame(), 1);
+      insnContext.pop(1);
       // Replace lookup switch with predicted jump
       insnContext.methodNode().instructions.set(tableSwitchInsn, new JumpInsnNode(GOTO, predictedJump));
     }

diff --git a/...narumi/deobfuscator/core/other/impl/universal/number/MathBinaryOperationsTransformer.java b/...narumi/deobfuscator/core/other/impl/universal/number/MathBinaryOperationsTransformer.java
@@ -43,9 +43,8 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
         return false;
       }
 
+      insnContext.pop(2);
       insnContext.methodNode().instructions.set(insnContext.insn(), AsmHelper.getNumber(result));
-      insnContext.methodNode().instructions.remove(value1SourceValue.getProducer());
-      insnContext.methodNode().instructions.remove(value2SourceValue.getProducer());
 
       return true;
     }

diff --git a/.../narumi/deobfuscator/core/other/impl/universal/number/MathUnaryOperationsTransformer.java b/.../narumi/deobfuscator/core/other/impl/universal/number/MathUnaryOperationsTransformer.java
@@ -32,8 +32,8 @@ protected boolean transformInstruction(Context context, InstructionContext insnC
     if (valueInsn.isNumber()) {
       Number castedNumber = AsmMathHelper.mathUnaryOperation(valueInsn.asNumber(), insnContext.insn().getOpcode());
 
+      insnContext.pop(1);
       insnContext.methodNode().instructions.set(insnContext.insn(), AsmHelper.getNumber(castedNumber));
-      insnContext.methodNode().instructions.remove(sourceValue.getProducer());
 
       return true;
     }
-Original file line number
+Diff line change
@@ Expand Up @@
           }
         }));
-        LOGGER.info("Removed {} unused local variables", this.getChangesCount());
+        LOGGER.info("Popped {} unused local variables", this.getChangesCount());
       }
     }