From bd048085bd061dbf88850eb09ad6783a53bbb319 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Mon, 11 Mar 2019 19:20:06 +0100
Subject: [PATCH] Add CheckBufferOverrun::arrayIndexThenCheck

---
 lib/checkbufferoverrun.cpp | 57 ++++++++++++++++++++++++++++++++++++++
 lib/checkbufferoverrun.h   |  8 +++++-
 test/testbufferoverrun.cpp |  2 +-
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp
index 5f496b3c6..5a48e65a2 100644
--- a/lib/checkbufferoverrun.cpp
+++ b/lib/checkbufferoverrun.cpp
@@ -278,6 +278,7 @@ void CheckBufferOverrun::negativeIndexError(const Token *tok, const Variable *va
                 negativeValue->isInconclusive());
 }
 
+//---------------------------------------------------------------------------
 
 size_t CheckBufferOverrun::getBufferSize(const Token *bufTok) const
 {
@@ -318,6 +319,7 @@ size_t CheckBufferOverrun::getBufferSize(const Token *bufTok) const
     // TODO: For pointers get pointer value..
     return 0;
 }
+//---------------------------------------------------------------------------
 
 static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::MinSize &minsize, const std::vector<const Token *> &args, const MathLib::bigint bufferSize, const Settings *settings)
 {
@@ -398,3 +400,58 @@ void CheckBufferOverrun::bufferOverflowError(const Token *tok)
 {
     reportError(tok, Severity::error, "bufferAccessOutOfBounds", "Buffer is accessed out of bounds: " + (tok ? tok->expressionString() : "buf"), CWE788, false);
 }
+
+//---------------------------------------------------------------------------
+
+void CheckBufferOverrun::arrayIndexThenCheck()
+{
+    if (!mSettings->isEnabled(Settings::PORTABILITY))
+        return;
+
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope * const scope : symbolDatabase->functionScopes) {
+        for (const Token *tok = scope->bodyStart; tok && tok != scope->bodyEnd; tok = tok->next()) {
+            if (Token::simpleMatch(tok, "sizeof (")) {
+                tok = tok->linkAt(1);
+                continue;
+            }
+
+            if (Token::Match(tok, "%name% [ %var% ]")) {
+                tok = tok->next();
+
+                const unsigned int indexID = tok->next()->varId();
+                const std::string& indexName(tok->strAt(1));
+
+                // Iterate AST upwards
+                const Token* tok2 = tok;
+                const Token* tok3 = tok2;
+                while (tok2->astParent() && tok2->tokType() != Token::eLogicalOp) {
+                    tok3 = tok2;
+                    tok2 = tok2->astParent();
+                }
+
+                // Ensure that we ended at a logical operator and that we came from its left side
+                if (tok2->tokType() != Token::eLogicalOp || tok2->astOperand1() != tok3)
+                    continue;
+
+                // check if array index is ok
+                // statement can be closed in parentheses, so "(| " is using
+                if (Token::Match(tok2, "&& (| %varid% <|<=", indexID))
+                    arrayIndexThenCheckError(tok, indexName);
+                else if (Token::Match(tok2, "&& (| %any% >|>= %varid% !!+", indexID))
+                    arrayIndexThenCheckError(tok, indexName);
+            }
+        }
+    }
+}
+
+void CheckBufferOverrun::arrayIndexThenCheckError(const Token *tok, const std::string &indexName)
+{
+    reportError(tok, Severity::style, "arrayIndexThenCheck",
+                "$symbol:" + indexName + "\n"
+                "Array index '$symbol' is used before limits check.\n"
+                "Defensive programming: The variable '$symbol' is used as an array index before it "
+                "is checked that is within limits. This can mean that the array might be accessed out of bounds. "
+                "Reorder conditions such as '(a[i] && i < 10)' to '(i < 10 && a[i])'. That way the array will "
+                "not be accessed if the index is out of limits.", CWE398, false);
+}
diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h
index 401237d3d..e897180eb 100644
--- a/lib/checkbufferoverrun.h
+++ b/lib/checkbufferoverrun.h
@@ -77,6 +77,7 @@ public:
         CheckBufferOverrun checkBufferOverrun(tokenizer, settings, errorLogger);
         checkBufferOverrun.arrayIndex();
         checkBufferOverrun.bufferOverflow();
+        checkBufferOverrun.arrayIndexThenCheck();
     }
 
     void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) OVERRIDE {
@@ -89,6 +90,7 @@ public:
         CheckBufferOverrun c(nullptr, settings, errorLogger);
         c.arrayIndexError(nullptr, nullptr, nullptr);
         c.negativeIndexError(nullptr, nullptr, nullptr);
+        c.arrayIndexThenCheckError(nullptr, "i");
     }
 
 private:
@@ -100,6 +102,9 @@ private:
     void bufferOverflow();
     void bufferOverflowError(const Token *tok);
 
+    void arrayIndexThenCheck();
+    void arrayIndexThenCheckError(const Token *tok, const std::string &indexName);
+
     size_t getBufferSize(const Token *bufTok) const;
 
     static std::string myName() {
@@ -110,7 +115,8 @@ private:
         return "Out of bounds checking:\n"
                "- Array index out of bounds\n"
                "- Buffer overflow\n"
-               "- Dangerous usage of strncat()\n";
+               "- Dangerous usage of strncat()\n"
+               "- Using array index before checking it\n";
     }
 };
 /// @}
diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp
index 56d2e9a4e..32deb0275 100644
--- a/test/testbufferoverrun.cpp
+++ b/test/testbufferoverrun.cpp
@@ -239,7 +239,7 @@ private:
         TEST_CASE(getErrorMessages);
 
         // Access array and then check if the used index is within bounds
-        // TODO TEST_CASE(arrayIndexThenCheck);
+        TEST_CASE(arrayIndexThenCheck);
 
         // TODO TEST_CASE(bufferNotZeroTerminated);