walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

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
This commit is contained in:
Paul Fultz II 2019-11-13 05:46:54 -06:00 committed by Daniel Marjamäki
parent 83095129d2
commit 7841430793
9 changed files with 47 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
cfg/cppcheck-cfg.rng
View File

@ -220,7 +220,11 @@
<element name="not-null"><empty/></element> <element name="not-null"><empty/></element>
</optional> </optional>
<optional> <optional>
<element name="not-uninit"><empty/></element> <element name="not-uninit">
<optional>
<attribute name="indirect"><ref name="INDIRECT"/></attribute>
</optional>
</element>
</optional> </optional>
<optional> <optional>
<element name="valid"> <element name="valid">
@ -505,6 +509,13 @@
</data> </data>
</define> </define>
<define name="INDIRECT">
<data type="integer">
<param name="minInclusive">0</param>
<param name="maxInclusive">2</param>
</data>
</define>
<define name="ARG-DIRECTION"> <define name="ARG-DIRECTION">
<choice> <choice>
<value>in</value> <value>in</value>

12
lib/checkuninitvar.cpp
View File

@ -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% (")) if (!pointer && Token::Match(vartok, "%name% ("))
return false; return false;
@ -999,7 +999,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc al
if (Token::Match(possibleParent, "[(,]")) { if (Token::Match(possibleParent, "[(,]")) {
if (unknown) if (unknown)
return false; // TODO: output some info message? 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) if (use >= 0)
return (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". * is passed "by reference" then it is not necessarily "used".
* @return -1 => unknown 0 => not used 1 => 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; bool unknown = false;
const Token *parent = getAstParentSkipPossibleCastAndAddressOf(vartok, &unknown); const Token *parent = getAstParentSkipPossibleCastAndAddressOf(vartok, &unknown);
@ -1183,9 +1183,9 @@ int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc
return alloc == NO_ALLOC; return alloc == NO_ALLOC;
} else { } else {
const bool isnullbad = mSettings->library.isnullargbad(start->previous(), argumentNumber + 1); 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; 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) if (alloc != NO_ALLOC)
return isnullbad && isuninitbad; return isnullbad && isuninitbad;
return isuninitbad && (!address || isnullbad); return isuninitbad && (!address || isnullbad);
@ -1366,7 +1366,7 @@ void CheckUninitVar::valueFlowUninit()
continue; continue;
bool uninitderef = false; bool uninitderef = false;
if (tok->variable()) { 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; continue;
bool unknown; bool unknown;
const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, mSettings); const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, mSettings);

4
lib/checkuninitvar.h
View File

@ -76,8 +76,8 @@ public:
bool checkIfForWhileHead(const Token *startparentheses, const Variable& var, bool suppressErrors, bool isuninit, Alloc alloc, const std::string &membervar); 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); 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); 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; bool isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect = 0) const;
int isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc) 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 isMemberVariableAssignment(const Token *tok, const std::string &membervar) const;
bool isMemberVariableUsage(const Token *tok, bool isPointer, Alloc alloc, const std::string &membervar) const; bool isMemberVariableUsage(const Token *tok, bool isPointer, Alloc alloc, const std::string &membervar) const;

12
lib/library.cpp
View File

