From d97942d3c63377831e1f150531400fcd1c476863 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 11 Apr 2022 00:23:44 -0500 Subject: [PATCH] Fix 6577: Detect pointer to uninitialised memory with clock_settime() (#3993) * Fix 6577: Detect pointer to uninitialised memory with clock_settime() * Format --- lib/astutils.cpp | 29 ++++++++++++++++------------- lib/checkuninitvar.cpp | 12 ++++++++++-- lib/valueflow.cpp | 4 ++++ test/cfg/posix.c | 2 +- test/testuninitvar.cpp | 6 ++++++ 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 502cc3c49..6462afa4e 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2129,7 +2129,8 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti const Token * const tok1 = tok; // address of variable - const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&"); + if (tok->astParent() && tok->astParent()->isUnaryOp("&")) + indirect++; int argnr; tok = getTokenArgumentFunction(tok, argnr); @@ -2148,25 +2149,27 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti const bool possiblyPassedByReference = (parenTok->next() == tok1 || Token::Match(tok1->previous(), ", %name% [,)}]")); if (!tok->function() && !tok->variable() && Token::Match(tok, "%name%")) { - // Check if direction (in, out, inout) is specified in the library configuration and use that - if (!addressOf && settings) { + if (settings) { + const bool requireInit = settings->library.isuninitargbad(tok, 1 + argnr); + const bool requireNonNull = settings->library.isnullargbad(tok, 1 + argnr); + // Check if direction (in, out, inout) is specified in the library configuration and use that const Library::ArgumentChecks::Direction argDirection = settings->library.getArgDirection(tok, 1 + argnr); if (argDirection == Library::ArgumentChecks::Direction::DIR_IN) return false; else if (argDirection == Library::ArgumentChecks::Direction::DIR_OUT || argDirection == Library::ArgumentChecks::Direction::DIR_INOUT) { - // With out or inout the direction of the content is specified, not a pointer itself, so ignore pointers for now - const ValueType * const valueType = tok1->valueType(); - if ((valueType && valueType->pointer == indirect) || (indirect == 0 && isArray(tok1))) { + if (indirect == 0 && isArray(tok1)) + return true; + // Assume that if the variable must be initialized then the indirection is 1 + if (indirect > 0 && requireInit && requireNonNull) return true; - } } - } - // if the library says 0 is invalid - // => it is assumed that parameter is an in parameter (TODO: this is a bad heuristic) - if (!addressOf && settings && settings->library.isnullargbad(tok, 1+argnr)) - return false; + // if the library says 0 is invalid + // => it is assumed that parameter is an in parameter (TODO: this is a bad heuristic) + if (indirect == 0 && requireNonNull) + return false; + } // possible pass-by-reference => inconclusive if (possiblyPassedByReference) { if (inconclusive != nullptr) @@ -2183,7 +2186,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti if (!arg) continue; conclusive = true; - if (addressOf || indirect > 0) { + if (indirect > 0) { if (!arg->isConst() && arg->isPointer()) return true; // If const is applied to the pointer, then the value can still be modified diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 4c97c388e..984482105 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1539,7 +1539,7 @@ void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string "$symbol:" + membername + "\nUninitialized struct member: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal); } -enum class ExprUsage { None, PassedByReference, Used }; +enum class ExprUsage { None, NotUsed, PassedByReference, Used }; static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings) { @@ -1571,6 +1571,12 @@ static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings static ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings) { + if (indirect > 0 && tok->astParent()) { + if (Token::Match(tok->astParent(), "%assign%") && astIsRhs(tok)) + return ExprUsage::NotUsed; + if (tok->astParent()->isCast()) + return ExprUsage::NotUsed; + } if (indirect == 0 && Token::Match(tok->astParent(), "%cop%|%assign%") && tok->astParent()->str() != "=") return ExprUsage::Used; return getFunctionUsage(tok, indirect, settings); @@ -1603,7 +1609,7 @@ void CheckUninitVar::valueFlowUninit() } if (ids.count(tok->exprId()) > 0) continue; - if (!tok->variable() && !tok->isUnaryOp("*")) + if (!tok->variable() && !tok->isUnaryOp("*") && !tok->isUnaryOp("&")) continue; if (Token::Match(tok, "%name% (")) continue; @@ -1642,6 +1648,8 @@ void CheckUninitVar::valueFlowUninit() continue; } ExprUsage usage = getExprUsage(tok, v->indirect, mSettings); + if (usage == ExprUsage::NotUsed) + continue; if (!v->subexpressions.empty() && usage == ExprUsage::PassedByReference) continue; if (usage != ExprUsage::Used) { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 315802109..1bfdad0a4 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -576,6 +576,10 @@ static void setTokenValue(Token* tok, ValueFlow::Value value, const Settings* se if (value.isUninitValue()) { if (Token::Match(tok, ". %var%")) setTokenValue(tok->next(), value, settings); + if (parent->isCast()) { + setTokenValue(parent, value, settings); + return; + } ValueFlow::Value pvalue = value; if (!value.subexpressions.empty() && Token::Match(parent, ". %var%")) { if (contains(value.subexpressions, parent->next()->str())) diff --git a/test/cfg/posix.c b/test/cfg/posix.c index a19d0186e..59090d3f8 100644 --- a/test/cfg/posix.c +++ b/test/cfg/posix.c @@ -593,7 +593,7 @@ void timet_h(struct timespec* ptp1) clock_settime(clk_id2, ptp1); struct timespec tp; - // TODO cppcheck-suppress uninitvar + // cppcheck-suppress uninitvar clock_settime(CLOCK_REALTIME, &tp); // #6577 - false negative // cppcheck-suppress uninitvar clock_settime(clk_id3, &tp); diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 1ef2c72e9..4d1f3d9ee 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -5241,6 +5241,12 @@ private: " return u.u16;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f() {\n" + " char src, dest;\n" + " std::memcpy(&dest, &src, 1);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Uninitialized variable: &src\n", errout.str()); } void valueFlowUninitBreak() { // Do not show duplicate warnings about the same uninitialized value