Fixed #2866 (Detect sign extension bugs)

lib/checkother.cpp

@@ -2761,15 +2761,22 @@ void CheckOther::checkCharVariable()
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
     {
         // Declaring the variable..
-        if (Token::Match(tok, "[{};(,] char %var% [;=,)]"))
+        if (Token::Match(tok, "[{};(,] const| char *| %var% [;=,)]") ||
+            Token::Match(tok, "[{};(,] const| char %var% ["))
         {
+            // goto 'char' token
+            tok = tok->next();
+            if (tok->str() == "const")
+                tok = tok->next();
+
             // Check for unsigned char
-            if (tok->tokAt(1)->isUnsigned())
+            if (tok->isUnsigned())
                 continue;
 
             // Set tok to point to the variable name
-            tok = tok->tokAt(2);
+            tok = tok->next();
-            if (tok->str() == "char")
+            const bool isPointer(tok->str() == "*" || tok->strAt(1) == "[");
+            if (tok->str() == "*")
                 tok = tok->next();
 
             // Check usage of char variable..
@@ -2786,9 +2793,6 @@ void CheckOther::checkCharVariable()
                         break;
                 }
 
-                else if (tok2->str() == "return")
-                    continue;
-
                 std::string temp = "%var% [ " + tok->str() + " ]";
                 if ((tok2->str() != ".") && Token::Match(tok2->next(), temp.c_str()))
                 {
@@ -2803,7 +2807,7 @@ void CheckOther::checkCharVariable()
                         continue;
 
                     // it's ok with a bitwise and where the other operand is 0xff or less..
-                    if (std::string(tok2->strAt(4)) == "&")
+                    if (tok2->strAt(4) == "&")
                     {
                         if (tok2->tokAt(3)->isNumber() && MathLib::isGreater("0x100", tok2->strAt(3)))
                             continue;
@@ -2819,6 +2823,21 @@ void CheckOther::checkCharVariable()
                     charBitOpError(tok2);
                     break;
                 }
+
+                if (isPointer && Token::Match(tok2, "[;{}] %var% = %any% [&|] ( * %varid% ) ;", tok->varId()))
+                {
+                    // it's ok with a bitwise and where the other operand is 0xff or less..
+                    if (tok2->strAt(4) == "&" && tok2->tokAt(3)->isNumber() && MathLib::isGreater("0x100", tok2->strAt(3)))
+                        continue;
+
+                    // is the result stored in a short|int|long?
+                    if (!Token::findmatch(_tokenizer->tokens(), "short|int|long %varid%", tok2->next()->varId()))
+                        continue;
+
+                    // This is an error..
+                    charBitOpError(tok2);
+                    break;
+                }
             }
         }
     }
@@ -3477,12 +3496,30 @@ void CheckOther::constStatementError(const Token *tok, const std::string &type)
 
 void CheckOther::charArrayIndexError(const Token *tok)
 {
-    reportError(tok, Severity::warning, "charArrayIndex", "Warning - using char variable as array index");
+    reportError(tok,
+                Severity::warning,
+                "charArrayIndex",
+                "When using a char variable as array index, sign extension will mean buffer overflow.\n"
+                "When using a char variable as array index, sign extension will cause buffer overflows. For example:\n"
+                "    char c = 0x80;\n"
+                "    char buf[512];\n"
+                "    buf[c] = 13;    // buffer overflow, index must be a value between 0 and 511.\n"
+                "    printf(\"%i\", buf[0x80]);\n"
+                "There is a big chance that the value that is printed won't be 13.");
 }
 
 void CheckOther::charBitOpError(const Token *tok)
 {
-    reportError(tok, Severity::warning, "charBitOp", "Warning - using char variable in bit operation");
+    reportError(tok,
+                Severity::warning,
+                "charBitOp",
+                "When using char variables in bit operations, sign extension can generate unexpected results.\n"
+                "When using char variables in bit operations, sign extension can generate unexpected results. For example:\n"
+                "    char c = 0x80;\n"
+                "    int i = 0 | c;\n"
+                "    if (i & 0x8000)\n"
+                "        printf(\"not expected\");\n"
+                "The 'not expected' will be printed on the screen.");
 }
 
 void CheckOther::variableScopeError(const Token *tok, const std::string &varname)

test/testcharvar.cpp

@@ -41,6 +41,7 @@ private:
         TEST_CASE(return1);
         TEST_CASE(assignChar);
         TEST_CASE(and03);
+        TEST_CASE(pointer);
     }
 
     void check(const char code[])
@@ -76,20 +77,20 @@ private:
               "    char ch = 0x80;\n"
               "    buf[ch] = 0;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Warning - using char variable as array index\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (warning) When using a char variable as array index, sign extension will mean buffer overflow.\n", errout.str());
 
         check("void foo()\n"
               "{\n"
               "    signed char ch = 0x80;\n"
               "    buf[ch] = 0;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Warning - using char variable as array index\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (warning) When using a char variable as array index, sign extension will mean buffer overflow.\n", errout.str());
 
         check("void foo(char ch)\n"
               "{\n"
               "    buf[ch] = 0;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Warning - using char variable as array index\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (warning) When using a char variable as array index, sign extension will mean buffer overflow.\n", errout.str());
     }
 
 
@@ -101,7 +102,7 @@ private:
               "    char ch;\n"
               "    result = a | ch;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Warning - using char variable in bit operation\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (warning) When using char variables in bit operations, sign extension can generate unexpected results.\n", errout.str());
     }
 
     void bitop2()
@@ -145,6 +146,15 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void pointer()
+    {
+        check("void f(char *p) {\n"
+              "    int ret = 0;\n"
+              "    ret |= *p;\n"
+              "    return ret;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) When using char variables in bit operations, sign extension can generate unexpected results.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestCharVar)