From ec1d034ee051bdfa5ab8cdd70ad339ebfcf5f646 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 4 Mar 2026 14:27:53 +0100 Subject: [PATCH] Java: Make Assignment extend BinaryExpr. --- java/ql/lib/semmle/code/java/Expr.qll | 6 ++++-- .../ql/lib/semmle/code/java/PrettyPrintAst.qll | 17 ----------------- .../semmle/code/java/arithmetic/Overflow.qll | 13 ++++--------- .../semmle/code/java/controlflow/Guards.qll | 10 ++-------- .../lib/semmle/code/java/dataflow/Nullness.qll | 3 +-- .../code/java/dataflow/RangeAnalysis.qll | 18 +----------------- .../rangeanalysis/SignAnalysisSpecific.qll | 8 ++------ .../java/security/InsecureRandomnessQuery.qll | 3 ++- .../java/security/NumericCastTaintedQuery.qll | 5 +---- .../semmle/code/java/security/RandomQuery.qll | 5 ++--- .../src/Likely Bugs/Arithmetic/OctalLiteral.ql | 1 + .../WhitespaceContradictsPrecedence.ql | 1 + .../Comparison/CompareIdenticalValues.ql | 1 + .../Comparison/UselessComparisonTest.ql | 15 ++++----------- .../legacy/AutoBoxing.ql | 1 + .../test-kotlin1/library-tests/exprs/binop.ql | 1 + .../test-kotlin2/library-tests/exprs/binop.ql | 1 + .../StringComparison/StringComparison.expected | 6 +++--- 18 files changed, 32 insertions(+), 83 deletions(-) diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 712b65820ff..31135429e2d 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -392,7 +392,7 @@ class ArrayInit extends Expr, @arrayinit { * element assignments since there the assignment destination is not directly * the array variable but instead an `ArrayAccess`. */ -class Assignment extends Expr, @assignment { +class Assignment extends BinaryExpr, @assignment { /** Gets the destination (left-hand side) of the assignment. */ Expr getDest() { result.isNthChildOf(this, 0) } @@ -417,6 +417,8 @@ class Assignment extends Expr, @assignment { * For example, `x = 23`. */ class AssignExpr extends Assignment, @assignexpr { + override string getOp() { result = "=" } + override string getAPrimaryQlClass() { result = "AssignExpr" } } @@ -445,7 +447,7 @@ class AssignOp extends Assignment, @assignop { override Expr getSource() { result.getParent() = this } /** Gets a string representation of the assignment operator of this compound assignment. */ - /*abstract*/ string getOp() { result = "??=" } + /*abstract*/ override string getOp() { result = "??=" } /** Gets a printable representation of this expression. */ override string toString() { result = "..." + this.getOp() + "..." } diff --git a/java/ql/lib/semmle/code/java/PrettyPrintAst.qll b/java/ql/lib/semmle/code/java/PrettyPrintAst.qll index 25660ec3ec5..52374e6e384 100644 --- a/java/ql/lib/semmle/code/java/PrettyPrintAst.qll +++ b/java/ql/lib/semmle/code/java/PrettyPrintAst.qll @@ -207,23 +207,6 @@ private class PpArrayInit extends PpAst, ArrayInit { override PpAst getChild(int i) { exists(int j | result = this.getInit(j) and i = 1 + 2 * j) } } -private class PpAssignment extends PpAst, Assignment { - override string getPart(int i) { - i = 1 and - this instanceof AssignExpr and - result = " = " - or - i = 1 and - result = " " + this.(AssignOp).getOp() + " " - } - - override PpAst getChild(int i) { - i = 0 and result = this.getDest() - or - i = 2 and result = this.getRhs() - } -} - private class PpLiteral extends PpAst, Literal { override string getPart(int i) { i = 0 and result = this.getLiteral() } } diff --git a/java/ql/lib/semmle/code/java/arithmetic/Overflow.qll b/java/ql/lib/semmle/code/java/arithmetic/Overflow.qll index e82192b0fba..2cc3f8f0ad2 100644 --- a/java/ql/lib/semmle/code/java/arithmetic/Overflow.qll +++ b/java/ql/lib/semmle/code/java/arithmetic/Overflow.qll @@ -93,8 +93,7 @@ class ArithExpr extends Expr { ) and forall(Expr e | e = this.(BinaryExpr).getAnOperand() or - e = this.(UnaryAssignExpr).getOperand() or - e = this.(AssignOp).getSource() + e = this.(UnaryAssignExpr).getOperand() | e.getType() instanceof NumType ) @@ -114,21 +113,17 @@ class ArithExpr extends Expr { */ Expr getLeftOperand() { result = this.(BinaryExpr).getLeftOperand() or - result = this.(UnaryAssignExpr).getOperand() or - result = this.(AssignOp).getDest() + result = this.(UnaryAssignExpr).getOperand() } /** * Gets the right-hand operand if this is a binary expression. */ - Expr getRightOperand() { - result = this.(BinaryExpr).getRightOperand() or result = this.(AssignOp).getRhs() - } + Expr getRightOperand() { result = this.(BinaryExpr).getRightOperand() } /** Gets an operand of this arithmetic expression. */ Expr getAnOperand() { result = this.(BinaryExpr).getAnOperand() or - result = this.(UnaryAssignExpr).getOperand() or - result = this.(AssignOp).getSource() + result = this.(UnaryAssignExpr).getOperand() } } diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll index 5c0d0666f15..23088bd2f80 100644 --- a/java/ql/lib/semmle/code/java/controlflow/Guards.qll +++ b/java/ql/lib/semmle/code/java/controlflow/Guards.qll @@ -179,13 +179,7 @@ private module GuardsInput implements SharedGuards::InputSig { class ConstantIntegerExpr = RU::ConstantIntegerExpr; - abstract class BinaryExpr extends Expr { - Expr getLeftOperand() { - result = this.(J::BinaryExpr).getLeftOperand() or result = this.(J::AssignOp).getDest() - } - - Expr getRightOperand() { - result = this.(J::BinaryExpr).getRightOperand() or result = this.(J::AssignOp).getRhs() - } - - final Expr getAnOperand() { result = this.getLeftOperand() or result = this.getRightOperand() } - - final predicate hasOperands(Expr e1, Expr e2) { - this.getLeftOperand() = e1 and this.getRightOperand() = e2 - or - this.getLeftOperand() = e2 and this.getRightOperand() = e1 - } - } + class BinaryExpr = J::BinaryExpr; class AddExpr extends BinaryExpr { AddExpr() { this instanceof J::AddExpr or this instanceof J::AssignAddExpr } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll index e4525ed36ea..a10cca653ce 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll @@ -161,13 +161,9 @@ module Private { this instanceof J::AssignUnsignedRightShiftExpr and result = TUnsignedRightShiftOp() } - Expr getLeftOperand() { - result = this.(J::BinaryExpr).getLeftOperand() or result = this.(J::AssignOp).getDest() - } + Expr getLeftOperand() { result = this.(J::BinaryExpr).getLeftOperand() } - Expr getRightOperand() { - result = this.(J::BinaryExpr).getRightOperand() or result = this.(J::AssignOp).getRhs() - } + Expr getRightOperand() { result = this.(J::BinaryExpr).getRightOperand() } } predicate ssaRead = RU::ssaRead/2; diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll index 7474c977fe6..524de13513c 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -73,7 +73,8 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig { predicate isBarrierOut(DataFlow::Node n) { isSink(n) } predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { - n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() + n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() and + not n2.asExpr() instanceof AssignExpr or n1.asExpr() = n2.asExpr().(UnaryExpr).getOperand() or diff --git a/java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll b/java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll index 4b2d7709fbd..793871a4bd2 100644 --- a/java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll +++ b/java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll @@ -31,10 +31,7 @@ class RightShiftOp extends Expr { this instanceof AssignUnsignedRightShiftExpr } - private Expr getLhs() { - this.(BinaryExpr).getLeftOperand() = result or - this.(Assignment).getDest() = result - } + private Expr getLhs() { this.(BinaryExpr).getLeftOperand() = result } /** * Gets the variable that is shifted. diff --git a/java/ql/lib/semmle/code/java/security/RandomQuery.qll b/java/ql/lib/semmle/code/java/security/RandomQuery.qll index 49174a1f61a..88f8611379c 100644 --- a/java/ql/lib/semmle/code/java/security/RandomQuery.qll +++ b/java/ql/lib/semmle/code/java/security/RandomQuery.qll @@ -54,9 +54,8 @@ private module PredictableSeedFlowConfig implements DataFlow::ConfigSig { private module PredictableSeedFlow = DataFlow::Global; private predicate predictableCalcStep(Expr e1, Expr e2) { - e2.(BinaryExpr).hasOperands(e1, any(PredictableSeedExpr p)) - or - exists(AssignOp a | a = e2 | e1 = a.getDest() and a.getRhs() instanceof PredictableSeedExpr) + e2.(BinaryExpr).hasOperands(e1, any(PredictableSeedExpr p)) and + not e2 instanceof AssignExpr or exists(ConstructorCall cc, TypeNumber t | cc = e2 | cc.getArgument(0) = e1 and diff --git a/java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql b/java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql index 1a8021253df..a7745860089 100644 --- a/java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql +++ b/java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql @@ -19,6 +19,7 @@ where lit.getLiteral() = val and val.regexpMatch("0[0-7][0-7]+") and lit.getParent() instanceof BinaryExpr and + not lit.getParent() instanceof Assignment and not lit.getParent() instanceof BitwiseExpr and not lit.getParent() instanceof ComparisonExpr select lit, "Integer literal starts with 0." diff --git a/java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql b/java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql index 0f1909c2dd2..45eb62c0def 100644 --- a/java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql +++ b/java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql @@ -145,6 +145,7 @@ int operatorWS(BinaryExpr expr) { /** Find nested binary expressions where the programmer may have made a precedence mistake. */ predicate interestingNesting(BinaryExpr inner, BinaryExpr outer) { inner = outer.getAChildExpr() and + not outer instanceof Assignment and not inner instanceof AssocNestedExpr and not inner instanceof HarmlessNestedExpr and not inner.isParenthesized() diff --git a/java/ql/src/Likely Bugs/Comparison/CompareIdenticalValues.ql b/java/ql/src/Likely Bugs/Comparison/CompareIdenticalValues.ql index e46dd7c5f79..a5163bd9dfc 100644 --- a/java/ql/src/Likely Bugs/Comparison/CompareIdenticalValues.ql +++ b/java/ql/src/Likely Bugs/Comparison/CompareIdenticalValues.ql @@ -58,6 +58,7 @@ predicate equal(Expr left, Expr right) { sameVariable(left, right, _) or exists(BinaryExpr bLeft, BinaryExpr bRight | bLeft = left and bRight = right | + not bLeft instanceof Assignment and bLeft.getKind() = bRight.getKind() and equal(bLeft.getLeftOperand(), bRight.getLeftOperand()) and equal(bLeft.getRightOperand(), bRight.getRightOperand()) diff --git a/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql b/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql index d9a1f8a3f65..88a956bb24c 100644 --- a/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql +++ b/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql @@ -101,17 +101,10 @@ Expr overFlowCand() { | bin instanceof AddExpr or bin instanceof MulExpr or - bin instanceof LeftShiftExpr - ) - or - exists(AssignOp op | - result = op and - positive(op.getDest()) and - positive(op.getRhs()) - | - op instanceof AssignAddExpr or - op instanceof AssignMulExpr or - op instanceof AssignLeftShiftExpr + bin instanceof LeftShiftExpr or + bin instanceof AssignAddExpr or + bin instanceof AssignMulExpr or + bin instanceof AssignLeftShiftExpr ) or exists(AddExpr add, CompileTimeConstantExpr c | diff --git a/java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql b/java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql index 0fc6527d258..88a653e3b51 100644 --- a/java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql +++ b/java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql @@ -36,6 +36,7 @@ Variable flowTarget(Expr arg) { */ predicate unboxed(BoxedExpr e) { exists(BinaryExpr bin | e = bin.getAnOperand() | + not bin instanceof Assignment and if bin instanceof EqualityTest or bin instanceof ComparisonExpr then bin.getAnOperand() instanceof PrimitiveExpr else bin instanceof PrimitiveExpr diff --git a/java/ql/test-kotlin1/library-tests/exprs/binop.ql b/java/ql/test-kotlin1/library-tests/exprs/binop.ql index d865d83fcf9..303eccba589 100644 --- a/java/ql/test-kotlin1/library-tests/exprs/binop.ql +++ b/java/ql/test-kotlin1/library-tests/exprs/binop.ql @@ -41,4 +41,5 @@ MaybeElement rhs(BinaryExpr e) { } from Expr e +where not e instanceof Assignment select e, lhs(e), rhs(e) diff --git a/java/ql/test-kotlin2/library-tests/exprs/binop.ql b/java/ql/test-kotlin2/library-tests/exprs/binop.ql index d865d83fcf9..303eccba589 100644 --- a/java/ql/test-kotlin2/library-tests/exprs/binop.ql +++ b/java/ql/test-kotlin2/library-tests/exprs/binop.ql @@ -41,4 +41,5 @@ MaybeElement rhs(BinaryExpr e) { } from Expr e +where not e instanceof Assignment select e, lhs(e), rhs(e) diff --git a/java/ql/test/query-tests/StringComparison/StringComparison.expected b/java/ql/test/query-tests/StringComparison/StringComparison.expected index 566244cc8f0..485fc58ed8a 100644 --- a/java/ql/test/query-tests/StringComparison/StringComparison.expected +++ b/java/ql/test/query-tests/StringComparison/StringComparison.expected @@ -1,3 +1,3 @@ -| StringComparison.java:23:6:23:19 | ... == ... | String values compared with == . | -| StringComparison.java:26:6:26:16 | ... == ... | String values compared with == . | -| StringComparison.java:29:6:29:20 | ... == ... | String values compared with == . | \ No newline at end of file +| StringComparison.java:23:6:23:19 | ... == ... | String values compared with ==. | +| StringComparison.java:26:6:26:16 | ... == ... | String values compared with ==. | +| StringComparison.java:29:6:29:20 | ... == ... | String values compared with ==. |