From 2f1d0e3eb007daf2cf5f3dcf9132fdabd173b47c Mon Sep 17 00:00:00 2001 From: Thiakil Date: Sun, 19 Jul 2020 13:06:44 +0800 Subject: add MappingValidator based test --- enigma/build.gradle | 2 +- .../java/cuchaz/enigma/TestAllowableClashes.java | 52 ++++++++++++++++++++++ .../cuchaz/enigma/ValidationContextMatcher.java | 35 +++++++++++++++ .../cuchaz/enigma/inputs/visibility/ClassA.java | 28 ++++++++++++ .../cuchaz/enigma/inputs/visibility/ClassB.java | 31 +++++++++++++ 5 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java create mode 100644 enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java create mode 100644 enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java create mode 100644 enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java diff --git a/enigma/build.gradle b/enigma/build.gradle index 7cebcd16..8d081457 100644 --- a/enigma/build.gradle +++ b/enigma/build.gradle @@ -53,7 +53,7 @@ file('src/test/java/cuchaz/enigma/inputs').listFiles().each { theFile -> args '@src/test/resources/proguard-test.conf', '-injars', file('build/test-inputs/' + "${theFile.name}.jar"), '-libraryjars', libraryJarsArg, - '-outjars', file('build/test-obf/' + "${theFile.name}.jar") + '-outjars', file('build/test-obf/' + "${theFile.name}.jar"), '-printmapping', file("build/test-obf/${theFile.name}-mapping.txt") } test.dependsOn "${theFile.name}TestObf" diff --git a/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java b/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java new file mode 100644 index 00000000..bfc21de3 --- /dev/null +++ b/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java @@ -0,0 +1,52 @@ +package cuchaz.enigma; + +import cuchaz.enigma.classprovider.ClasspathClassProvider; +import cuchaz.enigma.translation.mapping.EntryMapping; +import cuchaz.enigma.translation.mapping.EntryRemapper; +import cuchaz.enigma.translation.mapping.serde.MappingFormat; +import cuchaz.enigma.translation.mapping.serde.MappingParseException; +import cuchaz.enigma.translation.mapping.tree.EntryTree; +import cuchaz.enigma.translation.mapping.tree.EntryTreeNode; +import cuchaz.enigma.translation.representation.entry.MethodEntry; +import cuchaz.enigma.utils.validation.ValidationContext; +import org.hamcrest.MatcherAssert; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; +import java.nio.file.Paths; + +/** + * Test that we can accept some name clashes that are allowed by javac + */ +public class TestAllowableClashes { + + private static final String inputBaseName = "build/test-obf/visibility"; + + @Test + public void test() throws IOException, MappingParseException { + //Load produced mappings + Enigma enigma = Enigma.create(); + EnigmaProject project = enigma.openJar(Paths.get(inputBaseName + ".jar"), new ClasspathClassProvider(), ProgressListener.none()); + EntryTree obfToDeobf = MappingFormat.PROGUARD.read(Paths.get(inputBaseName + "-mapping.txt"), ProgressListener.none(), null); + + //Load them into enigma, none should conflict + EntryRemapper mapper = project.getMapper(); + for (int round=0; round<2; round++) { + for (EntryTreeNode node : obfToDeobf) { + Assert.assertNotEquals(null, node.getValue()); + if (node.getEntry() instanceof MethodEntry && (node.getEntry() + .getName() + .equals("") || node.getEntry().getName().equals(""))) { + //skip proguard's constructor entries + continue; + } + System.out.println(node.getEntry().toString() + " -> " + node.getValue().getTargetName()); + ValidationContext vc = new ValidationContext(); + mapper.mapFromObf(vc, node.getEntry(), node.getValue()); + MatcherAssert.assertThat(vc, ValidationContextMatcher.INSTANCE); + } + } + } + +} diff --git a/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java b/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java new file mode 100644 index 00000000..5404077f --- /dev/null +++ b/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java @@ -0,0 +1,35 @@ +package cuchaz.enigma; + +import cuchaz.enigma.utils.validation.ParameterizedMessage; +import cuchaz.enigma.utils.validation.ValidationContext; +import org.hamcrest.CustomMatcher; +import org.hamcrest.Description; + +class ValidationContextMatcher extends CustomMatcher { + public static final ValidationContextMatcher INSTANCE = new ValidationContextMatcher(); + + private ValidationContextMatcher() { + super("ValidationContext can proceed"); + } + + @Override + public boolean matches(Object item) { + return item instanceof ValidationContext && ((ValidationContext) item).canProceed(); + } + + @Override + public void describeMismatch(Object item, Description description) { + if (!(item instanceof ValidationContext)) { + description.appendText("expected ValidationContext, was").appendValue(item); + return; + } + ValidationContext vc = (ValidationContext) item; + for (ParameterizedMessage message : vc.getMessages()) { + description.appendText(message.getText()); + String longMessage = message.getLongText(); + if (longMessage != null && !longMessage.trim().isEmpty()){ + description.appendText(longMessage); + } + } + } +} diff --git a/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java new file mode 100644 index 00000000..458f7c0d --- /dev/null +++ b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java @@ -0,0 +1,28 @@ +package cuchaz.enigma.inputs.visibility; + +class ClassA { + + protected Object protectedParentPrivateChild; + public Object publicParentPrivateChild; + + public static Object LOGGER = null; + + protected static Object LOGGER2 = null; + + public Object publicPublic; + + public static void equalAccessStatic() {} + + protected static void protectedPublicStatic(){} + + private static void privateStaticParentPublicStaticChild(){} + + private void privateParentPublicStaticChild() {} + + static void packagePrivateParentProtectedChild(){} + + private static void packagePrivateChild(){} + + public ClassA returningSubclass(){return null;} + +} \ No newline at end of file diff --git a/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java new file mode 100644 index 00000000..2a1b68a7 --- /dev/null +++ b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java @@ -0,0 +1,31 @@ +package cuchaz.enigma.inputs.visibility; + +public class ClassB extends ClassA { + private Object protectedParentPrivateChild; + + private Object publicParentPrivateChild; + + public Object publicPublic; + + public static Object LOGGER; + + public static Object LOGGER2 = null; + + public static void equalAccessStatic() { + } + + public static void protectedPublicStatic() { + } + + public static void privateStaticParentPublicStaticChild() { + } + + public static void privateParentPublicStaticChild() { + } + + protected static void packagePrivateParentProtectedChild(){} + + static void packagePrivateChild(){} + + public ClassB returningSubclass(){return null;} +} -- cgit v1.2.3 From 08ed439aa1deaeefe4145d5c00a0f79e15f62751 Mon Sep 17 00:00:00 2001 From: Thiakil Date: Sun, 19 Jul 2020 16:42:17 +0800 Subject: allow name clashes to pass where javac would accept them --- .../translation/mapping/MappingValidator.java | 116 ++++++++++++++++----- .../translation/representation/AccessFlags.java | 21 ++++ 2 files changed, 111 insertions(+), 26 deletions(-) diff --git a/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java b/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java index f9f3b88e..134c05ce 100644 --- a/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java +++ b/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java @@ -1,15 +1,19 @@ package cuchaz.enigma.translation.mapping; import java.util.Collection; -import java.util.HashSet; +import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; import cuchaz.enigma.analysis.index.InheritanceIndex; import cuchaz.enigma.analysis.index.JarIndex; import cuchaz.enigma.translation.Translator; import cuchaz.enigma.translation.mapping.tree.EntryTree; +import cuchaz.enigma.translation.representation.AccessFlags; import cuchaz.enigma.translation.representation.entry.ClassEntry; +import cuchaz.enigma.translation.representation.entry.DefEntry; import cuchaz.enigma.translation.representation.entry.Entry; +import cuchaz.enigma.translation.representation.entry.FieldEntry; import cuchaz.enigma.utils.validation.Message; import cuchaz.enigma.utils.validation.ValidationContext; @@ -35,18 +39,16 @@ public class MappingValidator { private void validateUnique(ValidationContext vc, Entry entry, String name) { ClassEntry containingClass = entry.getContainingClass(); - Collection relatedClasses = getRelatedClasses(containingClass); - - for (ClassEntry relatedClass : relatedClasses) { - Entry relatedEntry = entry.replaceAncestor(containingClass, relatedClass); - Entry translatedEntry = deobfuscator.translate(relatedEntry); - - Collection> translatedSiblings = obfToDeobf.getSiblings(relatedEntry).stream() - .map(deobfuscator::translate) - .collect(Collectors.toList()); + InheritanceIndex inheritanceIndex = index.getInheritanceIndex(); - if (!isUnique(translatedEntry, translatedSiblings, name)) { - Entry parent = translatedEntry.getParent(); + //Step 1, check it's unique within its own siblings + Collection> directTranslatedSiblings = obfToDeobf.getSiblings(entry).stream() + .map(deobfuscator::translate) + .collect(Collectors.toList()); + for (Entry sibling : directTranslatedSiblings) { + if (entry.canConflictWith(sibling) && sibling.getName().equals(name) && isSynthetic(entry) == isSynthetic(sibling)) { + // allow clash if one is synthetic and the other is not + Entry parent = entry.getParent(); if (parent != null) { vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent); } else { @@ -54,26 +56,88 @@ public class MappingValidator { } } } - } - private Collection getRelatedClasses(ClassEntry classEntry) { - InheritanceIndex inheritanceIndex = index.getInheritanceIndex(); + //Step 2, check ancestors, ignoring members invisible to children + Set ancestors = inheritanceIndex.getAncestors(containingClass); + for (ClassEntry ancestor : ancestors) { + Entry reparentedEntry = entry.replaceAncestor(containingClass, ancestor); + Entry translatedEntry = Objects.requireNonNull(deobfuscator.translate(reparentedEntry), "Translation failed"); + Collection> translatedSiblings = obfToDeobf.getSiblings(reparentedEntry).stream() + .filter(it->!entry.equals(it))//e.g. for root classes, ensure we dont match the name against itself + .filter(this::isVisibleToChildren) + .collect(Collectors.toList()); + for (Entry parentSibling : translatedSiblings) { + Entry parentSiblingTranslated = Objects.requireNonNull(deobfuscator.translate(parentSibling), "Translation failed"); + if (translatedEntry.canConflictWith(parentSiblingTranslated) && parentSiblingTranslated.getName().equals(name) && !isAcceptableOverride(parentSibling, entry)) { + Entry parent = translatedEntry.getParent(); + if (parent != null) { + vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent); + } else { + vc.raise(Message.NONUNIQUE_NAME, name); + } + } + } + } - Collection relatedClasses = new HashSet<>(); - relatedClasses.add(classEntry); - relatedClasses.addAll(inheritanceIndex.getChildren(classEntry)); - relatedClasses.addAll(inheritanceIndex.getAncestors(classEntry)); + //Step 3, if this entry is visible to children, see if it clashes with any of their names + if (isVisibleToChildren(entry)) { + Collection children = inheritanceIndex.getDescendants(containingClass); + for (ClassEntry child : children) { + Entry reparentedEntry = entry.replaceAncestor(containingClass, child); + Entry translatedEntry = Objects.requireNonNull(deobfuscator.translate(reparentedEntry), "Translation failed"); + Collection> siblings = obfToDeobf.getSiblings(reparentedEntry).stream() + .filter(it->!entry.equals(it))//e.g. for root classes, ensure we dont match the name against itself + .collect(Collectors.toList()); + for (Entry childSibling : siblings) { + Entry childSiblingTranslated = Objects.requireNonNull(deobfuscator.translate(childSibling), "Translation failed"); + if (translatedEntry.canConflictWith(childSiblingTranslated) && childSiblingTranslated.getName().equals(name) && !isAcceptableOverride(entry, childSibling)) { + Entry parent = translatedEntry.getParent(); + if (parent != null) { + vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent); + } else { + vc.raise(Message.NONUNIQUE_NAME, name); + } + } + } + } + } + } - return relatedClasses; + private boolean isVisibleToChildren(Entry entry) { + if (entry instanceof DefEntry) { + return !((DefEntry) entry).getAccess().isPrivate(); + } + AccessFlags accessFlags = index.getEntryIndex().getEntryAccess(entry); + if (accessFlags != null) { + return !accessFlags.isPrivate(); + } + return true;//unknown, assume yes } - private boolean isUnique(Entry entry, Collection> siblings, String name) { - for (Entry sibling : siblings) { - if (entry.canConflictWith(sibling) && sibling.getName().equals(name)) { - return false; - } + private boolean isAcceptableOverride(Entry ancestor, Entry descendent) { + if (ancestor instanceof FieldEntry && descendent instanceof FieldEntry){ + return true;//fields don't apply here + } + + AccessFlags ancestorFlags = findAccessFlags(ancestor); + AccessFlags descendentFlags = findAccessFlags(descendent); + + if (ancestorFlags == null || descendentFlags == null) { + return false;//we can't make any assumptions } - return true; + + //bad == accessLevel < superAccessLevel + return !(descendentFlags.getAccessLevel() < ancestorFlags.getAccessLevel()); + } + + private boolean isSynthetic(Entry entry) { + AccessFlags accessFlags = findAccessFlags(entry); + return accessFlags != null && accessFlags.isSynthetic(); + } + + private AccessFlags findAccessFlags(Entry entry) { + return (entry instanceof DefEntry) ? ((DefEntry) entry).getAccess() : index.getEntryIndex() + .getEntryAccess(entry); } } diff --git a/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java b/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java index b280eef2..aa48a5b2 100644 --- a/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java +++ b/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java @@ -8,6 +8,10 @@ import java.lang.reflect.Modifier; public class AccessFlags { public static final AccessFlags PRIVATE = new AccessFlags(Opcodes.ACC_PRIVATE); public static final AccessFlags PUBLIC = new AccessFlags(Opcodes.ACC_PUBLIC); + public static final int ACCESS_LEVEL_PUBLIC = 4; + public static final int ACCESS_LEVEL_PROTECTED = 3; + public static final int ACCESS_LEVEL_PACKAGE_LOCAL = 2; + public static final int ACCESS_LEVEL_PRIVATE = 1; private int flags; @@ -89,6 +93,23 @@ public class AccessFlags { return this.flags; } + /** + * Adapted from https://github.com/JetBrains/intellij-community/blob/6472c347db91d11bbf02895a767198f9d884b119/java/java-psi-api/src/com/intellij/psi/util/PsiUtil.java#L389 + * @return visibility access level on a 'weakness scale' + */ + public int getAccessLevel() { + if (isPrivate()) { + return ACCESS_LEVEL_PRIVATE; + } + if (isProtected()) { + return ACCESS_LEVEL_PROTECTED; + } + if (isPublic()) { + return ACCESS_LEVEL_PUBLIC; + } + return ACCESS_LEVEL_PACKAGE_LOCAL; + } + @Override public boolean equals(Object obj) { return obj instanceof AccessFlags && ((AccessFlags) obj).flags == flags; -- cgit v1.2.3