From db57efa486da3b44b49ab07bd9456bcd9fc4ef59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 22 Aug 2021 16:37:41 +0200 Subject: [PATCH] CheckBufferOverrun: Reimplement CheckBufferOverrun::argumentSize check --- lib/checkbufferoverrun.cpp | 54 ++++++++++++++++++++++++++++++++++++++ lib/checkbufferoverrun.h | 8 +++++- test/testbufferoverrun.cpp | 13 +++------ 3 files changed, 64 insertions(+), 11 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index c6e818feb..9c1f751aa 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -52,6 +52,7 @@ namespace { // CWE ids used: static const CWE CWE131(131U); // Incorrect Calculation of Buffer Size static const CWE CWE170(170U); // Improper Null Termination +static const CWE CWE_ARGUMENT_SIZE(398U); // Indicator of Poor Code Quality static const CWE CWE_ARRAY_INDEX_THEN_CHECK(398U); // Indicator of Poor Code Quality static const CWE CWE682(682U); // Incorrect Calculation static const CWE CWE758(758U); // Reliance on Undefined, Unspecified, or Implementation-Defined Behavior @@ -761,8 +762,61 @@ void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::stri "zero at the end of the buffer. This causes bugs later in the code if the code " "assumes buffer is null-terminated.", CWE170, Certainty::inconclusive); } +//--------------------------------------------------------------------------- +void CheckBufferOverrun::argumentSize() +{ + // Check '%type% x[10]' arguments + if (!mSettings->severity.isEnabled(Severity::warning)) + return; + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope * const scope : symbolDatabase->functionScopes) { + for (const Token *tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { + if (!tok->function() || !Token::Match(tok, "%name% (")) + continue; + + // If argument is '%type% a[num]' then check bounds against num + const Function *callfunc = tok->function(); + const std::vector callargs = getArguments(tok); + for (nonneg int paramIndex = 0; paramIndex < callargs.size() && paramIndex < callfunc->argCount(); ++paramIndex) { + const Variable* const argument = callfunc->getArgumentVar(paramIndex); + if (!argument || !argument->nameToken() || !argument->isArray()) + continue; + if (!argument->valueType() || !callargs[paramIndex]->valueType()) + continue; + if (argument->valueType()->type != callargs[paramIndex]->valueType()->type) + continue; + const Token * calldata = callargs[paramIndex]; + while (Token::Match(calldata, "::|.")) + calldata = calldata->astOperand2(); + if (!calldata->variable() || !calldata->variable()->isArray()) + continue; + if (calldata->variable()->dimensions().size() != argument->dimensions().size()) + continue; + bool err = false; + for (int d = 0; d < argument->dimensions().size(); ++d) { + const auto& dim1 = calldata->variable()->dimensions()[d]; + const auto& dim2 = argument->dimensions()[d]; + if (!dim1.known || !dim2.known) + break; + if (dim1.num < dim2.num) + err = true; + } + if (err) + argumentSizeError(tok, tok->str(), argument->name()); + } + } + } +} + +void CheckBufferOverrun::argumentSizeError(const Token *tok, const std::string &functionName, const std::string &varname) +{ + reportError(tok, Severity::warning, "argumentSize", + "$symbol:" + functionName + '\n' + + "$symbol:" + varname + '\n' + + "The array '" + varname + "' is too small, the function '" + functionName + "' expects a bigger one.", CWE_ARGUMENT_SIZE, Certainty::normal); +} //--------------------------------------------------------------------------- // CTU.. diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 4d0f67ddc..875679dd5 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -74,6 +74,7 @@ public: checkBufferOverrun.arrayIndexThenCheck(); checkBufferOverrun.stringNotZeroTerminated(); checkBufferOverrun.objectIndex(); + checkBufferOverrun.argumentSize(); } void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { @@ -84,6 +85,7 @@ public: c.arrayIndexThenCheckError(nullptr, "i"); c.bufferOverflowError(nullptr, nullptr, Certainty::normal); c.objectIndexError(nullptr, nullptr, true); + c.argumentSizeError(nullptr, "function", "buffer"); } /** @brief Parse current TU and extract file info */ @@ -114,6 +116,9 @@ private: void stringNotZeroTerminated(); void terminateStrncpyError(const Token *tok, const std::string &varname); + void argumentSize(); + void argumentSizeError(const Token *tok, const std::string &functionName, const std::string &varname); + void objectIndex(); void objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known); @@ -153,7 +158,8 @@ private: "- Buffer overflow\n" "- Dangerous usage of strncat()\n" "- Using array index before checking it\n" - "- Partial string write that leads to buffer that is not zero terminated.\n"; + "- Partial string write that leads to buffer that is not zero terminated.\n" + "- Check for large enough arrays being passed to functions\n"; } }; /// @} diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 28491c135..564ec207b 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2855,28 +2855,21 @@ private: " char a[2];\n" " f(a);\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", errout.str()); check("void f(float a[10][3]);\n" "void g() {\n" " float a[2][3];\n" " f(a);\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", errout.str()); check("void f(int a[20]);\n" "void g() {\n" " int a[2];\n" " f(a);\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", "", errout.str()); - - check("void f(int a[20]);\n" - "void g() {\n" - " int a[5];\n" - " f(a);\n" - "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) The array 'a' is too small, the function 'f' expects a bigger one.\n", errout.str()); check("void f(int a[]) {\n" " switch (2) {\n"