From 13ffefc8b8633f2fe708c5b979afc0e7d4aef57a Mon Sep 17 00:00:00 2001
From: rikardfalkeborn <rikard.falkeborn@gmail.com>
Date: Tue, 1 Jan 2019 14:15:50 +0100
Subject: [PATCH] Valueflow: Fix right shift with more than 31 bits (#1553)

When comparing if the shift is large enough to make the result zero, use
an unsigned long long to make sure the result fits. Also, a check that
avoids setting the value if the shift is equal to or larger than the
number of bits in the operand (this is undefined behaviour). Finally,
add a check to make sure the calculated value is not too large to store.

Add test cases to cover this.

This was detected by an MSVC warning.

valueflow.cpp(1350): warning C4334: '<<' : result of 32-bit shift implicitly
                     converted to 64 bits (was 64-bit shift intended?)
---
 lib/valueflow.cpp      | 18 ++++++++++--
 test/testvalueflow.cpp | 67 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
index 2ba7df878..db46355c0 100644
--- a/lib/valueflow.cpp
+++ b/lib/valueflow.cpp
@@ -1315,7 +1315,7 @@ static bool getExpressionRange(const Token *expr, MathLib::bigint *minvalue, Mat
     return false;
 }
 
-static void valueFlowRightShift(TokenList *tokenList)
+static void valueFlowRightShift(TokenList *tokenList, const Settings* settings)
 {
     for (Token *tok = tokenList->front(); tok; tok = tok->next()) {
         if (tok->str() != ">>")
@@ -1345,7 +1345,19 @@ static void valueFlowRightShift(TokenList *tokenList)
             continue;
         if (lhsmax < 0)
             continue;
-        if ((1 << rhsvalue) <= lhsmax)
+        int lhsbits;
+        if ((tok->astOperand1()->valueType()->type == ValueType::Type::CHAR) ||
+            (tok->astOperand1()->valueType()->type == ValueType::Type::SHORT) ||
+            (tok->astOperand1()->valueType()->type == ValueType::Type::BOOL) ||
+            (tok->astOperand1()->valueType()->type == ValueType::Type::INT))
+            lhsbits = settings->int_bit;
+        else if (tok->astOperand1()->valueType()->type == ValueType::Type::LONG)
+            lhsbits = settings->long_bit;
+        else if (tok->astOperand1()->valueType()->type == ValueType::Type::LONGLONG)
+            lhsbits = settings->long_long_bit;
+        else
+            continue;
+        if (rhsvalue >= lhsbits || rhsvalue >= MathLib::bigint_bits || (1ULL << rhsvalue) <= lhsmax)
             continue;
 
         ValueFlow::Value val(0);
@@ -4764,7 +4776,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
     std::size_t values = 0;
     while (std::time(0) < timeout && values < getTotalValues(tokenlist)) {
         values = getTotalValues(tokenlist);
-        valueFlowRightShift(tokenlist);
+        valueFlowRightShift(tokenlist, settings);
         valueFlowOppositeCondition(symboldatabase, settings);
         valueFlowTerminatingCondition(tokenlist, symboldatabase, settings);
         valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings);
diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp
index be9691f1a..89d418afc 100644
--- a/test/testvalueflow.cpp
+++ b/test/testvalueflow.cpp
@@ -2341,6 +2341,11 @@ private:
 
     void valueFlowRightShift() {
         const char *code;
+        /* Set some temporary fixed values to simplify testing */
+        const Settings settingsTmp = settings;
+        settings.int_bit = 32;
+        settings.long_bit = 64;
+        settings.long_long_bit = MathLib::bigint_bits * 2;
 
         code = "int f(int a) {\n"
                "  int x = (a & 0xff) >> 16;\n"
@@ -2353,6 +2358,68 @@ private:
                "  return x;\n"
                "}";
         ASSERT_EQUALS(true, testValueOfX(code,3U,0));
+
+        code = "int f(int y) {\n"
+               "  int x = (y & 0xFFFFFFF) >> 31;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(true, testValueOfX(code, 3u, 0));
+
+        code = "int f(int y) {\n"
+               "  int x = (y & 0xFFFFFFF) >> 32;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        code = "int f(short y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 31;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(true, testValueOfX(code, 3u, 0));
+
+        code = "int f(short y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 32;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        code = "int f(long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 63;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(true, testValueOfX(code, 3u, 0));
+
+        code = "int f(long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 64;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        code = "int f(long long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 63;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(true, testValueOfX(code, 3u, 0));
+
+        code = "int f(long long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 64;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        code = "int f(long long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 121;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        code = "int f(long long y) {\n"
+               "  int x = (y & 0xFFFFFF) >> 128;\n"
+               "  return x;\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 3u, 0));
+
+        settings = settingsTmp;
     }
 
     void valueFlowFwdAnalysis() {