Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 53e4da8

Browse files
Marcono1234Marcono1234
committedDec 31, 2024
Improve switched-arguments.ql
1 parent a19ea77 commit 53e4da8

File tree

2 files changed

+240
-25
lines changed

2 files changed

+240
-25
lines changed
 

‎codeql-custom-queries-java/queries/likely-bugs/switched-arguments-precise.ql

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
/**
2+
* Finds cases of switched arguments based on argument and parameter names.
3+
*
4+
* For example:
5+
* ```java
6+
* void addUser(String firstName, String lastName) {
7+
* ...
8+
* }
9+
*
10+
* ...
11+
*
12+
* String firstName = ...;
13+
* String lastName = ...;
14+
* // Provides arguments in wrong order
15+
* addUser(lastName, firstName);
16+
* ```
17+
*
18+
* @id TODO
19+
* @kind problem
20+
* @precision medium
21+
*/
22+
23+
import java
24+
import semmle.code.java.dataflow.DataFlow
25+
26+
predicate isAssignable(Parameter param, Variable argVar) {
27+
exists(Type paramType, Type argType | paramType = param.getType() and argType = argVar.getType() |
28+
paramType = argType or
29+
paramType = argType.(RefType).getAnAncestor() or
30+
paramType.(BoxedType).getPrimitiveType() = argType or
31+
paramType = argType.(BoxedType).getPrimitiveType() or
32+
// Note: This might be too lenient
33+
paramType.getErasure() = argType.getErasure()
34+
)
35+
}
36+
37+
// CodeQL seems to generate param names in the form p0, p1, ... for external classes
38+
predicate areParamNamesKnown(Callable c) {
39+
c.fromSource()
40+
or
41+
// Or there is a param name which is not `p<N>`
42+
exists(string paramName | paramName = c.getAParameter().getName() |
43+
not paramName.regexpMatch("p\\d+")
44+
)
45+
}
46+
47+
bindingset[s, prefix]
48+
predicate startsWith(string s, string prefix) { s.indexOf(prefix) = 0 }
49+
50+
bindingset[s, suffix]
51+
predicate endsWith(string s, string suffix) { s.indexOf(suffix) = s.length() - suffix.length() }
52+
53+
predicate matchesParamName(Parameter param, Variable argVar) {
54+
exists(string paramName, string varName |
55+
// Compare case insensitively
56+
paramName = param.getName().toLowerCase() and
57+
varName = argVar.getName().toLowerCase()
58+
|
59+
paramName = varName or
60+
startsWith(varName, paramName) or
61+
endsWith(varName, paramName)
62+
)
63+
}
64+
65+
class ComparableCompareToMethod extends Method {
66+
ComparableCompareToMethod() {
67+
getDeclaringType().hasQualifiedName("java.lang", "Comparable") and
68+
hasName("compareTo")
69+
}
70+
}
71+
72+
class ComparatorCompareMethod extends Method {
73+
ComparatorCompareMethod() {
74+
getDeclaringType().hasQualifiedName("java.util", "Comparator") and hasName("compare")
75+
}
76+
}
77+
78+
predicate isEnclosedByComparison(Call call, Variable var1, Variable var2) {
79+
exists(Expr condition |
80+
exists(IfStmt ifStmt |
81+
condition = ifStmt.getCondition() and
82+
call.getEnclosingStmt().getEnclosingStmt*() = [ifStmt.getThen(), ifStmt.getElse()]
83+
)
84+
or
85+
exists(ConditionalExpr e |
86+
condition = e.getCondition() and call.(MethodAccess).getParent*() = e.getABranchExpr()
87+
)
88+
|
89+
// Comparison expr
90+
exists(ComparisonExpr c | c.getParent*() = condition |
91+
c.getAnOperand() = var1.getAnAccess() and c.getAnOperand() = var2.getAnAccess()
92+
)
93+
or
94+
// Or `Comparable#compareTo` or `Comparator#compare` call
95+
exists(RValue varReadA, RValue varReadB |
96+
varReadA = var1.getAnAccess() and varReadB = var2.getAnAccess()
97+
or
98+
varReadA = var2.getAnAccess() and varReadB = var1.getAnAccess()
99+
|
100+
exists(MethodAccess compareToCall |
101+
compareToCall.getParent*() = condition and
102+
compareToCall.getMethod().getSourceDeclaration().getASourceOverriddenMethod*() instanceof
103+
ComparableCompareToMethod
104+
|
105+
compareToCall.getQualifier() = varReadA and
106+
compareToCall.getArgument(0) = varReadB
107+
)
108+
or
109+
exists(MethodAccess compareCall |
110+
compareCall.getParent*() = condition and
111+
compareCall.getMethod().getSourceDeclaration().getASourceOverriddenMethod*() instanceof
112+
ComparatorCompareMethod
113+
|
114+
compareCall.getArgument(0) = varReadA and
115+
compareCall.getArgument(1) = varReadB
116+
)
117+
)
118+
)
119+
}
120+
121+
from
122+
Call call, Callable callee, int param1Index, Parameter param1, int param2Index, Parameter param2,
123+
RValue arg1, Variable var1, RValue arg2, Variable var2
124+
where
125+
call.getCallee() = callee and
126+
areParamNamesKnown(callee) and
127+
// Enforce ordering, to avoid reporing switched arguments twice
128+
param1Index < param2Index and
129+
// Ignore if argument is part of varargs argument
130+
(
131+
call.getNumArgument() <= callee.getNumberOfParameters() or
132+
param2Index < callee.getNumberOfParameters() - 1
133+
) and
134+
param1 = callee.getParameter(param1Index) and
135+
param2 = callee.getParameter(param2Index) and
136+
arg1 = call.getArgument(param1Index) and
137+
arg1.getVariable() = var1 and
138+
arg2 = call.getArgument(param2Index) and
139+
arg2.getVariable() = var2 and
140+
var1 != var2 and
141+
// And param names indicate that args are switched
142+
matchesParamName(param1, var2) and
143+
not matchesParamName(param1, var1) and
144+
matchesParamName(param2, var1) and
145+
not matchesParamName(param2, var2) and
146+
// And args can really be switched
147+
isAssignable(param1, var2) and
148+
isAssignable(param2, var1) and
149+
// And does not call the same enclosing method or own constructor, in that case the arguments might be intentionally switched
150+
not (
151+
callee = call.getEnclosingCallable()
152+
or
153+
arg1.(FieldRead).isOwnFieldAccess() and
154+
arg2.(FieldRead).isOwnFieldAccess() and
155+
callee.(Constructor).getDeclaringType() = call.getEnclosingCallable().getDeclaringType()
156+
) and
157+
// Ignore if args are being compared, and are intentionally switched based on the result
158+
not isEnclosedByComparison(call, var1, var2) and
159+
// Ignore if part of a test assertion which might have intentionally switched args
160+
not (
161+
call.getEnclosingCallable().getDeclaringType() instanceof TestClass and
162+
exists(MethodAccess assertionCall | assertionCall.getMethod().getName().matches("assert%") |
163+
// Directly used as arg (possibly nested inside other expressions, e.g. comparison expr)
164+
call.(MethodAccess).getParent*() = assertionCall.getAnArgument()
165+
or
166+
// Or with dataflow
167+
DataFlow::localExprFlow(call, assertionCall.getAnArgument())
168+
)
169+
)
170+
select arg1, "'" + param1.getName() + "' arg might be switched with $@ (index " + param2Index + ")",
171+
arg2, "'" + param2.getName() + "' arg"

