Bug: Logic error in AlienHostComponentFilter.accepts leads to incorrect callback filtering #838
Description
File: soot-infoflow-android/src/soot/jimple/infoflow/android/filters/AlienHostComponentFilter.java
Method: accepts(SootClass component, SootClass callbackHandler)
Hi FlowDroid team,
I believe I've identified a logical error in the AlienHostComponentFilter.accepts method that causes it to incorrectly reject valid callbacks.
Bug Description
The comment in the code states the intention:
Java
// If the callback class only has constructors that require references
// to other components, it is not meant to be used by the current
// component.
This implies that the filter should return false (reject the callback) if and only if all public/non-private constructors require references to other components.
However, the current implementation does the opposite. It iterates through the constructors, and if it finds any constructor that is not usable (i.e., takes another component as a parameter), it immediately returns false.
Java
// ... inside constructor loop ...
// Do we have a usable constructor?
if (!isConstructorUsable)
return false;
// ... end of constructor loop ...
return true;
This logic is flawed. If a class has two constructors, one that is usable and one that is not, the current code will find the unusable one and immediately return false, incorrectly discarding the callback.
The correct logic should be to iterate through all constructors and look for at least one usable constructor. If a single usable constructor is found, the method should immediately return true (accept the callback). If the loop completes without finding any usable constructors, then it should return false.
Impact
This bug causes FlowDroid to incorrectly discard many valid callbacks, potentially leading to missed data flows.
Proposed Fix
The logic should be inverted to match the comment's intent. We should return true as soon as we find a usable constructor, and false only if all constructors have been checked and none are usable.
Here is the diff:
Original (Buggy) Code:
Java
// ...
for (SootMethod cons : callbackHandler.getMethods()) {
if (cons.isConstructor() && !cons.isPrivate()) {
boolean isConstructorUsable = true;
// Check if we have at least one other component in the parameter
// list
for (int i = 0; i < cons.getParameterCount(); i++) {
Type paramType = cons.getParameterType(i);
if (paramType != component.getType() && paramType instanceof RefType) {
if (components.contains(((RefType) paramType).getSootClass())) {
isConstructorUsable = false;
break;
}
}
}
// Do we have a usable constructor?
if (!isConstructorUsable)
return false;
}
}
return true;
}
Suggested (Fixed) Code:
Java
// ...
for (SootMethod cons : callbackHandler.getMethods()) {
if (cons.isConstructor() && !cons.isPrivate()) {
boolean isConstructorUsable = true;
// Check if we have at least one other component in the parameter
// list
for (int i = 0; i < cons.getParameterCount(); i++) {
Type paramType = cons.getParameterType(i);
if (paramType != component.getType() && paramType instanceof RefType) {
if (components.contains(((RefType) paramType).getSootClass())) {
isConstructorUsable = false;
break;
}
}
}
// Do we have a usable constructor?
if (isConstructorUsable)
return true;
}
}
return false;
}
This change correctly implements the logic described in the comments.
Thank you for your work on this project!