Fix ExecutableMatcher to enforce invocation constraints for constructors#10
Fix ExecutableMatcher to enforce invocation constraints for constructors#10Howard20181 merged 6 commits intodocsfrom
Conversation
…ods and constructors Co-authored-by: Howard20181 <40033067+Howard20181@users.noreply.github.com>
Co-authored-by: Howard20181 <40033067+Howard20181@users.noreply.github.com>
…ly once Co-authored-by: Howard20181 <40033067+Howard20181@users.noreply.github.com>
Co-authored-by: Howard20181 <40033067+Howard20181@users.noreply.github.com>
Co-authored-by: Howard20181 <40033067+Howard20181@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes ExecutableMatcher so that invocation constraints (setInvokedMethods() / setInvokedConstructors()) are enforced consistently for both methods and constructors, by ensuring DEX invocation relationships can be stored and queried for either executable type.
Changes:
- Switched invocation relationship maps to be keyed by
Executableinstead ofMethod. - Updated DEX analysis reflection resolution to treat
"<init>"as a constructor (getDeclaredConstructor()). - Removed method-only gating so invocation constraints are evaluated for constructors as well.
Comments suppressed due to low confidence (2)
helper/src/main/java/io/github/libxposed/helper/HookBuilderImpl.java:667
- The invocation-collection block after resolving
currentExecutableis indented as if it were still inside the inner try/catch, which makes the control flow hard to follow. Consider reformatting / adjusting indentation (and possibly extracting the resolution + collection into small helpers) so the outer vs inner try scopes are visually clear.
// Try to resolve current method or constructor via reflection
Executable currentExecutable;
try {
var currentClass = reflector.loadClass(currentClassName);
var currentParamTypes = getParameterTypesFromProto(currentProto);
if (currentParamTypes == null) {
return;
}
// Check if it's a constructor
if ("<init>".equals(currentMethodName)) {
currentExecutable = currentClass.getDeclaredConstructor(currentParamTypes);
} else {
currentExecutable = currentClass.getDeclaredMethod(currentMethodName, currentParamTypes);
}
} catch (ClassNotFoundException | NoSuchMethodException e) {
return;
}
// Collect all methods and constructors invoked by this method
Set<Method> invokedMethodsSet = new HashSet<>();
Set<Constructor<?>> invokedConstructorsSet = new HashSet<>();
helper/src/main/java/io/github/libxposed/helper/HookBuilderImpl.java:1748
- Potentially confusing name: method doMatch also refers to field parameterCount (as this.parameterCount).
final int parameterCount;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Cast once for constraint checking (only needed if constraints are present) | ||
| // At this point, reflect is guaranteed to be either Method or Constructor | ||
| // because line 1754 returns false if it's neither | ||
| Executable currentExecutable = null; | ||
| if (invokedMethods != null || invokedConstructors != null) { | ||
| currentExecutable = asExecutable(reflect); | ||
| } | ||
|
|
||
| // Check invoked methods constraint | ||
| if (invokedMethods != null && reflect instanceof Method) { | ||
| var currentMethod = (Method) reflect; | ||
| var invokedMethodsSet = methodInvocationsMap.get(currentMethod); | ||
| if (invokedMethods != null) { | ||
| var invokedMethodsSet = methodInvocationsMap.get(currentExecutable); | ||
| if (invokedMethodsSet == null || invokedMethodsSet.isEmpty()) { |
There was a problem hiding this comment.
currentExecutable is nullable and is later used as the key for ConcurrentHashMap.get(...), which will throw if it ever becomes null (CHM disallows null keys). Since this method already proves reflect is a Method or Constructor, consider assigning Executable currentExecutable = (Executable) reflect; (or make asExecutable() non-null / assert) to remove the nullable path and avoid future refactors accidentally introducing an NPE.
ExecutableMatcherapplies to both methods and constructors, butsetInvokedMethods()andsetInvokedConstructors()constraints were only enforced when matching methods. Constructor matches silently ignored these constraints.Root Cause
Three related issues:
Method: Maps usedMethodas keys instead ofExecutable, preventing constructor data storagegetDeclaredMethod(), failing to resolve<init>methods asConstructorobjectsinstanceof Method:doMatch()only checked constraints whenreflect instanceof MethodChanges
methodInvocationsMapandconstructorInvocationsMapto key byExecutable<init>and callgetDeclaredConstructor()appropriatelyinstanceof Methodguard from constraint checks indoMatch()getInvokedMethods()andgetInvokedConstructors()to handle both executable typesasExecutable()helper to eliminate type-casting duplicationExample
Before: constraints silently ignored for constructors
After: constraints properly enforced for both
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.