‎codeql-custom-queries-java/queries/likely-bugs/switched-arguments.ql

Lines changed: 69 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,84 @@
11
/**
22
* Finds cases of switched arguments based on argument and parameter names.
3+
*
4+
* For example:
5+
* ```java
6+
* void addUser(String firstName, String lastName) {
7+
* ...
8+
* }
9+
*
10+
* ...
11+
*
12+
* String firstName = ...;
13+
* String lastName = ...;
14+
* // Provides arguments in wrong order
15+
* addUser(lastName, firstName);
16+
* ```
17+
*
18+
* @id TODO
19+
* @kind problem
20+
* @precision low
321
*/
422

23+
// Note: This query has rather low precision; a more precise variant is `switched-arguments-precise.ql`
24+
// (but that might have more false negatives)
525
import java
626
import semmle.code.java.dataflow.DataFlow
727

8-
// TODO: This does not cover primitives conversion and might be wrong for generics
28+
// TODO: Might be wrong for generics
929
predicate isAssignable(Type paramType, Type argType) {
10-
paramType = argType
11-
or exists (RefType ancestor | ancestor = argType.(RefType).getAnAncestor() | paramType.(RefType) = ancestor)
30+
paramType = argType or
31+
paramType = argType.(RefType).getAnAncestor() or
32+
paramType.(BoxedType).getPrimitiveType() = argType or
33+
paramType = argType.(BoxedType).getPrimitiveType()
1234
}
1335

