From 4b2b846e6e3cfc9d387a839235f5303abdab9875 Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 19 Apr 2024 05:51:59 -0400 Subject: [PATCH 1/4] Make entry-point summarizer lookup latest class instance instead of always pointing to the original --- .../info/summary/builtin/EntryPointSummarizer.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/recaf-ui/src/main/java/software/coley/recaf/services/info/summary/builtin/EntryPointSummarizer.java b/recaf-ui/src/main/java/software/coley/recaf/services/info/summary/builtin/EntryPointSummarizer.java index bf4581720..df635fc59 100644 --- a/recaf-ui/src/main/java/software/coley/recaf/services/info/summary/builtin/EntryPointSummarizer.java +++ b/recaf-ui/src/main/java/software/coley/recaf/services/info/summary/builtin/EntryPointSummarizer.java @@ -7,6 +7,8 @@ import javafx.geometry.Insets; import javafx.scene.Node; import javafx.scene.control.Label; +import software.coley.recaf.info.ClassInfo; +import software.coley.recaf.info.JvmClassInfo; import software.coley.recaf.info.member.MethodMember; import software.coley.recaf.services.cell.icon.IconProviderService; import software.coley.recaf.services.cell.text.TextProviderService; @@ -19,6 +21,8 @@ import software.coley.recaf.workspace.model.resource.WorkspaceResource; import java.util.List; +import java.util.Objects; +import java.util.function.Supplier; import static java.lang.reflect.Modifier.PUBLIC; import static java.lang.reflect.Modifier.STATIC; @@ -58,16 +62,16 @@ public boolean summarize(@Nonnull Workspace workspace, List entryMethods = cls.getMethods().stream() .filter(this::isJvmEntry) .toList(); - // TODO: The 'gotoDeclaration' usage here will always hold the original reference - // - If a user makes a change to the class, these links will not point to the updated class if (!entryMethods.isEmpty()) { + Supplier classLookup = () -> Objects.requireNonNullElse(bundle.get(cls.getName()), cls); + // Add entry for class String classDisplay = textService.getJvmClassInfoTextProvider(workspace, resource, bundle, cls).makeText(); Node classIcon = iconService.getJvmClassInfoIconProvider(workspace, resource, bundle, cls).makeIcon(); Label classLabel = new Label(classDisplay, classIcon); classLabel.setOnMouseEntered(e -> classLabel.getStyleClass().add(Styles.TEXT_UNDERLINED)); classLabel.setOnMouseExited(e -> classLabel.getStyleClass().remove(Styles.TEXT_UNDERLINED)); - classLabel.setOnMouseClicked(e -> actions.gotoDeclaration(workspace, resource, bundle, cls)); + classLabel.setOnMouseClicked(e -> actions.gotoDeclaration(workspace, resource, bundle, classLookup.get())); consumer.appendSummary(classLabel); // Add entries for methods @@ -80,7 +84,7 @@ public boolean summarize(@Nonnull Workspace workspace, methodLabel.setOnMouseEntered(e -> methodLabel.getStyleClass().add(Styles.TEXT_UNDERLINED)); methodLabel.setOnMouseExited(e -> methodLabel.getStyleClass().remove(Styles.TEXT_UNDERLINED)); methodLabel.setOnMouseClicked(e -> { - actions.gotoDeclaration(workspace, resource, bundle, cls) + actions.gotoDeclaration(workspace, resource, bundle, classLookup.get()) .requestFocus(method); }); consumer.appendSummary(methodLabel); From da04dc259786611a35078d82e239008e5b336c27 Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 19 Apr 2024 06:53:38 -0400 Subject: [PATCH 2/4] Update jasm to 2.4.1 snapshot --- gradle/libs.versions.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index ac35129b3..eb25d8501 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -17,7 +17,7 @@ ikonli = "12.3.1" instrument-server = "1.4.0" jackson = "2.16.1" jakarta-annotation = "3.0.0-M1" -jasm = "cceee82338" +jasm = "1246e379c5" jlinker = "1.0.7" jphantom = "1.4.4" junit = "5.10.2" From 127d53c87702626efbe3069a04127821ad5c3184 Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 19 Apr 2024 07:37:06 -0400 Subject: [PATCH 3/4] Improve expression compiler's handling of obfuscated contexts --- .../assembler/ExpressionCompiler.java | 107 +++++++++++++++--- .../java/software/coley/recaf/util/Types.java | 8 +- .../assembler/ExpressionCompilerTest.java | 72 ++++++++++++ 3 files changed, 169 insertions(+), 18 deletions(-) diff --git a/recaf-core/src/main/java/software/coley/recaf/services/assembler/ExpressionCompiler.java b/recaf-core/src/main/java/software/coley/recaf/services/assembler/ExpressionCompiler.java index 210e3b334..e145ab531 100644 --- a/recaf-core/src/main/java/software/coley/recaf/services/assembler/ExpressionCompiler.java +++ b/recaf-core/src/main/java/software/coley/recaf/services/assembler/ExpressionCompiler.java @@ -38,14 +38,12 @@ @WorkspaceScoped public class ExpressionCompiler { private static final Pattern IMPORT_EXTRACT_PATTERN = RegexUtil.pattern("^\\s*(import \\w.+;)"); - private static final Pattern WORD_PATTERN = RegexUtil.pattern("\\w+"); - private static final Pattern WORD_DOT_PATTERN = RegexUtil.pattern("[\\w.]+"); private static final String EXPR_MARKER = "/* EXPR_START */"; private final JavacCompiler javac; private final Workspace workspace; private final AssemblerPipelineGeneralConfig assemblerConfig; - private String className; private int classAccess; + private String className; private String superName; private List implementing; private int versionTarget; @@ -58,7 +56,7 @@ public class ExpressionCompiler { @Inject public ExpressionCompiler(@Nonnull Workspace workspace, @Nonnull JavacCompiler javac, - @Nonnull AssemblerPipelineGeneralConfig assemblerConfig) { + @Nonnull AssemblerPipelineGeneralConfig assemblerConfig) { this.workspace = workspace; this.javac = javac; this.assemblerConfig = assemblerConfig; @@ -90,11 +88,15 @@ public void clearContext() { * Class to pull info from. */ public void setClassContext(@Nonnull JvmClassInfo classInfo) { - className = classInfo.getName(); + String type = classInfo.getName(); + String superType = classInfo.getSuperName(); + className = isSafeInternalClassName(type) ? type : "obfuscated_class"; classAccess = classInfo.getAccess(); versionTarget = NumberUtil.intClamp(classInfo.getVersion() - JvmClassInfo.BASE_VERSION, JavacCompiler.getMinTargetVersion(), JavaVersion.get()); - superName = classInfo.getSuperName(); - implementing = classInfo.getInterfaces(); + superName = superType != null && isSafeInternalClassName(superType) ? superType : null; + implementing = classInfo.getInterfaces().stream() + .filter(ExpressionCompiler::isSafeInternalClassName) + .toList(); fields = classInfo.getFields(); methods = classInfo.getMethods(); @@ -226,7 +228,7 @@ private String generateClass(@Nonnull String expression) throws ExpressionCompil code.append(" implements ").append(implementing.stream().map(s -> s.replace('/', '.')).collect(Collectors.joining(", "))).append(' '); code.append("{\n"); - // Enum constants must come first if the class is an enum + // Enum constants must come first if the class is an enum. if (isEnum) { int enumConsts = 0; for (FieldMember field : fields) { @@ -243,7 +245,8 @@ private String generateClass(@Nonnull String expression) throws ExpressionCompil code.append(';'); } - // Method structure to house the expression + // Need to build the method structure to house the expression. + // We'll start off with the access level. int parameterVarIndex = 0; if (AccessFlag.isPublic(methodFlags)) code.append("public "); @@ -255,6 +258,8 @@ else if (AccessFlag.isPrivate(methodFlags)) code.append("static "); else parameterVarIndex++; + + // Add the return type. ClassType returnType = methodType.returnType(); if (returnType instanceof PrimitiveType primitiveReturn) { code.append(primitiveReturn.name()).append(' '); @@ -269,31 +274,56 @@ else if (AccessFlag.isPrivate(methodFlags)) } code.append("[]".repeat(arrayReturn.dimensions())); } + + // Now the method name. code.append(' ').append(methodName).append('('); + + // And now the parameters. int parameterCount = methodType.parameterTypes().size(); Set usedVariables = new HashSet<>(); for (int i = 0; i < parameterCount; i++) { + // Lookup the parameter variable LocalVariable parameterVariable = getParameterVariable(parameterVarIndex, i); String parameterName = parameterVariable.getName(); + + // Record the parameter as being used usedVariables.add(parameterName); + + // Skip if the parameter is illegally named. + if (!isSafeName(parameterName)) + continue; + + // Append the parameter. NameType varInfo = getInfo(parameterName, parameterVariable.getDescriptor()); parameterVarIndex += varInfo.size; code.append(varInfo.className).append(' ').append(varInfo.name); if (i < parameterCount - 1) code.append(", "); - } for (LocalVariable variable : methodVariables) { String name = variable.getName(); + + // Skip illegal named variables and the implicit 'this' if (!isSafeName(name) || name.equals("this")) continue; + + // Skip if we already included the parameter in the loop above. boolean hasPriorParameters = !usedVariables.isEmpty(); if (!usedVariables.add(name)) continue; + + // Append the parameter. NameType varInfo = getInfo(name, variable.getDescriptor()); if (hasPriorParameters) code.append(", "); code.append(varInfo.className).append(' ').append(varInfo.name); } + + // If we skipped the last parameter for some reason we need to remove the trailing ', ' before closing + // off the parameters section. + if (code.substring(code.length() - 2).endsWith(", ")) + code.setLength(code.length() - 2); + + // Close off declaration and add a throws so the user doesn't need to specify try-catch. code.append(") throws Throwable { " + EXPR_MARKER + " \n"); code.append(expression); code.append("}\n"); @@ -312,6 +342,7 @@ else if (AccessFlag.isPrivate(methodFlags)) if (fieldInfo.className.equals(className.replace('/', '.')) && field.hasFinalModifier() && field.hasStaticModifier()) continue; + // Append the field. The only modifier that we care about here is if it is static or not. if (field.hasStaticModifier()) code.append("static "); code.append(fieldInfo.className).append(' ').append(fieldInfo.name).append(";\n"); @@ -352,7 +383,7 @@ else if (AccessFlag.isPrivate(methodFlags)) }).allMatch(ExpressionCompiler::isSafeClassName)) continue; - // Stub the method + // Stub the method. Start with the access modifiers. if (method.hasPublicModifier()) code.append("public "); else if (method.hasProtectedModifier()) @@ -362,10 +393,13 @@ else if (method.hasPrivateModifier()) if (method.hasStaticModifier()) code.append("static "); + // Method name. Consider edge case for constructors. if (isCtor) code.append(StringUtil.shortenPath(className)).append('('); else code.append(returnInfo.className).append(' ').append(returnInfo.name).append('('); + + // Add the parameters. We only care about the types, names don't really matter. List methodParameterTypes = localMethodType.parameterTypes(); parameterCount = methodParameterTypes.size(); for (int i = 0; i < parameterCount; i++) { @@ -384,6 +418,8 @@ else if (method.hasPrivateModifier()) "missing type information for: " + superName); if (superPath != null) { // To make it easy, we'll find the simplest constructor in the parent class and pass dummy values. + // Unlike regular methods we cannot just say 'throw new RuntimeException();' since calling + // the 'super(...)' is required. MethodType parentConstructor = superPath.getValue().methodStream() .filter(m -> m.getName().equals("")) .map(m -> Types.methodType(m.getDescriptor())) @@ -524,6 +560,8 @@ private String methodDescriptorWithVariables() throws ExpressionCompileException LocalVariable parameterVariable = getParameterVariable(parameterVarIndex, i); String parameterName = parameterVariable.getName(); usedVariables.add(parameterName); + if (!isSafeName(parameterName)) + continue; NameType varInfo = getInfo(parameterName, parameterVariable.getDescriptor()); parameterVarIndex += varInfo.size; sb.append(parameterVariable.getDescriptor()); @@ -547,17 +585,58 @@ private String methodDescriptorWithVariables() throws ExpressionCompileException * @return {@code true} when it can be used as a variable name safely. */ private static boolean isSafeName(@Nonnull String name) { - return WORD_PATTERN.matches(name); + // Name must not be empty. + if (name.isEmpty()) + return false; + + // Must be comprised of valid identifier characters. + char first = name.charAt(0); + if (!Character.isJavaIdentifierStart(first)) + return false; + char[] chars = name.toCharArray(); + for (int i = 1; i < chars.length; i++) { + if (!Character.isJavaIdentifierPart(chars[i])) + return false; + } + + // Cannot be a reserved keyword. + return !Keywords.getKeywords().contains(name); + } + + /** + * @param internalName + * Name to check. Expected to be in the internal format. IE {@code java/lang/String}. + * + * @return {@code true} when it can be used as a class name safely. + */ + private static boolean isSafeInternalClassName(@Nonnull String internalName) { + // Sanity check input + if (internalName.indexOf('.') >= 0) + throw new IllegalStateException("Saw source name format, expected internal name format"); + + // All package name portions and the class name must be valid names. + return StringUtil.fastSplit(internalName, true, '/').stream() + .allMatch(ExpressionCompiler::isSafeName); } /** * @param name - * Name to check. + * Name to check. Expected to be in the source format. IE {@code java.lang.String}. * * @return {@code true} when it can be used as a class name safely. */ private static boolean isSafeClassName(@Nonnull String name) { - return WORD_DOT_PATTERN.matches(name); + // Sanity check input + if (name.indexOf('/') >= 0) + throw new IllegalStateException("Saw internal name format, expected source name format"); + + // Allow primitives + if (software.coley.recaf.util.Types.isPrimitiveClassName(name)) + return true; + + // All package name portions and the class name must be valid names. + return StringUtil.fastSplit(name, true, '.').stream() + .allMatch(ExpressionCompiler::isSafeName); } /** diff --git a/recaf-core/src/main/java/software/coley/recaf/util/Types.java b/recaf-core/src/main/java/software/coley/recaf/util/Types.java index 748714129..96ceaa647 100644 --- a/recaf-core/src/main/java/software/coley/recaf/util/Types.java +++ b/recaf-core/src/main/java/software/coley/recaf/util/Types.java @@ -97,16 +97,16 @@ public static String classToPrimitive(@Nonnull String name) { } /** - * @param desc + * @param name * Some class name. * * @return {@code true} if it matches the class name of a primitive type. */ - public static boolean isPrimitiveClassName(@Nullable String desc) { - if (desc == null) + public static boolean isPrimitiveClassName(@Nullable String name) { + if (name == null) return false; for (Type prim : PRIMITIVES) - if (prim.getClassName().equals(desc)) + if (prim.getClassName().equals(name)) return true; return false; } diff --git a/recaf-core/src/test/java/software/coley/recaf/services/assembler/ExpressionCompilerTest.java b/recaf-core/src/test/java/software/coley/recaf/services/assembler/ExpressionCompilerTest.java index c772a887a..0011fe5a5 100644 --- a/recaf-core/src/test/java/software/coley/recaf/services/assembler/ExpressionCompilerTest.java +++ b/recaf-core/src/test/java/software/coley/recaf/services/assembler/ExpressionCompilerTest.java @@ -3,8 +3,15 @@ import jakarta.annotation.Nonnull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; import software.coley.recaf.info.JvmClassInfo; +import software.coley.recaf.info.builder.JvmClassInfoBuilder; import software.coley.recaf.services.compile.CompilerDiagnostic; import software.coley.recaf.test.TestBase; import software.coley.recaf.test.TestClassUtils; @@ -17,6 +24,7 @@ import java.util.List; import static org.junit.jupiter.api.Assertions.*; +import static org.objectweb.asm.Opcodes.*; /** * Tests for {@link ExpressionCompiler} @@ -134,6 +142,70 @@ void classAndMethodContextForStaticInitializer() { assertSuccess(result); } + @Nested + class ObfuscatedContexts { + @ParameterizedTest + @ValueSource(strings = {"void", "null", "int", "private", "throws", "", "\0", " ", "-10", "100", ""}) + void ignoreIllegalFieldName(String illegalFieldName) { + ClassWriter cw = new ClassWriter(0); + cw.visit(V1_8, ACC_PUBLIC, "ExampleClass", null, "java/lang/Object", null); + cw.visitField(ACC_PRIVATE, illegalFieldName, "I", null, null); + cw.visitMethod(ACC_PRIVATE, "methodName", "()V", null, null); + JvmClassInfo classInfo = new JvmClassInfoBuilder(cw.toByteArray()).build(); + + // The expression compiler should skip the field since it has an illegal name. + assembler.setClassContext(classInfo); + assembler.setMethodContext(classInfo.getFirstDeclaredMethodByName("methodName")); + ExpressionResult result = compile(""); + assertSuccess(result); + } + + @ParameterizedTest + @ValueSource(strings = {"void", "null", "int", "private", "throws", "", "\0", " ", "-10", "100", ""}) + void ignoreIllegalMethodName(String illegalMethodName) { + ClassWriter cw = new ClassWriter(0); + cw.visit(V1_8, ACC_PUBLIC, "ExampleClass", null, "java/lang/Object", null); + cw.visitMethod(ACC_PRIVATE, illegalMethodName, "()I", null, null); + cw.visitMethod(ACC_PRIVATE, "methodName", "()V", null, null); + JvmClassInfo classInfo = new JvmClassInfoBuilder(cw.toByteArray()).build(); + + // The expression compiler should skip the method since it has an illegal name. + assembler.setClassContext(classInfo); + assembler.setMethodContext(classInfo.getFirstDeclaredMethodByName("methodName")); + ExpressionResult result = compile(""); + assertSuccess(result); + } + + @ParameterizedTest + @ValueSource(strings = {"void", "null", "int", "private", "throws", "", "\0", " ", "-10", "100", ""}) + void ignoreIllegalMethodContextName(String illegalMethodName) { + ClassWriter cw = new ClassWriter(0); + cw.visit(V1_8, ACC_PUBLIC, "ExampleClass", null, "java/lang/Object", null); + Label start = new Label(); + Label end = new Label(); + + MethodVisitor mv = cw.visitMethod(ACC_PRIVATE, illegalMethodName, "(IIII)V", null, null); + mv.visitCode(); + mv.visitLabel(start); + mv.visitInsn(ICONST_0); + mv.visitInsn(IRETURN); + mv.visitLabel(end); + mv.visitEnd(); + mv.visitLocalVariable("one", "I", null, start, end, 1); + mv.visitLocalVariable("two", "I", null, start, end, 2); + mv.visitLocalVariable("three", "I", null, start, end, 3); + mv.visitLocalVariable(illegalMethodName, "I", null, start, end, 4); // Add an illegal named parameter + JvmClassInfo classInfo = new JvmClassInfoBuilder(cw.toByteArray()).build(); + + // The expression compiler should rename the obfuscated method specified as the context. + // Variables passed in (that are not illegally named) and such should still be accessible. + assembler.setClassContext(classInfo); + assembler.setMethodContext(classInfo.getFirstDeclaredMethodByName(illegalMethodName)); + ExpressionResult result = compile("int result = one + two + three;"); + assertSuccess(result); + } + } + private static void assertSuccess(@Nonnull ExpressionResult result) { assertNull(result.getException(), "Exception thrown when compiling: " + result.getException()); assertTrue(result.getDiagnostics().isEmpty(), "There were " + result.getDiagnostics().size() + " compiler messages"); From 7e1bc1fa5ebf7cd0268628af2780e9f77127a0fb Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 19 Apr 2024 07:44:40 -0400 Subject: [PATCH 4/4] Update vineflower to 1.10.1 --- gradle/libs.versions.toml | 2 +- .../services/decompile/vineflower/VineflowerDecompiler.java | 2 +- recaf-ui/build.gradle | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index eb25d8501..91067bb5f 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -33,7 +33,7 @@ reactfx = { strictly = "2.0-M5" } # won't get updates, dead regex = "0.1.15" richtextfx = "0.11.2" treemapfx = "1.1.0" -vineflower = "1.9.3" +vineflower = "1.10.1" wordwrap = "0.1.12" benmanes-versions = "0.42.0" gradle-coverage-report-aggregator = "1.3.0" diff --git a/recaf-core/src/main/java/software/coley/recaf/services/decompile/vineflower/VineflowerDecompiler.java b/recaf-core/src/main/java/software/coley/recaf/services/decompile/vineflower/VineflowerDecompiler.java index c7d393b1b..ab7799ab8 100644 --- a/recaf-core/src/main/java/software/coley/recaf/services/decompile/vineflower/VineflowerDecompiler.java +++ b/recaf-core/src/main/java/software/coley/recaf/services/decompile/vineflower/VineflowerDecompiler.java @@ -32,7 +32,7 @@ public class VineflowerDecompiler extends AbstractJvmDecompiler { @Inject public VineflowerDecompiler(@Nonnull VineflowerConfig config) { // Change this version to be dynamic when / if the Vineflower authors make a function that returns the version... - super(NAME, "1.9.3", config); + super(NAME, "1.10.1", config); this.config = config; logger = new VineflowerLogger(config); } diff --git a/recaf-ui/build.gradle b/recaf-ui/build.gradle index 9ebba22f9..5b212fdf0 100644 --- a/recaf-ui/build.gradle +++ b/recaf-ui/build.gradle @@ -33,6 +33,7 @@ shadowJar { exclude "META-INF/maven/**" exclude "META-INF/rewrite/**" exclude "META-INF/proguard/*" + exclude "META-INF/plugins/*" exclude "META-INF/native-image/*" exclude "META-INF/*.properties"