@ -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()) { for (const tinyxml2::XMLElement *argnode = functionnode->FirstChildElement(); argnode; argnode = argnode->NextSiblingElement()) {
const std::string argnodename = argnode->Name(); 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") if (argnodename == "not-bool")
ac.notbool = true; ac.notbool = true;
else if (argnodename == "not-null") else if (argnodename == "not-null")
ac.notnull = true; ac.notnull = true;
else if (argnodename == "not-uninit") else if (argnodename == "not-uninit")
ac.notuninit = true; ac.notuninit = indirect;
else if (argnodename == "formatstr") else if (argnodename == "formatstr")
ac.formatstr = true; ac.formatstr = true;
else if (argnodename == "strz") else if (argnodename == "strz")
@ -775,6 +779,8 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co
else else
unknown_elements.insert(argnodename); unknown_elements.insert(argnodename);
} }
if (ac.notuninit == 0)
ac.notuninit = ac.notnull ? 1 : 0;
} else if (functionnodename == "ignorefunction") { } else if (functionnodename == "ignorefunction") {
func.ignore = true; func.ignore = true;
} else if (functionnodename == "formatstr") { } else if (functionnodename == "formatstr") {
@ -951,7 +957,7 @@ bool Library::isnullargbad(const Token *ftok, int argnr) const
return arg && arg->notnull; 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); const ArgumentChecks *arg = getarg(ftok, argnr);
if (!arg) { 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) if (it != functions.cend() && it->second.formatstr && !it->second.formatstr_scan)
return true; return true;
} }
return arg && arg->notuninit; return arg && arg->notuninit >= indirect;
} }

6
lib/library.h
View File

@ -242,7 +242,7 @@ public:
ArgumentChecks() : ArgumentChecks() :
notbool(false), notbool(false),
notnull(false), notnull(false),
notuninit(false), notuninit(-1),
formatstr(false), formatstr(false),
strz(false), strz(false),
optional(false), optional(false),
@ -253,7 +253,7 @@ public:
bool notbool; bool notbool;
bool notnull; bool notnull;
bool notuninit; int notuninit;
bool formatstr; bool formatstr;
bool strz; bool strz;
bool optional; bool optional;
@ -318,7 +318,7 @@ public:
} }
bool isnullargbad(const Token *ftok, int argnr) const; 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 { bool isargformatstr(const Token *ftok, int argnr) const {
const ArgumentChecks *arg = getarg(ftok, argnr); const ArgumentChecks *arg = getarg(ftok, argnr);

3
lib/valueflow.cpp
View File

@ -5140,9 +5140,6 @@ static void valueFlowSubFunction(TokenList* tokenlist, ErrorLogger* errorLogger,
// Don't forward lifetime values // Don't forward lifetime values
argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
// Don't forward <Uninit> values, this is handled by CTU. We also had a FP #9347
argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isUninitValue));
if (argvalues.empty()) if (argvalues.empty())
continue; continue;

4
man/reference-cfg-format.md
View File

@ -172,6 +172,10 @@ Here is the minimal windows.cfg:
</function> </function>
</def> </def>
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 ### Null pointers
Cppcheck assumes it's ok to pass NULL pointers to functions. Here is an example program: Cppcheck assumes it's ok to pass NULL pointers to functions. Here is an example program:

6
test/testlibrary.cpp
View File

@ -229,7 +229,7 @@ private:
Library library; Library library;
ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); 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[2].notnull);
ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[3].formatstr); ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[3].formatstr);
ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[4].strz); ASSERT_EQUALS(true, library.functions["foo"].argumentChecks[4].strz);
@ -248,7 +248,7 @@ private:
Library library; Library library;
ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); 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 { void function_arg_variadic() const {
@ -262,7 +262,7 @@ private:
Library library; Library library;
ASSERT_EQUALS(true, Library::OK == (readLibrary(library, xmldata)).errorcode); 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); TokenList tokenList(nullptr);
std::istringstream istr("foo(a,b,c,d,e);"); std::istringstream istr("foo(a,b,c,d,e);");

9
test/testuninitvar.cpp
View File

@ -4285,6 +4285,13 @@ private:
"}"); "}");
ASSERT_EQUALS("[test.cpp:8]: (error) Uninitialized variable: a\n", errout.str()); 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" valueFlowUninit("void test(int p) {\n"
" int f;\n" " int f;\n"
" if (p > 0)\n" " if (p > 0)\n"
@ -4307,7 +4314,7 @@ private:
" someType_t gVar;\n" " someType_t gVar;\n"
" bar(&gVar);\n" " bar(&gVar);\n"
"}\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" valueFlowUninit("typedef struct \n"
"{\n" "{\n"