From 7233ffa50f249db18aae7b8fc8fc2ebd251a094e Mon Sep 17 00:00:00 2001 From: Gulshan Singh Date: Tue, 6 Oct 2020 23:57:05 -0700 Subject: [PATCH] Address review comments --- .../rangeanalysis/BinaryOrAssignOperation.qll | 31 ----- .../ConstantBitwiseAndExprRange.qll | 130 ++++++++---------- 2 files changed, 55 insertions(+), 106 deletions(-) delete mode 100644 cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/BinaryOrAssignOperation.qll diff --git a/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/BinaryOrAssignOperation.qll b/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/BinaryOrAssignOperation.qll deleted file mode 100644 index 1670f4ebcd0..00000000000 --- a/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/BinaryOrAssignOperation.qll +++ /dev/null @@ -1,31 +0,0 @@ -private import cpp - -Expr getLOp(Operation o) { - result = o.(BinaryOperation).getLeftOperand() or - result = o.(Assignment).getLValue() -} - -Expr getROp(Operation o) { - result = o.(BinaryOperation).getRightOperand() or - result = o.(Assignment).getRValue() -} - -private newtype TBinaryOrAssignOperation = - BinaryOp(BinaryOperation op) or - AssignOp(AssignOperation op) - -class BinaryOrAssignOperation extends TBinaryOrAssignOperation { - BinaryOperation asBinaryOp() { this = BinaryOp(result) } - - AssignOperation asAssignOp() { this = AssignOp(result) } - - Expr getLeftOperand() { result = getLOp(asBinaryOp()) or result = getLOp(asAssignOp()) } - - Expr getRightOperand() { result = getROp(asBinaryOp()) or result = getROp(asAssignOp()) } - - Expr getAnOperand() { result = getLeftOperand() or result = getRightOperand() } - - Operation getOperation() { result = asBinaryOp() or result = asAssignOp() } - - string toString() { result = asBinaryOp().toString() or result = asAssignOp().toString() } -} diff --git a/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantBitwiseAndExprRange.qll b/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantBitwiseAndExprRange.qll index 27f9733b557..e4c76e16120 100644 --- a/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantBitwiseAndExprRange.qll +++ b/cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantBitwiseAndExprRange.qll @@ -1,50 +1,6 @@ private import cpp private import experimental.semmle.code.cpp.models.interfaces.SimpleRangeAnalysisExpr private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils -private import experimental.semmle.code.cpp.rangeanalysis.BinaryOrAssignOperation - -/** - * The current implementation for `BitwiseAndExpr` only handles cases where both operands are - * either unsigned or non-negative constants. This class not only covers these cases, but also - * adds support for `&` expressions between a signed integer with a non-negative range and a - * non-negative constant. It also adds support for `&=` for the same set of cases as `&`. - */ -private class ConstantBitwiseAndExprRange extends SimpleRangeAnalysisExpr { - BinaryOrAssignConstantBitwiseAndExpr e; - - ConstantBitwiseAndExprRange() { this = e.getOperation() } - - BinaryOrAssignConstantBitwiseAndExpr getExpr() { result = e } - - Expr getLeftOperand() { result = e.getLeftOperand() } - - Expr getRightOperand() { result = e.getRightOperand() } - - override float getLowerBounds() { result = e.getLowerBounds() } - - override float getUpperBounds() { result = e.getUpperBounds() } - - override predicate dependsOnChild(Expr child) { child = e.getAnOperand() } -} - -private class ConstantBitwiseAndExprOp extends Expr { - BinaryOrAssignConstantBitwiseAndExpr b; - float lowerBound; - float upperBound; - - ConstantBitwiseAndExprOp() { - this = b.getAnOperand() and - lowerBound = getFullyConvertedLowerBounds(this) and - upperBound = getFullyConvertedUpperBounds(this) and - lowerBound <= upperBound - } - - float getLowerBound() { result = lowerBound } - - float getUpperBound() { result = upperBound } - - predicate hasNegativeRange() { getLowerBound() < 0 or getUpperBound() < 0 } -} /** * Holds if `e` is a constant or if it is a variable with a constant value @@ -58,32 +14,49 @@ float evaluateConstantExpr(Expr e) { ) } -private class BinaryOrAssignConstantBitwiseAndExpr extends BinaryOrAssignOperation { - BinaryOrAssignConstantBitwiseAndExpr() { - ( - getOperation() instanceof BitwiseAndExpr +/** + * The current implementation for `BitwiseAndExpr` only handles cases where both operands are + * either unsigned or non-negative constants. This class not only covers these cases, but also + * adds support for `&` expressions between a signed integer with a non-negative range and a + * non-negative constant. It also adds support for `&=` for the same set of cases as `&`. + */ +private class ConstantBitwiseAndExprRange extends SimpleRangeAnalysisExpr { + ConstantBitwiseAndExprRange() { + exists(Expr l, Expr r | + l = this.(BitwiseAndExpr).getLeftOperand() and + r = this.(BitwiseAndExpr).getRightOperand() or - getOperation() instanceof AssignAndExpr - ) and - // Make sure all operands and the result type are integral - getOperation().getUnspecifiedType() instanceof IntegralType and - getLeftOperand().getUnspecifiedType() instanceof IntegralType and - getRightOperand().getUnspecifiedType() instanceof IntegralType and - // No operands can be negative constants - not (evaluateConstantExpr(getLeftOperand()) < 0 or evaluateConstantExpr(getRightOperand()) < 0) and - // At least one operand must be a non-negative constant - (evaluateConstantExpr(getLeftOperand()) >= 0 or evaluateConstantExpr(getRightOperand()) >= 0) + l = this.(AssignAndExpr).getLValue() and + r = this.(AssignAndExpr).getRValue() + | + // No operands can be negative constants + not (evaluateConstantExpr(getLOp(this)) < 0 or evaluateConstantExpr(getROp(this)) < 0) and + // At least one operand must be a non-negative constant + (evaluateConstantExpr(getLOp(this)) >= 0 or evaluateConstantExpr(getROp(this)) >= 0) + ) } - float getLowerBounds() { - // If both operands have non-negative ranges, the lower bound is zero. If an operand can have - // negative values, the lower bound is unconstrained. - exists(ConstantBitwiseAndExprOp l, ConstantBitwiseAndExprOp r | - l = getLeftOperand() and - r = getRightOperand() and + Expr getLeftOperand() { + result = this.(BitwiseAndExpr).getLeftOperand() or + result = this.(AssignAndExpr).getLValue() + } + + Expr getRightOperand() { + result = this.(BitwiseAndExpr).getRightOperand() or + result = this.(AssignAndExpr).getRValue() + } + + override float getLowerBounds() { + // If an operand can have negative values, the lower bound is unconstrained. + // Otherwise, the lower bound is zero. + exists(float lLower, float lUpper, float rLower, float rUpper | + lLower = getFullyConvertedLowerBounds(getLeftOperand()) and + lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and + rLower = getFullyConvertedLowerBounds(getRightOperand()) and + rUpper = getFullyConvertedUpperBounds(getRightOperand()) and ( - (l.hasNegativeRange() or r.hasNegativeRange()) and - result = exprMinVal(getOperation()) + (lLower < 0 or rLower < 0) and + result = exprMinVal(this) or // This technically results in two lowerBounds when an operand range is negative, but // that's fine since `exprMinVal(x) <= 0`. We can't use an if statement here without @@ -93,20 +66,27 @@ private class BinaryOrAssignConstantBitwiseAndExpr extends BinaryOrAssignOperati ) } - float getUpperBounds() { + override float getUpperBounds() { // If an operand can have negative values, the upper bound is unconstrained. - // Otherwise, the upper bound is the maximum of the upper bounds of the operands - exists(ConstantBitwiseAndExprOp l, ConstantBitwiseAndExprOp r | - l = getLeftOperand() and - r = getRightOperand() and + // Otherwise, the upper bound is the minimum of the upper bounds of the operands + exists(float lLower, float lUpper, float rLower, float rUpper | + lLower = getFullyConvertedLowerBounds(getLeftOperand()) and + lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and + rLower = getFullyConvertedLowerBounds(getRightOperand()) and + rUpper = getFullyConvertedUpperBounds(getRightOperand()) and ( - (l.hasNegativeRange() or r.hasNegativeRange()) and - result = exprMaxVal(getOperation()) + (lLower < 0 or rLower < 0) and + result = exprMaxVal(this) or // This technically results in two upperBounds when an operand range is negative, but - // that's fine since `exprMaxVal(b) >= result` - result = r.getUpperBound().minimum(l.getUpperBound()) + // that's fine since `exprMaxVal(b) >= result`. We can't use an if statement here without + // non-monotonic recursion issues + result = rUpper.minimum(lUpper) ) ) } + + override predicate dependsOnChild(Expr child) { + child = getLeftOperand() or child = getRightOperand() + } }