From 1758e25207cad070ffbad8226288d3c63eac777c Mon Sep 17 00:00:00 2001 From: Gulshan Singh Date: Tue, 24 Jan 2023 22:44:02 -0800 Subject: [PATCH] Merge lshift/rshift range expressions into a single file and address PR comments --- .../extensions/ConstantRShiftExprRange.qll | 136 ------------------ ...prRange.qll => ConstantShiftExprRange.qll} | 129 +++++++++++++++-- 2 files changed, 120 insertions(+), 145 deletions(-) delete mode 100644 cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantRShiftExprRange.qll rename cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/{ConstantLShiftExprRange.qll => ConstantShiftExprRange.qll} (53%) diff --git a/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantRShiftExprRange.qll b/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantRShiftExprRange.qll deleted file mode 100644 index 5f7384048c0..00000000000 --- a/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantRShiftExprRange.qll +++ /dev/null @@ -1,136 +0,0 @@ -private import cpp -private import experimental.semmle.code.cpp.models.interfaces.SimpleRangeAnalysisExpr -private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils - -float evaluateConstantExpr1(Expr e) { - result = e.getValue().toFloat() - or - // This handles when a constant value is put into a variable - // and the variable is used later - exists(SsaDefinition defn, StackVariable sv | - defn.getAUse(sv) = e and - result = defn.getDefiningValue(sv).getValue().toFloat() - ) -} - -// If the constant right operand is negative or is greater than or equal to the number of -// bits in the left operands type, then the result is undefined (except on the IA-32 -// architecture where the shift value is masked with 0b00011111, but we can't -// assume the architecture). -bindingset[val] -pragma[inline] -private predicate isValidShiftExprShift(float val, Expr l) { - val >= 0 and - // We use getFullyConverted because the spec says to use the *promoted* left operand - val < (l.getFullyConverted().getUnderlyingType().getSize() * 8) -} - -/** - * This handles the `>>` and `>>=` operators when at least one operand is a constant (and if the - * right operand is a constant, it must be "valid" (see `isValidShiftExprShift`)). When handling any - * undefined behavior, it leaves the values unconstrained. From the C++ standard: "The behavior is - * undefined if the right operand is negative, or greater than or equal to the length in bits of the - * promoted left operand. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an - * unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the - * integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the - * resulting value is implementation-defined." - */ -class ConstantRShiftExprRange extends SimpleRangeAnalysisExpr { - /** - * Holds for `a >> b` or `a >>= b` in one of the following two cases: - * 1. `a` is a constant and `b` is not - * 2. `b` is constant - * - * We don't handle the case where `a` and `b` are both non-constant values. - */ - ConstantRShiftExprRange() { - getUnspecifiedType() instanceof IntegralType and - exists(Expr l, Expr r | - l = this.(RShiftExpr).getLeftOperand() and - r = this.(RShiftExpr).getRightOperand() - or - l = this.(AssignRShiftExpr).getLValue() and - r = this.(AssignRShiftExpr).getRValue() - | - l.getUnspecifiedType() instanceof IntegralType and - r.getUnspecifiedType() instanceof IntegralType and - ( - // If the left operand is a constant, verify that the right operand is not a constant - exists(evaluateConstantExpr1(l)) and not exists(evaluateConstantExpr1(r)) - or - // If the right operand is a constant, check if it is a valid shift expression - exists(float constROp | - constROp = evaluateConstantExpr1(r) and isValidShiftExprShift(constROp, l) - ) - ) - ) - } - - Expr getLeftOperand() { - result = this.(RShiftExpr).getLeftOperand() or - result = this.(AssignRShiftExpr).getLValue() - } - - Expr getRightOperand() { - result = this.(RShiftExpr).getRightOperand() or - result = this.(AssignRShiftExpr).getRValue() - } - - override float getLowerBounds() { - exists(int lLower, int lUpper, int rLower, int rUpper | - lLower = getFullyConvertedLowerBounds(getLeftOperand()) and - lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and - rLower = getFullyConvertedLowerBounds(getRightOperand()) and - rUpper = getFullyConvertedUpperBounds(getRightOperand()) and - lLower <= lUpper and - rLower <= rUpper - | - if - lLower < 0 - or - not ( - isValidShiftExprShift(rLower, getLeftOperand()) and - isValidShiftExprShift(rUpper, getLeftOperand()) - ) - then - // We don't want to deal with shifting negative numbers at the moment, - // and a negative shift is implementation defined, so we set the result - // to the minimum value - result = exprMinVal(this) - else - // We can get the smallest value by shifting the smallest bound by the largest bound - result = lLower.bitShiftRight(rUpper) - ) - } - - override float getUpperBounds() { - exists(int lLower, int lUpper, int rLower, int rUpper | - lLower = getFullyConvertedLowerBounds(getLeftOperand()) and - lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and - rLower = getFullyConvertedLowerBounds(getRightOperand()) and - rUpper = getFullyConvertedUpperBounds(getRightOperand()) and - lLower <= lUpper and - rLower <= rUpper - | - if - lLower < 0 - or - not ( - isValidShiftExprShift(rLower, getLeftOperand()) and - isValidShiftExprShift(rUpper, getLeftOperand()) - ) - then - // We don't want to deal with shifting negative numbers at the moment, - // and a negative shift is implementation defined, so we set the result - // to the maximum value - result = exprMaxVal(this) - else - // We can get the largest value by shifting the largest bound by the smallest bound - result = lUpper.bitShiftRight(rLower) - ) - } - - override predicate dependsOnChild(Expr child) { - child = getLeftOperand() or child = getLeftOperand() - } -} diff --git a/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantLShiftExprRange.qll b/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantShiftExprRange.qll similarity index 53% rename from cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantLShiftExprRange.qll rename to cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantShiftExprRange.qll index 2bcc4c5ec98..253f45b018c 100644 --- a/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantLShiftExprRange.qll +++ b/cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/extensions/ConstantShiftExprRange.qll @@ -2,7 +2,7 @@ private import cpp private import experimental.semmle.code.cpp.models.interfaces.SimpleRangeAnalysisExpr private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils -float evaluateConstantExpr2(Expr e) { +float evaluateConstantExpr(Expr e) { result = e.getValue().toFloat() or // This handles when a constant value is put into a variable @@ -18,7 +18,6 @@ float evaluateConstantExpr2(Expr e) { // architecture where the shift value is masked with 0b00011111, but we can't // assume the architecture). bindingset[val] -pragma[inline] private predicate isValidShiftExprShift(float val, Expr l) { val >= 0 and // We use getFullyConverted because the spec says to use the *promoted* left operand @@ -32,9 +31,121 @@ private predicate canLShiftOverflow(int val, int shift, int max_val) { } /** - * This handles the `<<` and `<<=` operators when at least one operand is a constant (and if the right operand - * is a constant, it must be "valid" (see `isValidShiftExprShift`)). When handling any undefined behavior, it - * leaves the values unconstrained. From the C++ standard: "The behavior is undefined if the right + * A range analysis expression consisting of the `>>` or `>>=` operator when at least + * one operand is a constant (and if the right operand is a constant, it must be "valid" + * (see `isValidShiftExprShift`)). When handling any undefined behavior, it leaves the + * values unconstrained. From the C++ standard: "The behavior is undefined if the right + * operand is negative, or greater than or equal to the length in bits of the promoted + * left operand. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an + * unsigned type or if E1 has a signed type and a non-negative value, the value of the + * result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a + * negative value, the resulting value is implementation-defined." + */ +class ConstantRShiftExprRange extends SimpleRangeAnalysisExpr { + /** + * Holds for `a >> b` or `a >>= b` in one of the following two cases: + * 1. `a` is a constant and `b` is not + * 2. `b` is constant + * + * We don't handle the case where `a` and `b` are both non-constant values. + */ + ConstantRShiftExprRange() { + getUnspecifiedType() instanceof IntegralType and + exists(Expr l, Expr r | + l = this.(RShiftExpr).getLeftOperand() and + r = this.(RShiftExpr).getRightOperand() + or + l = this.(AssignRShiftExpr).getLValue() and + r = this.(AssignRShiftExpr).getRValue() + | + l.getUnspecifiedType() instanceof IntegralType and + r.getUnspecifiedType() instanceof IntegralType and + ( + // If the left operand is a constant, verify that the right operand is not a constant + exists(evaluateConstantExpr(l)) and not exists(evaluateConstantExpr(r)) + or + // If the right operand is a constant, check if it is a valid shift expression + exists(float constROp | + constROp = evaluateConstantExpr(r) and isValidShiftExprShift(constROp, l) + ) + ) + ) + } + + Expr getLeftOperand() { + result = this.(RShiftExpr).getLeftOperand() or + result = this.(AssignRShiftExpr).getLValue() + } + + Expr getRightOperand() { + result = this.(RShiftExpr).getRightOperand() or + result = this.(AssignRShiftExpr).getRValue() + } + + override float getLowerBounds() { + exists(int lLower, int lUpper, int rLower, int rUpper | + lLower = getFullyConvertedLowerBounds(getLeftOperand()) and + lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and + rLower = getFullyConvertedLowerBounds(getRightOperand()) and + rUpper = getFullyConvertedUpperBounds(getRightOperand()) and + lLower <= lUpper and + rLower <= rUpper + | + if + lLower < 0 + or + not ( + isValidShiftExprShift(rLower, getLeftOperand()) and + isValidShiftExprShift(rUpper, getLeftOperand()) + ) + then + // We don't want to deal with shifting negative numbers at the moment, + // and a negative shift is implementation defined, so we set the result + // to the minimum value + result = exprMinVal(this) + else + // We can get the smallest value by shifting the smallest bound by the largest bound + result = lLower.bitShiftRight(rUpper) + ) + } + + override float getUpperBounds() { + exists(int lLower, int lUpper, int rLower, int rUpper | + lLower = getFullyConvertedLowerBounds(getLeftOperand()) and + lUpper = getFullyConvertedUpperBounds(getLeftOperand()) and + rLower = getFullyConvertedLowerBounds(getRightOperand()) and + rUpper = getFullyConvertedUpperBounds(getRightOperand()) and + lLower <= lUpper and + rLower <= rUpper + | + if + lLower < 0 + or + not ( + isValidShiftExprShift(rLower, getLeftOperand()) and + isValidShiftExprShift(rUpper, getLeftOperand()) + ) + then + // We don't want to deal with shifting negative numbers at the moment, + // and a negative shift is implementation defined, so we set the result + // to the maximum value + result = exprMaxVal(this) + else + // We can get the largest value by shifting the largest bound by the smallest bound + result = lUpper.bitShiftRight(rLower) + ) + } + + override predicate dependsOnChild(Expr child) { + child = getLeftOperand() or child = getLeftOperand() + } +} + +/** + * A range analysis expression consisting of the `<<` or `<<=` operator when at least + * one operand is a constant (and if the right operand is a constant, it must be "valid" + * (see `isValidShiftExprShift`)). When handling any undefined behavior, it leaves the + * values unconstrained. From the C++ standard: "The behavior is undefined if the right * operand is negative, or greater than or equal to the length in bits of the promoted left operand. * The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 * has an unsigned type, the value of the result is E1 × 2 E2, reduced modulo one more than the @@ -64,11 +175,11 @@ class ConstantLShiftExprRange extends SimpleRangeAnalysisExpr { r.getUnspecifiedType() instanceof IntegralType and ( // If the left operand is a constant, verify that the right operand is not a constant - exists(evaluateConstantExpr2(l)) and not exists(evaluateConstantExpr2(r)) + exists(evaluateConstantExpr(l)) and not exists(evaluateConstantExpr(r)) or // If the right operand is a constant, check if it is a valid shift expression exists(float constROp | - constROp = evaluateConstantExpr2(r) and isValidShiftExprShift(constROp, l) + constROp = evaluateConstantExpr(r) and isValidShiftExprShift(constROp, l) ) ) ) @@ -109,7 +220,7 @@ class ConstantLShiftExprRange extends SimpleRangeAnalysisExpr { // (a shift of 1) but doing a shift by the upper bound would give 0b01000000. // So if the left shift operation causes an overflow, we just assume the max value // If necessary, we may be able to improve this bound in the future - if canLShiftOverflow(lUpper, rUpper, exprMaxVal(this).(int)) + if canLShiftOverflow(lUpper, rUpper, exprMaxVal(this)) then result = exprMinVal(this) else result = lLower.bitShiftLeft(rLower) ) @@ -140,7 +251,7 @@ class ConstantLShiftExprRange extends SimpleRangeAnalysisExpr { // (a shift of 1) but doing a shift by the upper bound would give 0b01000000. // So if the left shift operation causes an overflow, we just assume the max value // If necessary, we may be able to improve this bound in the future - if canLShiftOverflow(lUpper, rUpper, exprMaxVal(this).(int)) + if canLShiftOverflow(lUpper, rUpper, exprMaxVal(this)) then result = exprMaxVal(this) else result = lUpper.bitShiftLeft(rUpper) )