Fixed #2956 (False negative: read array and then immediately check the index 'str[i] && i<sizeof(str)')

lib/checkbufferoverrun.cpp

@@ -2179,3 +2179,35 @@ void CheckBufferOverrun::executionPaths()
 
 
 
+
+void CheckBufferOverrun::arrayIndexThenCheck()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
+    {
+        if (Token::Match(tok, "%var% [ %var% ]"))
+        {
+            const std::string arrayName(tok->str());
+            const std::string indexName(tok->strAt(2));
+
+            // skip array index..
+            tok = tok->tokAt(4);
+            while (tok->str() == "[")
+                tok = tok->link()->next();
+
+            // skip comparison
+            if (Token::Match(tok, "==|!=|<|<=|>|>= %any% &&"))
+                tok = tok->tokAt(2);
+
+            // check if array index is ok
+            if (Token::Match(tok, ("&& " + indexName + " <|<=").c_str()))
+                arrayIndexThenCheckError(tok, indexName);
+        }
+    }
+}
+
+void CheckBufferOverrun::arrayIndexThenCheckError(const Token *tok, const std::string &indexName)
+{
+    reportError(tok, Severity::style, "arrayIndexThenCheck", "array index " + indexName + " is used before bounds check");
+}

lib/checkbufferoverrun.h

@@ -64,6 +64,7 @@ public:
         CheckBufferOverrun checkBufferOverrun(tokenizer, settings, errorLogger);
         checkBufferOverrun.bufferOverrun();
         checkBufferOverrun.negativeIndex();
+        checkBufferOverrun.arrayIndexThenCheck();
 
         /** ExecutionPath checking.. */
         checkBufferOverrun.executionPaths();
@@ -72,6 +73,9 @@ public:
     /** @brief %Check for buffer overruns */
     void bufferOverrun();
 
+    /** @brief Using array index before bounds check */
+    void arrayIndexThenCheck();
+
     /** @brief %Check for buffer overruns by inspecting execution paths */
     void executionPaths();
 
@@ -216,6 +220,7 @@ public:
     void negativeIndexError(const Token *tok, MathLib::bigint index);
     void cmdLineArgsError(const Token *tok);
     void pointerOutOfBounds(const Token *tok, const std::string &object);	// UB when result of calculation is out of bounds
+    void arrayIndexThenCheckError(const Token *tok, const std::string &indexName);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
     {
@@ -229,6 +234,7 @@ public:
         c.negativeIndexError(0, -1);
         c.cmdLineArgsError(0);
         c.pointerOutOfBounds(0, "array");
+        c.arrayIndexThenCheckError(0, "index");
     }
 
     std::string myName() const

test/testbufferoverrun.cpp

@@ -60,6 +60,7 @@ private:
         CheckBufferOverrun checkBufferOverrun(&tokenizer, &settings, this);
         checkBufferOverrun.bufferOverrun();
         checkBufferOverrun.negativeIndex();
+        checkBufferOverrun.arrayIndexThenCheck();
     }
 
     void run()
@@ -217,6 +218,9 @@ private:
         TEST_CASE(getErrorMessages);
 
         TEST_CASE(unknownMacroNoDecl);    // #2638 - not variable declaration: 'AAA a[0] = 0;'
+
+        // Access array and then check if the used index is within bounds
+        TEST_CASE(arrayIndexThenCheck);
     }
 
 
@@ -3015,6 +3019,21 @@ private:
               "}");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void arrayIndexThenCheck()
+    {
+        check("void f(const char s[]) {\n"
+              "    if (s[i] == 'x' && i < y) {\n"
+              "    }"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) array index i is used before bounds check\n", errout.str());
+
+        check("void f(const char s[]) {\n"
+              "    for (i = 0; s[i] == 'x' && i < y; ++i) {\n"
+              "    }"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) array index i is used before bounds check\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestBufferOverrun)