Skip to content

Commit

Permalink
do not fully validate parameter indices in hasEntry (#238)
Browse files Browse the repository at this point in the history
* do not fully validate parameter indices in hasEntry

* checkstyle
  • Loading branch information
ix0rai authored Nov 7, 2024
1 parent fd66d9b commit 8740a14
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 42 deletions.
42 changes: 39 additions & 3 deletions enigma/src/main/java/org/quiltmc/enigma/api/EnigmaProject.java
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public boolean isNavigable(Entry<?> obfEntry) {
return false;
}

return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry, this);
return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry);
}

public boolean isRenamable(Entry<?> obfEntry) {
Expand Down Expand Up @@ -211,7 +211,7 @@ public boolean isRenamable(Entry<?> obfEntry) {
return false;
}

return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry, this);
return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry);
}

private static boolean isEnumValueOfMethod(ClassDefEntry parent, MethodEntry method) {
Expand All @@ -237,7 +237,7 @@ public boolean isObfuscated(Entry<?> entry) {
}

public boolean isSynthetic(Entry<?> entry) {
return this.jarIndex.getIndex(EntryIndex.class).hasEntry(entry, this) && this.jarIndex.getIndex(EntryIndex.class).getEntryAccess(entry).isSynthetic();
return this.jarIndex.getIndex(EntryIndex.class).hasEntry(entry) && this.jarIndex.getIndex(EntryIndex.class).getEntryAccess(entry).isSynthetic();
}

public boolean isAnonymousOrLocal(ClassEntry classEntry) {
Expand All @@ -246,6 +246,42 @@ public boolean isAnonymousOrLocal(ClassEntry classEntry) {
return enclosingMethodIndex.hasEnclosingMethod(classEntry);
}

/**
* Verifies that the provided {@code parameter} has a valid index for its parent method.
* This method validates both the upper and lower bounds of the parent method's index range.
*
* <p>Note that this method could still return {@code true} for an invalid index in the case that the index is impossible due to double-size parameters --
* for example, if the index is 4 and there's a double at index 3, the index would be invalid.
* But honestly, we at <a href=https://quiltmc.org>QuiltMC</a> doubt that that's a situation you'll ever be running into.
* If it is, complain <a href=https://github.com/QuiltMC/enigma/issues>in our issue tracker</a> about us writing this whole comment instead of implementing that functionality.
*
* @param parameter the parameter to validate
* @return whether the index is valid
*/
public boolean validateParameterIndex(LocalVariableEntry parameter) {
MethodEntry parent = parameter.getParent();
EntryIndex index = this.jarIndex.getIndex(EntryIndex.class);

if (index.hasMethod(parent)) {
AtomicInteger maxLocals = new AtomicInteger(-1);
ClassEntry parentClass = parent != null ? parent.getParent() : null;

// find max_locals for method, representing the number of parameters it receives (JVMS§4.7.3)
// note: parent class cannot be null, warning suppressed
ClassNode classNode = this.getClassProvider().get(parentClass.getFullName());
if (classNode != null) {
classNode.methods.stream()
.filter(node -> node.name.equals(parent.getName()) && node.desc.equals(parent.getDesc().toString()))
.findFirst().ifPresent(node -> maxLocals.set(node.maxLocals));
}

// if maxLocals is -1 it's not found for the method and should be ignored
return index.validateParameterIndex(parameter) && (maxLocals.get() == -1 || parameter.getIndex() <= maxLocals.get() - 1);
}

return false;
}

public JarExport exportRemappedJar(ProgressListener progress) {
Collection<ClassEntry> classEntries = this.jarIndex.getIndex(EntryIndex.class).getClasses();
ClassProvider fixingClassProvider = new ObfuscationFixClassProvider(this.classProvider, this.jarIndex);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.quiltmc.enigma.api.analysis.index.jar;

import org.objectweb.asm.tree.ClassNode;
import org.quiltmc.enigma.api.EnigmaProject;
import org.quiltmc.enigma.api.translation.mapping.EntryMapping;
import org.quiltmc.enigma.api.translation.mapping.tree.EntryTree;
Expand All @@ -19,7 +18,6 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;

public class EntryIndex implements JarIndexer {
private final EntryTree<EntryMapping> tree = new HashEntryTree<>();
Expand Down Expand Up @@ -73,26 +71,13 @@ public boolean hasField(FieldEntry entry) {
/**
* Checks whether the entry has been indexed and therefore exists in the JAR file.
* <br>
* Parameters are not indexed, and this method does not fully verify validity of parameter indices.
* Therefore, it is possible that this method returns {@code true} for an invalid parameter.
* For parameters, which are not indexed, checks the parent method and partially verifies their indices.
* To fully validate a parameter's index, use {@link EnigmaProject#validateParameterIndex(LocalVariableEntry)}.
*
* @param entry the entry to check
* @return whether the entry exists
* @see #hasEntry(Entry, EnigmaProject)
*/
public boolean hasEntry(Entry<?> entry) {
return this.hasEntry(entry, null);
}

/**
* Checks whether the entry has been indexed and therefore exists in the JAR file.
* <br>
* For parameters, which are not indexed, verifies that they have a valid index and therefore could exist.
* @param entry the entry to check
* @param project the current project
* @return whether the entry exists
*/
@SuppressWarnings("ConstantConditions")
public boolean hasEntry(Entry<?> entry, @Nullable EnigmaProject project) {
if (entry instanceof ClassEntry classEntry) {
return this.hasClass(classEntry);
} else if (entry instanceof MethodEntry methodEntry) {
Expand All @@ -101,32 +86,30 @@ public boolean hasEntry(Entry<?> entry, @Nullable EnigmaProject project) {
return this.hasField(fieldEntry);
} else if (entry instanceof LocalVariableEntry localVariableEntry) {
MethodEntry parent = localVariableEntry.getParent();
if (this.hasMethod(parent)) {
AtomicInteger maxLocals = new AtomicInteger(-1);
ClassEntry parentClass = parent != null ? parent.getParent() : null;

if (project != null) {
// find max_locals for method, representing the number of parameters it receives (JVMS§4.7.3)
// note: parent class cannot be null, warning suppressed
ClassNode classNode = project.getClassProvider().get(parentClass.getFullName());
if (classNode != null) {
classNode.methods.stream()
.filter(node -> node.name.equals(parent.getName()) && node.desc.equals(parent.getDesc().toString()))
.findFirst().ifPresent(node -> maxLocals.set(node.maxLocals));
}
}

AccessFlags parentAccess = this.getMethodAccess(parent);
int startIndex = parentAccess != null && parentAccess.isStatic() ? 0 : 1;

// if maxLocals is -1 it's not found for the method and should be ignored
return localVariableEntry.getIndex() >= startIndex && (maxLocals.get() == -1 || localVariableEntry.getIndex() <= maxLocals.get() - 1);
if (this.hasEntry(parent)) {
return this.validateParameterIndex(localVariableEntry);
}
}

return false;
}

/**
* Validates that the parameter index is not below the minimum index for its parent method and therefore could be valid.
* <br>Note that this method does not guarantee the index is valid -- for full validation, call {@link EnigmaProject#validateParameterIndex(LocalVariableEntry)}.
*
* @param parameter the parameter to validate
* @return whether the index could be valid
* @see EnigmaProject#validateParameterIndex(LocalVariableEntry)
*/
public boolean validateParameterIndex(LocalVariableEntry parameter) {
MethodEntry parent = parameter.getParent();
AccessFlags parentAccess = this.getMethodAccess(parent);

int startIndex = parentAccess != null && parentAccess.isStatic() ? 0 : 1;
return parameter.getIndex() >= startIndex;
}

@Nullable
public AccessFlags getMethodAccess(MethodEntry entry) {
var def = this.methodDefinitions.get(entry);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.quiltmc.enigma.api.translation.mapping.tree.EntryTree;
import org.quiltmc.enigma.api.translation.mapping.tree.EntryTreeNode;
import org.quiltmc.enigma.api.translation.representation.entry.Entry;
import org.quiltmc.enigma.api.translation.representation.entry.LocalVariableEntry;
import org.quiltmc.enigma.api.translation.representation.entry.MethodEntry;

import java.util.Collection;
Expand Down Expand Up @@ -62,7 +63,8 @@ private void tryDropBrokenEntry(Dropped dropped, Entry<?> entry) {
}

private boolean shouldDropBrokenEntry(Dropped dropped, Entry<?> entry) {
if (!this.index.getIndex(EntryIndex.class).hasEntry(entry, this.project)) {
if (!this.index.getIndex(EntryIndex.class).hasEntry(entry)
|| (entry instanceof LocalVariableEntry parameter && !this.project.validateParameterIndex(parameter))) {
return true;
}

Expand Down

0 comments on commit 8740a14

Please sign in to comment.