Always extract ValueEQ/NEExpr for Kotlin ==/!=

I introduce AnyEqualsExpr for either reference or value equality and AnyEqualityTest for the same concept including not-equals operators, and use them wherever the written QL clearly doesn't care about the difference between reference and value comparison, typically because it is concerned with testing against null or against a primitive constant.
This commit is contained in:
Chris Smowton
2022-02-04 16:42:13 +00:00
committed by Ian Lynagh
parent a120fab9f7
commit f95effcf82
27 changed files with 118 additions and 80 deletions

View File

@@ -9,6 +9,6 @@
import java
from EqualityTest eq
from AnyEqualityTest eq
where eq.getAnOperand() instanceof BooleanLiteral
select eq

View File

@@ -225,12 +225,12 @@ class CompileTimeConstantExpr extends Expr {
)
or
(
b instanceof EQExpr and
b instanceof AnyEqualsExpr and
if left = right then result = true else result = false
)
or
(
b instanceof NEExpr and
b instanceof AnyNotEqualsExpr and
if left != right then result = true else result = false
)
)
@@ -242,12 +242,12 @@ class CompileTimeConstantExpr extends Expr {
right = b.getRightOperand().(CompileTimeConstantExpr).getBooleanValue()
|
(
b instanceof EQExpr and
b instanceof AnyEqualsExpr and
if left = right then result = true else result = false
)
or
(
b instanceof NEExpr and
b instanceof AnyNotEqualsExpr and
if left != right then result = true else result = false
)
or
@@ -269,15 +269,18 @@ class CompileTimeConstantExpr extends Expr {
/*
* JLS 15.28 specifies that compile-time `String` constants are interned. Therefore `==`
* equality can be interpreted as equality over the constant values, not the references.
*
* Kotlin's `==` and `===` operators will return the same result for `String`s, so they
* can be handled alike:
*/
(
b instanceof EQExpr and
b instanceof AnyEqualsExpr and
if left = right then result = true else result = false
)
or
(
b instanceof NEExpr and
b instanceof AnyNotEqualsExpr and
if left != right then result = true else result = false
)
)
@@ -359,7 +362,7 @@ class CompileTimeConstantExpr extends Expr {
or
b instanceof XorBitwiseExpr and result = v1.bitXor(v2)
// No `int` value for `AndLogicalExpr` or `OrLogicalExpr`.
// No `int` value for `LTExpr`, `GTExpr`, `LEExpr`, `GEExpr`, `EQExpr` or `NEExpr`.
// No `int` value for `LTExpr`, `GTExpr`, `LEExpr`, `GEExpr`, `AnyEqualsExpr` or `AnyNotEqualsExpr`.
)
or
// Ternary conditional, with compile-time constant condition.
@@ -952,7 +955,7 @@ class GEExpr extends BinaryExpr, @geexpr {
override string getAPrimaryQlClass() { result = "GEExpr" }
}
/** A binary expression using the `==` operator. */
/** A binary expression using Java's `==` or Kotlin's `===` operator. */
class EQExpr extends BinaryExpr, @eqexpr {
override string getOp() { result = " == " }
@@ -961,12 +964,12 @@ class EQExpr extends BinaryExpr, @eqexpr {
/** A binary expression using the Kotlin `==` operator, semantically equivalent to `Objects.equals`. */
class ValueEQExpr extends BinaryExpr, @valueeqexpr {
override string getOp() { result = " == " }
override string getOp() { result = " (value equals) " }
override string getAPrimaryQlClass() { result = "ValueEQExpr" }
}
/** A binary expression using the `!=` operator. */
/** A binary expression using Java's `!=` or Kotlin's `!==` operator. */
class NEExpr extends BinaryExpr, @neexpr {
override string getOp() { result = " != " }
@@ -975,11 +978,35 @@ class NEExpr extends BinaryExpr, @neexpr {
/** A binary expression using the Kotlin `!=` operator, semantically equivalent to `Objects.equals`. */
class ValueNEExpr extends BinaryExpr, @valueneexpr {
override string getOp() { result = " != " }
override string getOp() { result = " (value not-equals) " }
override string getAPrimaryQlClass() { result = "ValueNEExpr" }
}
/**
* A binary expression using either Java or Kotlin's `==` operator.
*
* This might test for reference equality or might function like `Objects.equals`. If you
* need to distinguish them, use `EQExpr` or `ValueEQExpr` instead.
*/
class AnyEqualsExpr extends BinaryExpr {
AnyEqualsExpr() {
this instanceof EQExpr or this instanceof ValueEQExpr
}
}
/**
* A binary expression using either Java or Kotlin's `!=` operator.
*
* This might test for reference equality or might function like `Objects.equals`. If you
* need to distinguish them, use `EQExpr` or `ValueEQExpr` instead.
*/
class AnyNotEqualsExpr extends BinaryExpr {
AnyNotEqualsExpr() {
this instanceof NEExpr or this instanceof ValueNEExpr
}
}
/**
* A bitwise expression.
*
@@ -1068,12 +1095,16 @@ class GreaterThanComparison extends ComparisonExpr {
/**
* An equality test is a binary expression using
* the `==` or `!=` operator.
* Java's `==` or `!=` operators, or Kotlin's `==`, `!=`, `===` or `!==` operators.
*
* This could be a reference- or a value-in/equality test.
*/
class EqualityTest extends BinaryExpr {
EqualityTest() {
class AnyEqualityTest extends BinaryExpr {
AnyEqualityTest() {
this instanceof EQExpr or
this instanceof NEExpr
this instanceof NEExpr or
this instanceof ValueEQExpr or
this instanceof ValueNEExpr
}
/** Gets a boolean indicating whether this is `==` (true) or `!=` (false). */
@@ -1081,6 +1112,23 @@ class EqualityTest extends BinaryExpr {
result = true and this instanceof EQExpr
or
result = false and this instanceof NEExpr
or
result = true and this instanceof ValueEQExpr
or
result = false and this instanceof ValueNEExpr
}
}
/**
* An equality test is a binary expression using
* Java's `==` or `!=` operator.
*
* If either operand is a reference type, this is a reference-in/equality test.
*/
class EqualityTest extends AnyEqualityTest {
EqualityTest() {
this instanceof EQExpr or
this instanceof NEExpr
}
}

View File

@@ -242,7 +242,7 @@ private predicate guardControls_v3(Guard guard, BasicBlock controlled, boolean b
}
private predicate equalityGuard(Guard g, Expr e1, Expr e2, boolean polarity) {
exists(EqualityTest eqtest |
exists(AnyEqualityTest eqtest |
eqtest = g and
polarity = eqtest.polarity() and
eqtest.hasOperands(e1, e2)

View File

@@ -133,12 +133,12 @@ class ConstantExpr extends Expr {
)
or
(
b instanceof EQExpr and
b instanceof AnyEqualsExpr and
if left = right then result = true else result = false
)
or
(
b instanceof NEExpr and
b instanceof AnyNotEqualsExpr and
if left != right then result = true else result = false
)
)

View File

@@ -35,7 +35,7 @@ predicate implies_v1(Guard g1, boolean b1, Guard g2, boolean b2) {
b1 = b2.booleanNot() and
b1 = [true, false]
or
exists(EqualityTest eqtest, boolean polarity, BooleanLiteral boollit |
exists(AnyEqualityTest eqtest, boolean polarity, BooleanLiteral boollit |
eqtest = g1 and
eqtest.hasOperands(g2, boollit) and
eqtest.polarity() = polarity and

View File

@@ -42,7 +42,7 @@ class IntComparableExpr extends Expr {
*/
pragma[nomagic]
Expr integerGuard(IntComparableExpr e, boolean branch, int k, boolean is_k) {
exists(EqualityTest eqtest, boolean polarity |
exists(AnyEqualityTest eqtest, boolean polarity |
eqtest = result and
eqtest.hasOperands(e, any(ConstantIntegerExpr c | c.getIntValue() = k)) and
polarity = eqtest.polarity() and
@@ -53,7 +53,7 @@ Expr integerGuard(IntComparableExpr e, boolean branch, int k, boolean is_k) {
)
)
or
exists(EqualityTest eqtest, int val, Expr c, boolean upper |
exists(AnyEqualityTest eqtest, int val, Expr c, boolean upper |
k = e.relevantInt() and
eqtest = result and
eqtest.hasOperands(e, c) and

View File

@@ -17,7 +17,7 @@ Expr alwaysNullExpr() {
/** Gets an equality test between an expression `e` and an enum constant `c`. */
Expr enumConstEquality(Expr e, boolean polarity, EnumConstant c) {
exists(EqualityTest eqtest |
exists(AnyEqualityTest eqtest |
eqtest = result and
eqtest.hasOperands(e, c.getAnAccess()) and
polarity = eqtest.polarity()
@@ -33,8 +33,10 @@ InstanceOfExpr instanceofExpr(SsaVariable v, RefType type) {
/**
* Gets an expression of the form `v1 == v2` or `v1 != v2`.
* The predicate is symmetric in `v1` and `v2`.
*
* Note this includes Kotlin's `==` and `!=` operators, which are value-equality tests.
*/
EqualityTest varEqualityTestExpr(SsaVariable v1, SsaVariable v2, boolean isEqualExpr) {
AnyEqualityTest varEqualityTestExpr(SsaVariable v1, SsaVariable v2, boolean isEqualExpr) {
result.hasOperands(v1.getAUse(), v2.getAUse()) and
isEqualExpr = result.polarity()
}
@@ -172,7 +174,7 @@ predicate nullCheckMethod(Method m, boolean branch, boolean isnull) {
* is true, and non-null if `isnull` is false.
*/
Expr basicNullGuard(Expr e, boolean branch, boolean isnull) {
exists(EqualityTest eqtest, boolean polarity |
exists(AnyEqualityTest eqtest, boolean polarity |
eqtest = result and
eqtest.hasOperands(e, any(NullLiteral n)) and
polarity = eqtest.polarity() and
@@ -191,7 +193,7 @@ Expr basicNullGuard(Expr e, boolean branch, boolean isnull) {
nullCheckMethod(call.getMethod(), branch, isnull)
)
or
exists(EqualityTest eqtest |
exists(AnyEqualityTest eqtest |
eqtest = result and
eqtest.hasOperands(e, clearlyNotNullExpr()) and
isnull = false and

View File

@@ -70,13 +70,13 @@ private predicate unboxed(Expr e) {
or
exists(AssignOp assign | assign.getSource() = e and assign.getType() instanceof PrimitiveType)
or
exists(EqualityTest eq |
exists(AnyEqualityTest eq |
eq.getAnOperand() = e and eq.getAnOperand().getType() instanceof PrimitiveType
)
or
exists(BinaryExpr bin |
bin.getAnOperand() = e and
not bin instanceof EqualityTest and
not bin instanceof AnyEqualityTest and
bin.getType() instanceof PrimitiveType
)
or
@@ -653,7 +653,7 @@ private Expr trackingVarGuard(
)
)
or
exists(EqualityTest eqtest, boolean branch0, boolean polarity, BooleanLiteral boollit |
exists(AnyEqualityTest eqtest, boolean branch0, boolean polarity, BooleanLiteral boollit |
eqtest = result and
eqtest.hasOperands(trackingVarGuard(trackssa, trackvar, kind, branch0, isA), boollit) and
eqtest.polarity() = polarity and

View File

@@ -387,7 +387,7 @@ private predicate taintPreservingArgumentToQualifier(Method method, int arg) {
/** A comparison or equality test with a constant. */
private predicate comparisonStep(Expr tracked, Expr sink) {
exists(Expr other |
exists(BinaryExpr e | e instanceof ComparisonExpr or e instanceof EqualityTest |
exists(BinaryExpr e | e instanceof ComparisonExpr or e instanceof AnyEqualityTest |
e = sink and
e.hasOperands(tracked, other)
)

View File

@@ -116,7 +116,7 @@ private predicate intentFlagsOrDataChecked(Guard g, Expr intent, boolean branch)
bitwiseLocalTaintStep*(DataFlow::exprNode(ma), DataFlow::exprNode(checkedValue))
|
bitwiseCheck(g, branch) and
checkedValue = g.(EqualityTest).getAnOperand().(AndBitwiseExpr)
checkedValue = g.(AnyEqualityTest).getAnOperand().(AndBitwiseExpr)
or
g.(MethodAccess).getMethod() instanceof EqualsMethod and
branch = true and
@@ -129,10 +129,10 @@ private predicate intentFlagsOrDataChecked(Guard g, Expr intent, boolean branch)
* and `false` otherwise.
*/
private predicate bitwiseCheck(Guard g, boolean branch) {
exists(CompileTimeConstantExpr operand | operand = g.(EqualityTest).getAnOperand() |
exists(CompileTimeConstantExpr operand | operand = g.(AnyEqualityTest).getAnOperand() |
if operand.getIntValue() = 0
then g.(EqualityTest).polarity() = branch
else g.(EqualityTest).polarity().booleanNot() = branch
then g.(AnyEqualityTest).polarity() = branch
else g.(AnyEqualityTest).polarity().booleanNot() = branch
)
}

View File

@@ -29,9 +29,9 @@ where
not isDefinitelyPositive(lhs.getLeftOperand()) and
lhs.getRightOperand().(IntegerLiteral).getIntValue() = 2 and
(
t instanceof EQExpr and rhs.getIntValue() = 1 and parity = "oddness"
t instanceof AnyEqualsExpr and rhs.getIntValue() = 1 and parity = "oddness"
or
t instanceof NEExpr and rhs.getIntValue() = 1 and parity = "evenness"
t instanceof AnyNotEqualsExpr and rhs.getIntValue() = 1 and parity = "evenness"
or
t instanceof GTExpr and rhs.getIntValue() = 0 and parity = "oddness"
)

View File

@@ -45,7 +45,7 @@ class ShiftExpr extends BinaryExpr {
*/
class RelationExpr extends BinaryExpr {
RelationExpr() {
this instanceof EqualityTest or
this instanceof AnyEqualityTest or
this instanceof ComparisonExpr
}
}
@@ -79,7 +79,7 @@ class AssocNestedExpr extends BinaryExpr {
parent.getKind() = this.getKind()
or
// Equality tests are associate over each other.
this instanceof EqualityTest and parent instanceof EqualityTest
this instanceof AnyEqualityTest and parent instanceof AnyEqualityTest
or
// (x*y)/z = x*(y/z)
this instanceof MulExpr and parent instanceof DivExpr and idx = 0

View File

@@ -14,7 +14,7 @@
import java
predicate comparison(BinaryExpr binop, Expr left, Expr right) {
(binop instanceof ComparisonExpr or binop instanceof EqualityTest) and
(binop instanceof ComparisonExpr or binop instanceof AnyEqualityTest) and
binop.getLeftOperand() = left and
binop.getRightOperand() = right
}
@@ -64,13 +64,13 @@ predicate equal(Expr left, Expr right) {
)
}
predicate specialCase(EqualityTest comparison, string msg) {
predicate specialCase(AnyEqualityTest comparison, string msg) {
exists(FloatingPointType fptp, string neg, string boxedName |
fptp = comparison.getAnOperand().getType() and
// Name of boxed type corresponding to `fptp`.
(if fptp.getName().toLowerCase() = "float" then boxedName = "Float" else boxedName = "Double") and
// Equality tests are tests for not-`NaN`, inequality tests for `NaN`.
(if comparison instanceof EQExpr then neg = "!" else neg = "") and
(if comparison instanceof AnyEqualsExpr then neg = "!" else neg = "") and
msg = "equivalent to " + neg + boxedName + ".isNaN(" + comparison.getLeftOperand() + ")"
)
}

View File

@@ -25,7 +25,7 @@ class BooleanExpr extends Expr {
private predicate assignAndCheck(AssignExpr e) {
exists(BinaryExpr c | e = c.getAChildExpr() |
c instanceof ComparisonExpr or
c instanceof EqualityTest
c instanceof AnyEqualityTest
)
}

View File

@@ -46,7 +46,7 @@ predicate comparisonMethod(Method m) { m.getName() = "compareTo" }
// Check for equalities of the form `a.x == b.x` or `a.x == x`, where `x` is assigned to `a.x`,
// which are less interesting but occur often.
predicate similarVarComparison(EqualityTest e) {
predicate similarVarComparison(AnyEqualityTest e) {
exists(Field f |
e.getLeftOperand() = f.getAnAccess() and
e.getRightOperand() = f.getAnAccess()
@@ -59,7 +59,7 @@ predicate similarVarComparison(EqualityTest e) {
)
}
from EqualityTest ee
from AnyEqualityTest ee
where
ee.getAnOperand().getType() instanceof Floating and
not ee.getAnOperand() instanceof NullLiteral and

View File

@@ -44,7 +44,7 @@ predicate constCond(BinaryExpr cond, boolean isTrue, Reason reason) {
isTrue = false and not comp.isStrict() and d1 > d2
)
or
exists(EqualityTest eq, Expr lhs, Expr rhs |
exists(AnyEqualityTest eq, Expr lhs, Expr rhs |
eq = cond and
lhs = eq.getLeftOperand() and
rhs = eq.getRightOperand()
@@ -192,7 +192,7 @@ predicate concurrentModificationTest(BinaryExpr test) {
* Holds if `test` and `guard` are equality tests of the same integral variable v with constants `c1` and `c2`.
*/
pragma[nomagic]
predicate guardedTest(EqualityTest test, Guard guard, boolean isEq, int i1, int i2) {
predicate guardedTest(AnyEqualityTest test, Guard guard, boolean isEq, int i1, int i2) {
exists(SsaVariable v, CompileTimeConstantExpr c1, CompileTimeConstantExpr c2 |
guard.isEquality(v.getAUse(), c1, isEq) and
test.hasOperands(v.getAUse(), c2) and
@@ -205,7 +205,7 @@ predicate guardedTest(EqualityTest test, Guard guard, boolean isEq, int i1, int
/**
* Holds if `guard` implies that `test` always has the value `testIsTrue`.
*/
predicate uselessEqTest(EqualityTest test, boolean testIsTrue, Guard guard) {
predicate uselessEqTest(AnyEqualityTest test, boolean testIsTrue, Guard guard) {
exists(boolean guardIsTrue, boolean guardpolarity, int i |
guardedTest(test, guard, guardpolarity, i, i) and
guard.controls(test.getBasicBlock(), guardIsTrue) and
@@ -218,7 +218,7 @@ where
(
if uselessEqTest(test, _, _)
then
exists(EqualityTest r |
exists(AnyEqualityTest r |
uselessEqTest(test, testIsTrue, r) and reason = ", because of $@" and reasonElem = r
)
else

View File

@@ -42,7 +42,7 @@ predicate uselessTest(ConditionNode s1, BinaryExpr test, boolean testIsTrue) {
exists(BoundKind boundKind, int bound |
// Simple range analysis. We infer a bound based on `cond` being
// either true (`condIsTrue = true`) or false (`condIsTrue = false`).
exists(EqualityTest condeq | cond = condeq and bound = k1 |
exists(AnyEqualityTest condeq | cond = condeq and bound = k1 |
condIsTrue = condeq.polarity() and boundKind.isEqual()
or
condIsTrue = condeq.polarity().booleanNot() and boundKind.isNotEqual()
@@ -74,7 +74,7 @@ predicate uselessTest(ConditionNode s1, BinaryExpr test, boolean testIsTrue) {
|
// Given the bound we check if the `test` is either
// always true (`testIsTrue = true`) or always false (`testIsTrue = false`).
exists(EqualityTest testeq, boolean pol | testeq = test and pol = testeq.polarity() |
exists(AnyEqualityTest testeq, boolean pol | testeq = test and pol = testeq.polarity() |
(
boundKind.providesLowerBound() and k2 < bound
or

View File

@@ -16,7 +16,7 @@ predicate nanField(Field f) {
f.hasName("NaN")
}
from EqualityTest eq, Field f, string classname
from AnyEqualityTest eq, Field f, string classname
where
eq.getAnOperand() = f.getAnAccess() and nanField(f) and f.getDeclaringType().hasName(classname)
select eq,

View File

@@ -21,7 +21,7 @@ private Expr getAFieldRead(Field f) {
* `f` or indirectly through a local variable `(x = f) == null`.
*/
private Expr getANullCheck(Field f) {
exists(EqualityTest eq | eq.polarity() = true |
exists(AnyEqualityTest eq | eq.polarity() = true |
eq.hasOperands(any(NullLiteral nl), getAFieldRead(f)) and
result = eq
)

View File

@@ -16,7 +16,7 @@
import java
/** A comparison (using `==`) with `null`. */
class NullEQExpr extends EQExpr {
class NullEQExpr extends AnyEqualsExpr {
NullEQExpr() { exists(NullLiteral l | l.getParent() = this) }
}

View File

@@ -16,7 +16,7 @@ private predicate inWeakCheck(Expr e) {
)
or
// Checking against `null` has no bearing on path traversal.
exists(EqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
exists(AnyEqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
}
// Ignore cases where the variable has been checked somehow,

View File

@@ -46,7 +46,7 @@ predicate boundedRead(RValue read) {
}
predicate castCheck(RValue read) {
exists(EqualityTest eq, CastExpr cast |
exists(AnyEqualityTest eq, CastExpr cast |
cast.getExpr() = read and
eq.hasOperands(cast, read.getVariable().getAnAccess())
)

View File

@@ -10,18 +10,18 @@
import java
class BoolCompare extends EqualityTest {
class BoolCompare extends AnyEqualityTest {
BoolCompare() { this.getAnOperand() instanceof BooleanLiteral }
predicate simplify(string pattern, string rewrite) {
exists(boolean b | b = this.getAnOperand().(BooleanLiteral).getBooleanValue() |
this instanceof EQExpr and b = true and pattern = "A == true" and rewrite = "A"
this instanceof AnyEqualsExpr and b = true and pattern = "A == true" and rewrite = "A"
or
this instanceof NEExpr and b = false and pattern = "A != false" and rewrite = "A"
this instanceof AnyNotEqualsExpr and b = false and pattern = "A != false" and rewrite = "A"
or
this instanceof EQExpr and b = false and pattern = "A == false" and rewrite = "!A"
this instanceof AnyEqualsExpr and b = false and pattern = "A == false" and rewrite = "!A"
or
this instanceof NEExpr and b = true and pattern = "A != true" and rewrite = "!A"
this instanceof AnyNotEqualsExpr and b = true and pattern = "A != true" and rewrite = "!A"
)
}
}
@@ -61,7 +61,7 @@ predicate conditionalWithBool(ConditionalExpr c, string pattern, string rewrite)
}
class ComparisonOrEquality extends BinaryExpr {
ComparisonOrEquality() { this instanceof ComparisonExpr or this instanceof EqualityTest }
ComparisonOrEquality() { this instanceof ComparisonExpr or this instanceof AnyEqualityTest }
predicate negate(string pattern, string rewrite) {
this instanceof EQExpr and pattern = "!(A == B)" and rewrite = "A != B"

View File

@@ -36,7 +36,7 @@ Variable flowTarget(Expr arg) {
*/
predicate unboxed(BoxedExpr e) {
exists(BinaryExpr bin | e = bin.getAnOperand() |
if bin instanceof EqualityTest or bin instanceof ComparisonExpr
if bin instanceof AnyEqualityTest or bin instanceof ComparisonExpr
then bin.getAnOperand() instanceof PrimitiveExpr
else bin instanceof PrimitiveExpr
)

View File

@@ -42,7 +42,7 @@ class AndroidFileLeakConfig extends TaintTracking::Configuration {
OnActivityForResultMethod oafr, ConditionBlock cb, CompileTimeConstantExpr cc,
VarAccess intentVar
|
cb.getCondition().(EQExpr).hasOperands(oafr.getParameter(0).getAnAccess(), cc) and
cb.getCondition().(AnyEqualsExpr).hasOperands(oafr.getParameter(0).getAnAccess(), cc) and
cc.getIntValue() = any(AndroidFileIntentInput fi).getRequestCode() and
intentVar.getType() instanceof TypeIntent and
cb.controls(intentVar.getBasicBlock(), true) and

View File

@@ -266,7 +266,7 @@ private predicate isNonConstantTimeComparisonCall(Expr firstInput, Expr secondIn
*/
private predicate existsFailFastCheck(Expr firstArray, Expr secondArray) {
exists(
Guard guard, EqualityTest eqTest, boolean branch, Stmt fastFailingStmt,
Guard guard, AnyEqualityTest eqTest, boolean branch, Stmt fastFailingStmt,
ArrayAccess firstArrayAccess, ArrayAccess secondArrayAccess
|
guard = eqTest and