From 6375fb4f085639ad50dfcf994b50e5b8a84ddbbe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Sun, 3 Jan 2021 21:03:07 +0100
Subject: [PATCH] New check: Warn about nonportable code that can be optimised
 (signed integer overflow)

---
 lib/checktype.cpp | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/checktype.h   |  6 ++++++
 test/testtype.cpp | 30 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/lib/checktype.cpp b/lib/checktype.cpp
index dade64c55..ef17e3593 100644
--- a/lib/checktype.cpp
+++ b/lib/checktype.cpp
@@ -222,6 +222,55 @@ void CheckType::integerOverflowError(const Token *tok, const ValueFlow::Value &v
                 value.isInconclusive());
 }
 
+//---------------------------------------------------------------------------------
+// Checking for code patterns that might be optimised differently by the compilers
+//---------------------------------------------------------------------------------
+
+void CheckType::checkIntegerOverflowOptimisations()
+{
+    // Various patterns are defined here:
+    // https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html
+
+    // These optimisations seem to generate "unwanted" output
+    // x + c < x       ->   false
+    // x + c <= x      ->   false
+    // x + c > x       ->   true
+    // x + c >= x      ->   true
+
+    if (!mSettings->isEnabled(Settings::PORTABILITY))
+        return;
+
+    for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
+        if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp())
+            continue;
+
+        if (Token::Match(tok->astOperand1(), "[+-]") &&
+            tok->astOperand1()->valueType() &&
+            tok->astOperand1()->valueType()->isIntegral() &&
+            tok->astOperand1()->valueType()->sign == ValueType::Sign::SIGNED &&
+            tok->astOperand1()->isBinaryOp() &&
+            tok->astOperand1()->astOperand2()->isNumber() &&
+            Token::Match(tok->astOperand1()->astOperand1(), "%var%") &&
+            tok->astOperand1()->astOperand1()->str() == tok->astOperand2()->str()) {
+            bool result;
+            if (tok->astOperand1()->str() == "+")
+                result = Token::Match(tok, ">|>=");
+            else
+                result = Token::Match(tok, "<|<=");
+            integerOverflowOptimisationError(tok, result ? "true" : "false");
+        }
+    }
+}
+
+void CheckType::integerOverflowOptimisationError(const Token *tok, const std::string &replace)
+{
+    const std::string expr = tok ? tok->expressionString() : "x+c<x";
+    const std::string errmsg =
+        "There is a danger that '" + expr + "' will be optimised into '" + replace + "'. Signed integer overflow is undefined behavior.\n"
+        "There is a danger that '" + expr + "' will be optimised into '" + replace + "'. Your code could work differently depending on what compiler/flags/version/etc is used. Signed integer overflow is undefined behavior and assuming that it will never happen; the result of '" + expr + "' is always '" + replace + "'.";
+    reportError(tok, Severity::portability, "integerOverflowOptimization", errmsg, CWE190, false);
+}
+
 //---------------------------------------------------------------------------
 // Checking for sign conversion when operand can be negative
 //---------------------------------------------------------------------------
diff --git a/lib/checktype.h b/lib/checktype.h
index e51da8d1d..b897d61a8 100644
--- a/lib/checktype.h
+++ b/lib/checktype.h
@@ -54,6 +54,7 @@ public:
         CheckType checkType(tokenizer, settings, errorLogger);
         checkType.checkTooBigBitwiseShift();
         checkType.checkIntegerOverflow();
+        checkType.checkIntegerOverflowOptimisations();
         checkType.checkSignConversion();
         checkType.checkLongCast();
         checkType.checkFloatToIntegerOverflow();
@@ -65,6 +66,9 @@ public:
     /** @brief %Check for integer overflow */
     void checkIntegerOverflow();
 
+    /** @brief Check for overflow code patterns that will be optimized */
+    void checkIntegerOverflowOptimisations();
+
     /** @brief %Check for dangerous sign conversion */
     void checkSignConversion();
 
@@ -81,6 +85,7 @@ private:
     void tooBigBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
     void tooBigSignedBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
     void integerOverflowError(const Token *tok, const ValueFlow::Value &value);
+    void integerOverflowOptimisationError(const Token *tok, const std::string &replace);
     void signConversionError(const Token *tok, const ValueFlow::Value *negativeValue, const bool constvalue);
     void longCastAssignError(const Token *tok);
     void longCastReturnError(const Token *tok);
@@ -108,6 +113,7 @@ private:
         return "Type checks\n"
                "- bitwise shift by too many bits (only enabled when --platform is used)\n"
                "- signed integer overflow (only enabled when --platform is used)\n"
+               "- expression that can be optimised 'out' because signed integer overflow is undefined behavior.\n"
                "- dangerous sign conversion, when signed value can be negative\n"
                "- possible loss of information when assigning int result to long variable\n"
                "- possible loss of information when returning int result as long return value\n"
diff --git a/test/testtype.cpp b/test/testtype.cpp
index 4a121a09a..87e8e6f12 100644
--- a/test/testtype.cpp
+++ b/test/testtype.cpp
@@ -35,6 +35,7 @@ private:
     void run() OVERRIDE {
         TEST_CASE(checkTooBigShift_Unix32);
         TEST_CASE(checkIntegerOverflow);
+        TEST_CASE(checkIntegerOverflowOptimisations);
         TEST_CASE(signConversion);
         TEST_CASE(longCastAssign);
         TEST_CASE(longCastReturn);
@@ -248,6 +249,35 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void checkIntegerOverflowOptimisations() {
+        Settings settings;
+        settings.addEnabled("warning");
+
+        check("int f(int x) { return x + 10 > x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10>x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x + 10 >= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10>=x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x + 10 < x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10<x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x + 10 <= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10<=x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x - 10 > x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10>x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x - 10 >= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10>=x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x - 10 < x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10<x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
+
+        check("int f(int x) { return x - 10 <= x; }", &settings);
+        ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10<=x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str());
+    }
+
     void signConversion() {
         check("x = -4 * (unsigned)y;");
         ASSERT_EQUALS("[test.cpp:1]: (warning) Expression '-4' has a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str());