14-
predicate flowVarToArg(RValue varReadExpr, Expr arg) {
15-
DataFlow::localFlow(DataFlow::exprNode(varReadExpr), DataFlow::exprNode(arg))
36+
// CodeQL seems to generate param names in the form p0, p1, ... for external classes
37+
predicate areParamNamesKnown(Callable c) {
38+
c.fromSource()
39+
or
40+
// Or there is a param name which is not `p<N>`
41+
exists(string paramName | paramName = c.getAParameter().getName() |
42+
not paramName.regexpMatch("p\\d+")
43+
)
1644
}
1745

18-
predicate isMatchingArg(MethodAccess call, Variable var) {
19-
exists(int paramIndex, Parameter p, RValue varReadExpr | p = call.getMethod().getParameter(paramIndex) |
20-
varReadExpr.getVariable() = var
21-
and p.getName() = var.getName()
22-
and isAssignable(p.getType(), var.getType())
23-
and flowVarToArg(varReadExpr, call.getArgument(paramIndex))
24-
)
46+
bindingset[s, suffix]
47+
predicate endsWithIgnoreCase(string s, string suffix) {
48+
s.toLowerCase().indexOf(suffix.toLowerCase()) = s.length() - suffix.length()
2549
}
2650

27-
from MethodAccess call, Method method, int paramIndex, Parameter otherParam, Variable var, RValue varReadExpr
51+
from
52+
Call call, Callable callee, int paramIndex, Parameter otherParam, int otherParamIndex,
53+
string otherParamName, Variable var, RValue callArg
2854
where
29-
call.getMethod() = method
30-
and flowVarToArg(varReadExpr, call.getArgument(paramIndex))
31-
// Find a parameter with the same name as the argument,
32-
// but at a different index
33-
and otherParam = method.getAParameter()
34-
and otherParam != method.getParameter(paramIndex)
35-
and var = varReadExpr.getVariable()
36-
and otherParam.getName() = var.getName()
37-
and isAssignable(otherParam.getType(), var.getType())
38-
// Var might be used as multiple args, verify that it is not matching
39-
and not isMatchingArg(call, var)
40-
select call, varReadExpr, otherParam
55+
call.getCallee() = callee and
56+
areParamNamesKnown(callee) and
57+
callArg = call.getArgument(paramIndex) and
58+
// Ignore if argument is part of varargs argument
59+
(
60+
call.getNumArgument() <= callee.getNumberOfParameters() or
61+
paramIndex < callee.getNumberOfParameters() - 1
62+
) and
63+
// Find a parameter with the same name as the argument, but at a different index
64+
otherParam = callee.getParameter(otherParamIndex) and
65+
otherParamIndex != paramIndex and
66+
var = callArg.getVariable() and
67+
otherParamName = otherParam.getName() and
68+
otherParamName = var.getName() and
69+
isAssignable(otherParam.getType(), var.getType()) and
70+
// Var might be used as multiple args, verify that it is not also used for the expected parameter
71+
not var.getAnAccess() = call.getArgument(otherParamIndex) and
72+
// And the other param does not have a variable with matching name as argument
73+
not exists(string otherArgVarName |
74+
otherArgVarName = call.getArgument(otherParamIndex).(RValue).getVariable().getName()
75+
|
76+
otherArgVarName = otherParamName or
77+
otherArgVarName.indexOf(otherParamName) = 0 or
78+
endsWithIgnoreCase(otherArgVarName, otherParamName)
79+
) and
80+
// And does not call the same enclosing method, in that case the arguments might be intentionally switched
81+
not callee = call.getEnclosingCallable()
82+
select callArg,
83+
"Argument might be used at wrong position; should probably be for $@ at index " + otherParamIndex,
84+
otherParam, "this parameter"

0 commit comments

Comments
 (0)
Please sign in to comment.