From 7841430793ace94689cad4612388196bed3332e1 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 13 Nov 2019 05:46:54 -0600 Subject: [PATCH] Fix issue 9428: FP uninitvar for pointer passed to sscanf (#2344) * Add indirect to library cfg files * Check indirect for non null arguments * Reenable subfunction analysis * Use indirect 1 when using not-null * Parse correct string name * Update documentation * Make attribute optional --- cfg/cppcheck-cfg.rng | 13 ++++++++++++- lib/checkuninitvar.cpp | 12 ++++++------ lib/checkuninitvar.h | 4 ++-- lib/library.cpp | 12 +++++++++--- lib/library.h | 6 +++--- lib/valueflow.cpp | 3 --- man/reference-cfg-format.md | 4 ++++ test/testlibrary.cpp | 6 +++--- test/testuninitvar.cpp | 9 ++++++++- 9 files changed, 47 insertions(+), 22 deletions(-) diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng index fd269c63e..983a4abfe 100644 --- a/cfg/cppcheck-cfg.rng +++ b/cfg/cppcheck-cfg.rng @@ -220,7 +220,11 @@ - + + + + + @@ -504,6 +508,13 @@ 20 + + + + 0 + 2 + + diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index ea491764d..d17a66beb 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -946,7 +946,7 @@ void CheckUninitVar::checkRhs(const Token *tok, const Variable &var, Alloc alloc } } -bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc alloc) const +bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const { if (!pointer && Token::Match(vartok, "%name% (")) return false; @@ -999,7 +999,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc al if (Token::Match(possibleParent, "[(,]")) { if (unknown) return false; // TODO: output some info message? - const int use = isFunctionParUsage(vartok, pointer, alloc); + const int use = isFunctionParUsage(vartok, pointer, alloc, indirect); if (use >= 0) return (use>0); } @@ -1127,7 +1127,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc al * is passed "by reference" then it is not necessarily "used". * @return -1 => unknown 0 => not used 1 => used */ -int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc) const +int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const { bool unknown = false; const Token *parent = getAstParentSkipPossibleCastAndAddressOf(vartok, &unknown); @@ -1183,9 +1183,9 @@ int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc return alloc == NO_ALLOC; } else { const bool isnullbad = mSettings->library.isnullargbad(start->previous(), argumentNumber + 1); - if (pointer && !address && isnullbad && alloc == NO_ALLOC) + if (indirect == 0 && pointer && !address && isnullbad && alloc == NO_ALLOC) return 1; - const bool isuninitbad = mSettings->library.isuninitargbad(start->previous(), argumentNumber + 1); + const bool isuninitbad = mSettings->library.isuninitargbad(start->previous(), argumentNumber + 1, indirect); if (alloc != NO_ALLOC) return isnullbad && isuninitbad; return isuninitbad && (!address || isnullbad); @@ -1366,7 +1366,7 @@ void CheckUninitVar::valueFlowUninit() continue; bool uninitderef = false; if (tok->variable()) { - if (!isVariableUsage(tok, tok->variable()->isPointer(), tok->variable()->isArray() ? ARRAY : NO_ALLOC)) + if (!isVariableUsage(tok, tok->variable()->isPointer(), tok->variable()->isArray() ? ARRAY : NO_ALLOC, v->indirect)) continue; bool unknown; const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, mSettings); diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index 659554e21..cefd07500 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -76,8 +76,8 @@ public: bool checkIfForWhileHead(const Token *startparentheses, const Variable& var, bool suppressErrors, bool isuninit, Alloc alloc, const std::string &membervar); bool checkLoopBody(const Token *tok, const Variable& var, const Alloc alloc, const std::string &membervar, const bool suppressErrors); void checkRhs(const Token *tok, const Variable &var, Alloc alloc, nonneg int number_of_if, const std::string &membervar); - bool isVariableUsage(const Token *vartok, bool pointer, Alloc alloc) const; - int isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc) const; + bool isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect = 0) const; + int isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect = 0) const; bool isMemberVariableAssignment(const Token *tok, const std::string &membervar) const; bool isMemberVariableUsage(const Token *tok, bool isPointer, Alloc alloc, const std::string &membervar) const; diff --git a/lib/library.cpp b/lib/library.cpp index eaa5537cc..0db43a461 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -663,12 +663,16 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co } for (const tinyxml2::XMLElement *argnode = functionnode->FirstChildElement(); argnode; argnode = argnode->NextSiblingElement()) { const std::string argnodename = argnode->Name(); + int indirect = 0; + const char * const indirectStr = node->Attribute("indirect"); + if (indirectStr) + indirect = atoi(indirectStr); if (argnodename == "not-bool") ac.notbool = true; else if (argnodename == "not-null") ac.notnull = true; else if (argnodename == "not-uninit") - ac.notuninit = true; + ac.notuninit = indirect; else if (argnodename == "formatstr") ac.formatstr = true; else if (argnodename == "strz") @@ -775,6 +779,8 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co else unknown_elements.insert(argnodename); } + if (ac.notuninit == 0) + ac.notuninit = ac.notnull ? 1 : 0; } else if (functionnodename == "ignorefunction") { func.ignore = true; } else if (functionnodename == "formatstr") { @@ -951,7 +957,7 @@ bool Library::isnullargbad(const Token *ftok, int argnr) const return arg && arg->notnull; } -bool Library::isuninitargbad(const Token *ftok, int argnr) const +bool Library::isuninitargbad(const Token *ftok, int argnr, int indirect) const { const ArgumentChecks *arg = getarg(ftok, argnr); if (!arg) { @@ -961,7 +967,7 @@ bool Library::isuninitargbad(const Token *ftok, int argnr) const if (it != functions.cend() && it->second.formatstr && !it->second.formatstr_scan) return true; } - return arg && arg->notuninit; + return arg && arg->notuninit >= indirect; } diff --git a/lib/library.h b/lib/library.h index f195ed500..009610153 100644 --- a/lib/library.h +++ b/lib/library.h @@ -242,7 +242,7 @@ public: ArgumentChecks() : notbool(false), notnull(false), - notuninit(false), + notuninit(-1), formatstr(false), strz(false), optional(false), @@ -253,7 +253,7 @@ public: bool notbool; bool notnull; - bool notuninit; + int notuninit; bool formatstr; bool strz; bool optional; @@ -318,7 +318,7 @@ public: } bool isnullargbad(const Token *ftok, int argnr) const; - bool isuninitargbad(const Token *ftok, int argnr) const; + bool isuninitargbad(const Token *ftok, int argnr, int indirect = 0) const; bool isargformatstr(const Token *ftok, int argnr) const { const ArgumentChecks *arg = getarg(ftok, argnr); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 437a56158..b79620aa2 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5140,9 +5140,6 @@ static void valueFlowSubFunction(TokenList* tokenlist, ErrorLogger* errorLogger, // Don't forward lifetime values argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); - // Don't forward values, this is handled by CTU. We also had a FP #9347 - argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isUninitValue)); - if (argvalues.empty()) continue; diff --git a/man/reference-cfg-format.md b/man/reference-cfg-format.md index 8e24e9497..3bfc1c0a9 100644 --- a/man/reference-cfg-format.md +++ b/man/reference-cfg-format.md @@ -172,6 +172,10 @@ Here is the minimal windows.cfg: +The `indirect` attribute can be set to control the indirection of uninitialized memory allowed. Setting `indirect` to `0` means no uninitialized memory is allowed. Setting it to `1` allows a pointer to uninitialized memory. Setting it to `2` allows a pointer to pointer to uninitialized memory. + +By default, cppcheck will use an indirect value of `0` unless `not-null` is used. When `not-null` is used, then `indirect` will default to `1`. + ### Null pointers Cppcheck assumes it's ok to pass NULL pointers to functions. Here is an example program: diff --git a/test/testlibrary.cpp b/test/testlibrary.cpp index 89ef70646..64f41030f 100644 --- a/test/testlibrary.cpp +++ b/test/testlibrary.cpp @@ -229,7 +229,7 @@ private: Library library; ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); - ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[1].notuninit); + ASSERT_EQUALS(0, library.functions["foo"].argumentChecks[1].notuninit); ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[2].notnull); ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[3].formatstr); ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[4].strz); @@ -248,7 +248,7 @@ private: Library library; ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); - ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[-1].notuninit); + ASSERT_EQUALS(0, library.functions["foo"].argumentChecks[-1].notuninit); } void function_arg_variadic() const { @@ -262,7 +262,7 @@ private: Library library; ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); - ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[-1].notuninit); + ASSERT_EQUALS(0, library.functions["foo"].argumentChecks[-1].notuninit); TokenList tokenList(nullptr); std::istringstream istr("foo(a,b,c,d,e);"); diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index cc9430f9d..8f6ed3a67 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4285,6 +4285,13 @@ private: "}"); ASSERT_EQUALS("[test.cpp:8]: (error) Uninitialized variable: a\n", errout.str()); + valueFlowUninit("void h() {\n" + " int i;\n" + " int* v = &i;\n" + " sscanf(\"0\", \"%d\", v);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + valueFlowUninit("void test(int p) {\n" " int f;\n" " if (p > 0)\n" @@ -4307,7 +4314,7 @@ private: " someType_t gVar;\n" " bar(&gVar);\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:5]: (error) Uninitialized variable: p->flags\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:5]: (error) Uninitialized variable: flags\n", errout.str()); valueFlowUninit("typedef struct \n" "{\n"