BufferOverrun: Use ValueFlow string values more

lib/checkbufferoverrun.cpp

@@ -1306,8 +1306,33 @@ void CheckBufferOverrun::bufferOverrun2()
 {
     // singlepass checking using ast, symboldatabase and valueflow
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
-        if (Token::Match(tok, "%var% [")) {
+        // Array index
-            const Variable *var = tok->variable();
+        if (!Token::Match(tok, "%var% ["))
+            continue;
+
+        // TODO: what to do about negative index..
+        const Token *index = tok->next()->astOperand2();
+        if (index && index->getValueLE(-1LL,_settings))
+            continue;
+
+        // Set full varname..
+        std::string varname(tok->str());
+        if (tok->astParent() && tok->astParent()->str() == ".") {
+            const Token *parent = tok->astParent();
+            while (parent->astParent() && parent->astParent()->str() == ".")
+                parent = parent->astParent();
+            varname = parent->expressionString();
+        }
+
+
+        const Token * const strtoken = tok->getValueTokenMinStrSize();
+        if (strtoken) {
+            ArrayInfo arrayInfo(tok->varId(), varname, 1U, Token::getStrSize(strtoken));
+            valueFlowCheckArrayIndex(tok->next(), arrayInfo);
+        }
+
+        else {
+            const Variable * const var = tok->variable();
             if (!var || var->nameToken() == tok || !var->isArray())
                 continue;
 
@@ -1315,20 +1340,8 @@ void CheckBufferOverrun::bufferOverrun2()
             if (var->dimension(0) <= 1 && Token::simpleMatch(var->nameToken()->linkAt(1),"] ; }"))
                 continue;
 
-            // TODO: what to do about negative index..
-            const Token *index = tok->next()->astOperand2();
-            if (index && index->getValueLE(-1LL,_settings))
-                continue;
-
             ArrayInfo arrayInfo(var,_tokenizer);
-
+            arrayInfo.varname(varname);
-            // Set full varname..
-            if (tok->astParent() && tok->astParent()->str() == ".") {
-                const Token *parent = tok->astParent();
-                while (parent->astParent() && parent->astParent()->str() == ".")
-                    parent = parent->astParent();
-                arrayInfo.varname(parent->expressionString());
-            }
 
             valueFlowCheckArrayIndex(tok->next(), arrayInfo);
         }
@@ -1540,12 +1553,15 @@ void CheckBufferOverrun::checkStringArgument()
 
             unsigned int argnr = 1;
             for (const Token *argtok = tok->tokAt(2); argtok; argtok = argtok->nextArgument(), argnr++) {
-                if (!Token::Match(argtok, "%str% ,|)"))
+                if (!Token::Match(argtok, "%var%|%str% ,|)"))
+                    continue;
+                const Token *strtoken = argtok->getValueTokenMinStrSize();
+                if (!strtoken)
                     continue;
                 const std::list<Library::ArgumentChecks::MinSize> *minsizes = _settings->library.argminsizes(tok->str(), argnr);
                 if (!minsizes)
                     continue;
-                if (checkMinSizes(*minsizes, tok, Token::getStrSize(argtok), nullptr))
+                if (checkMinSizes(*minsizes, tok, Token::getStrSize(strtoken), nullptr))
                     bufferOverrunError(argtok);
             }
         }

lib/token.cpp

@@ -1283,6 +1283,40 @@ const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Sett
     return ret;
 }
 
+const Token *Token::getValueTokenMinStrSize() const
+{
+    const Token *ret = nullptr;
+    std::size_t minsize = ~0U;
+    std::list<ValueFlow::Value>::const_iterator it;
+    for (it = values.begin(); it != values.end(); ++it) {
+        if (it->tokvalue && it->tokvalue->type() == Token::eString) {
+            std::size_t size = getStrSize(it->tokvalue);
+            if (!ret || size < minsize) {
+                minsize = size;
+                ret = it->tokvalue;
+            }
+        }
+    }
+    return ret;
+}
+
+const Token *Token::getValueTokenMaxStrLength() const
+{
+    const Token *ret = nullptr;
+    std::size_t maxlength = 0U;
+    std::list<ValueFlow::Value>::const_iterator it;
+    for (it = values.begin(); it != values.end(); ++it) {
+        if (it->tokvalue && it->tokvalue->type() == Token::eString) {
+            std::size_t length = getStrLength(it->tokvalue);
+            if (!ret || length > maxlength) {
+                maxlength = length;
+                ret = it->tokvalue;
+            }
+        }
+    }
+    return ret;
+}
+
 void Token::assignProgressValues(Token *tok)
 {
     unsigned int total_count = 0;

lib/token.h

@@ -670,21 +670,8 @@ public:
     const ValueFlow::Value * getValueLE(const MathLib::bigint val, const Settings *settings) const;
     const ValueFlow::Value * getValueGE(const MathLib::bigint val, const Settings *settings) const;
 
-    const Token *getValueTokenMaxStrLength() const {
+    const Token *getValueTokenMaxStrLength() const;
-        const Token *ret = nullptr;
+    const Token *getValueTokenMinStrSize() const;
-        std::size_t maxlength = 0U;
-        std::list<ValueFlow::Value>::const_iterator it;
-        for (it = values.begin(); it != values.end(); ++it) {
-            if (it->tokvalue && it->tokvalue->type() == Token::eString) {
-                std::size_t length = getStrLength(it->tokvalue);
-                if (!ret || length > maxlength) {
-                    maxlength = length;
-                    ret = it->tokvalue;
-                }
-            }
-        }
-        return ret;
-    }
 
 private:
 

test/testbufferoverrun.cpp

@@ -224,6 +224,8 @@ private:
         TEST_CASE(buffer_overrun_function_array_argument);
         TEST_CASE(possible_buffer_overrun_1); // #3035
 
+        TEST_CASE(valueflow_string); // using ValueFlow string values in checking
+
         // It is undefined behaviour to point out of bounds of an array
         // the address beyond the last element is in bounds
         // char a[10];
@@ -2893,6 +2895,30 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void valueflow_string() { // using ValueFlow string values in checking
+        checkstd("void f() {\n"
+                 "  char buf[3];\n"
+                 "  const char *x = s;\n"
+                 "  if (cond) x = \"abcde\";\n"
+                 "  strcpy(buf,x);\n" // <- buffer overflow when x is "abcde"
+                 "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: buf\n", errout.str());
+
+        checkstd("void f() {\n"
+                 "  const char *x = s;\n"
+                 "  if (cond) x = \"abcde\";\n"
+                 "  memcpy(buf,x,20);\n" // <- buffer overflow when x is "abcde"
+                 "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str());
+
+        check("char f() {\n"
+              "  const char *x = s;\n"
+              "  if (cond) x = \"abcde\";\n"
+              "  return x[20];\n" // <- array index out of bounds when x is "abcde"
+              "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Array 'x[6]' accessed at index 20, which is out of bounds.\n", errout.str());
+    }
+
     void pointer_out_of_bounds_1() {
         check("void f() {\n"
               "    char a[10];\n"