From 1cf3196b611fc321ed5d30df09f0b38614290263 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 1 Oct 2020 15:52:07 +0200 Subject: [PATCH] Fix additional PR review findings --- .../rangeanalysis/SignAnalysisCommon.qll | 17 +-- .../rangeanalysis/SignAnalysisSpecific.qll | 139 +++++++++--------- .../rangeanalysis/SignAnalysisCommon.qll | 17 +-- .../rangeanalysis/SignAnalysisSpecific.qll | 37 ++--- .../dataflow/sign-analysis/SignAnalysis.ql | 2 +- 5 files changed, 101 insertions(+), 111 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll index 64f4a9d10c3..19412a0ba39 100644 --- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll @@ -328,21 +328,20 @@ Sign exprSign(Expr e) { /** Gets a possible sign for `e` from the signs of its child nodes. */ private Sign specificSubExprSign(Expr e) { - result = exprSign(getASubExpr(e)) + result = exprSign(getASubExprWithSameSign(e)) or - e = - any(DivExpr div | - result = exprSign(div.getLeftOperand()) and - result != TZero() and - div.getRightOperand().(RealLiteral).getValue().toFloat() = 0 - ) + exists(DivExpr div | div = e | + result = exprSign(div.getLeftOperand()) and + result != TZero() and + div.getRightOperand().(RealLiteral).getValue().toFloat() = 0 + ) or - exists(UnaryExpr unary | unary = e | + exists(UnaryOperation unary | unary = e | result = exprSign(unary.getOperand()).applyUnaryOp(unary.getOp()) ) or exists(Sign s1, Sign s2 | binaryOpSigns(e, s1, s2) | - result = s1.applyBinaryOp(s2, e.(BinaryExpr).getOp()) + result = s1.applyBinaryOp(s2, e.(BinaryOperation).getOp()) ) } diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll index ef1bff62f40..40e07eacbb9 100644 --- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll @@ -6,6 +6,7 @@ module Private { private import csharp as CS private import ConstantUtils as CU private import semmle.code.csharp.controlflow.Guards as G + private import Sign import Impl class Guard = G::Guard; @@ -42,7 +43,73 @@ module Private { class DivExpr = CS::DivExpr; - class BinaryOperation = CS::BinaryOperation; + /** Class to represent unary operation. */ + class UnaryOperation extends Expr { + UnaryOperation() { + this instanceof CS::PreIncrExpr or + this instanceof CS::PreDecrExpr or + this instanceof CS::UnaryMinusExpr or + this instanceof CS::ComplementExpr + } + + /** Returns the operand of this expression. */ + Expr getOperand() { + result = this.(CS::PreIncrExpr).getOperand() or + result = this.(CS::PreDecrExpr).getOperand() or + result = this.(CS::UnaryMinusExpr).getOperand() or + result = this.(CS::ComplementExpr).getOperand() + } + + /** Returns the operation representing this expression. */ + TUnarySignOperation getOp() { + this instanceof CS::PreIncrExpr and result = TIncOp() + or + this instanceof CS::PreDecrExpr and result = TDecOp() + or + this instanceof CS::UnaryMinusExpr and result = TNegOp() + or + this instanceof CS::ComplementExpr and result = TBitNotOp() + } + } + + /** Class to represent binary operation. */ + class BinaryOperation extends CS::BinaryOperation { + BinaryOperation() { + this instanceof CS::AddExpr or + this instanceof CS::SubExpr or + this instanceof CS::MulExpr or + this instanceof CS::DivExpr or + this instanceof CS::RemExpr or + this instanceof CS::BitwiseAndExpr or + this instanceof CS::BitwiseOrExpr or + this instanceof CS::BitwiseXorExpr or + this instanceof CS::LShiftExpr or + this instanceof CS::RShiftExpr + } + + /** Returns the operation representing this expression. */ + TBinarySignOperation getOp() { + this instanceof CS::AddExpr and result = TAddOp() + or + this instanceof CS::SubExpr and result = TSubOp() + or + this instanceof CS::MulExpr and result = TMulOp() + or + this instanceof CS::DivExpr and result = TDivOp() + or + this instanceof CS::RemExpr and result = TRemOp() + or + this instanceof CS::BitwiseAndExpr and result = TBitAndOp() + or + this instanceof CS::BitwiseOrExpr and result = TBitOrOp() + or + this instanceof CS::BitwiseXorExpr and result = TBitXorOp() + or + this instanceof CS::LShiftExpr and result = TLShiftOp() + or + this instanceof CS::RShiftExpr and result = TRShiftOp() + } + } predicate ssaRead = SU::ssaRead/2; } @@ -203,7 +270,7 @@ private module Impl { } /** Returns a sub expression of `e` for expression types where the sign depends on the child. */ - Expr getASubExpr(Expr e) { + Expr getASubExprWithSameSign(Expr e) { result = e.(AssignExpr).getRValue() or result = e.(AssignOperation).getExpandedAssignment() or result = e.(UnaryPlusExpr).getOperand() or @@ -218,74 +285,6 @@ private module Impl { result = e.(CastExpr).getExpr() } - /** Class to represent unary expressions. */ - class UnaryExpr extends Expr { - UnaryExpr() { - this instanceof PreIncrExpr or - this instanceof PreDecrExpr or - this instanceof UnaryMinusExpr or - this instanceof ComplementExpr - } - - /** Returns the operand of this expression. */ - Expr getOperand() { - result = this.(PreIncrExpr).getOperand() or - result = this.(PreDecrExpr).getOperand() or - result = this.(UnaryMinusExpr).getOperand() or - result = this.(ComplementExpr).getOperand() - } - - /** Returns the operation representing this expression. */ - TUnarySignOperation getOp() { - this instanceof PreIncrExpr and result = TIncOp() - or - this instanceof PreDecrExpr and result = TDecOp() - or - this instanceof UnaryMinusExpr and result = TNegOp() - or - this instanceof ComplementExpr and result = TBitNotOp() - } - } - - /** Class to represent binary expressions. */ - class BinaryExpr extends Expr { - BinaryExpr() { - this instanceof AddExpr or - this instanceof SubExpr or - this instanceof MulExpr or - this instanceof DivExpr or - this instanceof RemExpr or - this instanceof BitwiseAndExpr or - this instanceof BitwiseOrExpr or - this instanceof BitwiseXorExpr or - this instanceof LShiftExpr or - this instanceof RShiftExpr - } - - /** Returns the operation representing this expression. */ - TBinarySignOperation getOp() { - this instanceof AddExpr and result = TAddOp() - or - this instanceof SubExpr and result = TSubOp() - or - this instanceof MulExpr and result = TMulOp() - or - this instanceof DivExpr and result = TDivOp() - or - this instanceof RemExpr and result = TRemOp() - or - this instanceof BitwiseAndExpr and result = TBitAndOp() - or - this instanceof BitwiseOrExpr and result = TBitOrOp() - or - this instanceof BitwiseXorExpr and result = TBitXorOp() - or - this instanceof LShiftExpr and result = TLShiftOp() - or - this instanceof RShiftExpr and result = TRShiftOp() - } - } - Expr getARead(Ssa::Definition v) { result = v.getARead() } Field getField(FieldAccess fa) { result = fa.getTarget() } diff --git a/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll b/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll index 64f4a9d10c3..19412a0ba39 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll @@ -328,21 +328,20 @@ Sign exprSign(Expr e) { /** Gets a possible sign for `e` from the signs of its child nodes. */ private Sign specificSubExprSign(Expr e) { - result = exprSign(getASubExpr(e)) + result = exprSign(getASubExprWithSameSign(e)) or - e = - any(DivExpr div | - result = exprSign(div.getLeftOperand()) and - result != TZero() and - div.getRightOperand().(RealLiteral).getValue().toFloat() = 0 - ) + exists(DivExpr div | div = e | + result = exprSign(div.getLeftOperand()) and + result != TZero() and + div.getRightOperand().(RealLiteral).getValue().toFloat() = 0 + ) or - exists(UnaryExpr unary | unary = e | + exists(UnaryOperation unary | unary = e | result = exprSign(unary.getOperand()).applyUnaryOp(unary.getOp()) ) or exists(Sign s1, Sign s2 | binaryOpSigns(e, s1, s2) | - result = s1.applyBinaryOp(s2, e.(BinaryExpr).getOp()) + result = s1.applyBinaryOp(s2, e.(BinaryOperation).getOp()) ) } diff --git a/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll b/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll index 6f2c1026ea6..84fbe10a91d 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll @@ -45,21 +45,6 @@ module Private { class DivExpr = J::DivExpr; - class BinaryOperation extends J::Expr { - BinaryOperation() { - this instanceof J::BinaryExpr or - this instanceof J::AssignOp - } - - 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() - } - } - /** Class to represent float and double literals. */ class RealLiteral extends J::Literal { RealLiteral() { @@ -68,9 +53,9 @@ module Private { } } - /** Class to represent unary expressions. */ - class UnaryExpr extends J::Expr { - UnaryExpr() { + /** Class to represent unary operation. */ + class UnaryOperation extends J::Expr { + UnaryOperation() { this instanceof J::PreIncExpr or this instanceof J::PreDecExpr or this instanceof J::MinusExpr or @@ -97,9 +82,9 @@ module Private { } } - /** Class to represent binary expressions. */ - class BinaryExpr extends J::Expr { - BinaryExpr() { + /** Class to represent binary operation. */ + class BinaryOperation extends J::Expr { + BinaryOperation() { this instanceof J::AddExpr or this instanceof J::AssignAddExpr or this instanceof J::SubExpr or @@ -170,6 +155,14 @@ module Private { or this instanceof J::AssignURShiftExpr and result = TURShiftOp() } + + 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() + } } predicate ssaRead = RU::ssaRead/2; @@ -300,7 +293,7 @@ private module Impl { } /** Returns a sub expression of `e` for expression types where the sign depends on the child. */ - Expr getASubExpr(Expr e) { + Expr getASubExprWithSameSign(Expr e) { result = e.(AssignExpr).getSource() or result = e.(PlusExpr).getExpr() or result = e.(PostIncExpr).getExpr() or diff --git a/java/ql/test/library-tests/dataflow/sign-analysis/SignAnalysis.ql b/java/ql/test/library-tests/dataflow/sign-analysis/SignAnalysis.ql index e47081a0094..9d3c03a7d08 100644 --- a/java/ql/test/library-tests/dataflow/sign-analysis/SignAnalysis.ql +++ b/java/ql/test/library-tests/dataflow/sign-analysis/SignAnalysis.ql @@ -18,5 +18,5 @@ string getASignString(Expr e) { } from Expr e -where not e instanceof Element or e.(Element).fromSource() +where e.getEnclosingCallable().fromSource() select e, strictconcat(string s | s = getASignString(e) | s, " ")