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

Fixed #10161 (False negative; uninitialized member variable in base class without constructor)

This commit is contained in:
Daniel Marjamäki 2021-02-01 18:58:51 +01:00
parent bd9e6212b2
commit 986f658e39
3 changed files with 71 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
lib/checkclass.cpp
View File

@ -138,8 +138,7 @@ void CheckClass::constructors()
std::vector<Usage> usageList; std::vector<Usage> usageList;
for (const Variable &var : scope->varlist) createUsageList(usageList, scope);
usageList.push_back(Usage(&var));
for (const Function &func : scope->functionList) { for (const Function &func : scope->functionList) {
if (!func.hasBody() || !(func.isConstructor() || func.type == Function::eOperatorEqual)) if (!func.hasBody() || !(func.isConstructor() || func.type == Function::eOperatorEqual))
@ -229,6 +228,7 @@ void CheckClass::constructors()
continue; continue;
const Scope *varType = var.typeScope(); const Scope *varType = var.typeScope();
if (!varType || varType->type != Scope::eUnion) { if (!varType || varType->type != Scope::eUnion) {
const bool derived = scope != var.scope();
if (func.type == Function::eConstructor && if (func.type == Function::eConstructor &&
func.nestedIn && (func.nestedIn->numConstructors - func.nestedIn->numCopyOrMoveConstructors) > 1 && func.nestedIn && (func.nestedIn->numConstructors - func.nestedIn->numCopyOrMoveConstructors) > 1 &&
func.argCount() == 0 && func.functionScope && func.argCount() == 0 && func.functionScope &&
@ -236,9 +236,9 @@ void CheckClass::constructors()
func.functionScope->bodyStart->link() == func.functionScope->bodyStart->next()) { func.functionScope->bodyStart->link() == func.functionScope->bodyStart->next()) {
// don't warn about user defined default constructor when there are other constructors // don't warn about user defined default constructor when there are other constructors
if (printInconclusive) if (printInconclusive)
uninitVarError(func.token, func.access == AccessControl::Private, func.type, scope->className, var.name(), true); uninitVarError(func.token, func.access == AccessControl::Private, func.type, var.scope()->className, var.name(), derived, true);
} else } else
uninitVarError(func.token, func.access == AccessControl::Private, func.type, scope->className, var.name(), inconclusive); uninitVarError(func.token, func.access == AccessControl::Private, func.type, var.scope()->className, var.name(), derived, inconclusive);
} }
} }
} }
@ -535,13 +535,24 @@ bool CheckClass::canNotMove(const Scope *scope)
return constructor && !(publicAssign || publicCopy || publicMove); return constructor && !(publicAssign || publicCopy || publicMove);
} }
void CheckClass::assignVar(nonneg int varid, const Scope *scope, std::vector<Usage> &usage) void CheckClass::createUsageList(std::vector<Usage>& usageList, const Scope *scope)
{ {
int count = 0; for (const Variable& var: scope->varlist)
usageList.push_back(Usage(&var));
if (scope->definedType) {
for (const Type::BaseInfo& baseInfo: scope->definedType->derivedFrom) {
const Scope *baseClass = baseInfo.type ? baseInfo.type->classScope : nullptr;
if (baseClass && baseClass->isClassOrStruct() && baseClass->numConstructors == 0)
createUsageList(usageList, baseClass);
}
}
}
for (std::list<Variable>::const_iterator var = scope->varlist.begin(); var != scope->varlist.end(); ++var, ++count) { void CheckClass::assignVar(std::vector<Usage> &usageList, nonneg int varid)
if (var->declarationId() == varid) { {
usage[count].assign = true; for (Usage& usage: usageList) {
if (usage.var->declarationId() == varid) {
usage.assign = true;
return; return;
} }
} }
@ -639,7 +650,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
} }
} }
} else if (level != 0 && Token::Match(ftok, "%name% =")) // assignment in the initializer: var(value = x) } else if (level != 0 && Token::Match(ftok, "%name% =")) // assignment in the initializer: var(value = x)
assignVar(ftok->varId(), scope, usage); assignVar(usage, ftok->varId());
// Level handling // Level handling
if (ftok->link() && Token::Match(ftok, "(|<")) if (ftok->link() && Token::Match(ftok, "(|<"))
@ -659,7 +670,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
// Variable getting value from stream? // Variable getting value from stream?
if (Token::Match(ftok, ">>|& %name%") && isLikelyStreamRead(true, ftok)) { if (Token::Match(ftok, ">>|& %name%") && isLikelyStreamRead(true, ftok)) {
assignVar(ftok->next()->varId(), scope, usage); assignVar(usage, ftok->next()->varId());
} }
// If assignment comes after an && or || this is really inconclusive because of short circuiting // If assignment comes after an && or || this is really inconclusive because of short circuiting
@ -686,7 +697,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
for (const Variable &var : scope->varlist) { for (const Variable &var : scope->varlist) {
if (var.declarationId() == ftok->next()->varId()) { if (var.declarationId() == ftok->next()->varId()) {
/** @todo false negative: we assume function changes variable state */ /** @todo false negative: we assume function changes variable state */
assignVar(ftok->next()->varId(), scope, usage); assignVar(usage, ftok->next()->varId());
break; break;
} }
} }
@ -735,7 +746,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
int offsetToMember = 4; int offsetToMember = 4;
if (ftok->strAt(2) == "&") if (ftok->strAt(2) == "&")
++offsetToMember; ++offsetToMember;
assignVar(ftok->tokAt(offsetToMember)->varId(), scope, usage); assignVar(usage, ftok->tokAt(offsetToMember)->varId());
ftok = ftok->linkAt(1); ftok = ftok->linkAt(1);
continue; continue;
} }
@ -744,7 +755,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
else if (Token::Match(ftok, "::| memset ( %name% ,")) { else if (Token::Match(ftok, "::| memset ( %name% ,")) {
if (ftok->str() == "::") if (ftok->str() == "::")
ftok = ftok->next(); ftok = ftok->next();
assignVar(ftok->tokAt(2)->varId(), scope, usage); assignVar(usage, ftok->tokAt(2)->varId());
ftok = ftok->linkAt(1); ftok = ftok->linkAt(1);
continue; continue;
} }
@ -819,7 +830,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
tok2 = tok2->next(); tok2 = tok2->next();
if (tok2->str() == "&") if (tok2->str() == "&")
tok2 = tok2->next(); tok2 = tok2->next();
assignVar(tok2->varId(), scope, usage); assignVar(usage, tok2->varId());
} }
} }
} }
@ -849,7 +860,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
else { else {
for (const Token *tok = ftok->tokAt(2); tok && tok != ftok->next()->link(); tok = tok->next()) { for (const Token *tok = ftok->tokAt(2); tok && tok != ftok->next()->link(); tok = tok->next()) {
if (tok->isName()) { if (tok->isName()) {
assignVar(tok->varId(), scope, usage); assignVar(usage, tok->varId());
} }
} }
} }
@ -858,7 +869,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
// Assignment of member variable? // Assignment of member variable?
else if (Token::Match(ftok, "%name% =")) { else if (Token::Match(ftok, "%name% =")) {
assignVar(ftok->varId(), scope, usage); assignVar(usage, ftok->varId());
bool bailout = ftok->variable() && ftok->variable()->isReference(); bool bailout = ftok->variable() && ftok->variable()->isReference();
const Token* tok2 = ftok->tokAt(2); const Token* tok2 = ftok->tokAt(2);
if (tok2->str() == "&") { if (tok2->str() == "&") {
@ -866,7 +877,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
bailout = true; bailout = true;
} }
if (tok2->variable() && (bailout || tok2->variable()->isArray()) && tok2->strAt(1) != "[") if (tok2->variable() && (bailout || tok2->variable()->isArray()) && tok2->strAt(1) != "[")
assignVar(tok2->varId(), scope, usage); assignVar(usage, tok2->varId());
} }
// Assignment of array item of member variable? // Assignment of array item of member variable?
@ -881,19 +892,19 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
break; break;
} }
if (tok2 && tok2->strAt(1) == "=") if (tok2 && tok2->strAt(1) == "=")
assignVar(ftok->varId(), scope, usage); assignVar(usage, ftok->varId());
} }
// Assignment of array item of member variable? // Assignment of array item of member variable?
else if (Token::Match(ftok, "* %name% =")) { else if (Token::Match(ftok, "* %name% =")) {
assignVar(ftok->next()->varId(), scope, usage); assignVar(usage, ftok->next()->varId());
} else if (Token::Match(ftok, "* this . %name% =")) { } else if (Token::Match(ftok, "* this . %name% =")) {
assignVar(ftok->tokAt(3)->varId(), scope, usage); assignVar(usage, ftok->tokAt(3)->varId());
} }
// The functions 'clear' and 'Clear' are supposed to initialize variable. // The functions 'clear' and 'Clear' are supposed to initialize variable.
if (Token::Match(ftok, "%name% . clear|Clear (")) { if (Token::Match(ftok, "%name% . clear|Clear (")) {
assignVar(ftok->varId(), scope, usage); assignVar(usage, ftok->varId());
} }
} }
} }
@ -916,14 +927,17 @@ void CheckClass::noExplicitConstructorError(const Token *tok, const std::string
reportError(tok, Severity::style, "noExplicitConstructor", "$symbol:" + classname + '\n' + message + '\n' + verbose, CWE398, false); reportError(tok, Severity::style, "noExplicitConstructor", "$symbol:" + classname + '\n' + message + '\n' + verbose, CWE398, false);
} }
void CheckClass::uninitVarError(const Token *tok, bool isprivate, Function::Type functionType, const std::string &classname, const std::string &varname, bool inconclusive) void CheckClass::uninitVarError(const Token *tok, bool isprivate, Function::Type functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive)
{ {
std::string message; std::string message;
if ((functionType == Function::eCopyConstructor || functionType == Function::eMoveConstructor) && inconclusive) if ((functionType == Function::eCopyConstructor || functionType == Function::eMoveConstructor) && inconclusive)
message = "Member variable '$symbol' is not assigned in the copy constructor. Should it be copied?"; message = "Member variable '$symbol' is not assigned in the copy constructor. Should it be copied?";
else else
message = "Member variable '$symbol' is not initialized in the constructor."; message = "Member variable '$symbol' is not initialized in the constructor.";
reportError(tok, Severity::warning, isprivate ? "uninitMemberVarPrivate" : "uninitMemberVar", "$symbol:" + classname + "::" + varname + "\n" + message, CWE398, inconclusive); if (derived)
message += " Maybe it should be initialized directly in the class " + classname + "?";
std::string id = std::string("uninit") + (derived ? "Derived" : "") + "MemberVar" + (isprivate ? "Private" : "");
reportError(tok, Severity::warning, id, "$symbol:" + classname + "::" + varname + "\n" + message, CWE398, inconclusive);
} }
void CheckClass::operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive) void CheckClass::operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive)

25
lib/checkclass.h
View File

@ -160,7 +160,7 @@ private:
void noCopyConstructorError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive); void noCopyConstructorError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive);
void noOperatorEqError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive); void noOperatorEqError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive);
void noDestructorError(const Scope *scope, bool isdefault, const Token *alloc); void noDestructorError(const Scope *scope, bool isdefault, const Token *alloc);
void uninitVarError(const Token *tok, bool isprivate, Function::Type functionType, const std::string &classname, const std::string &varname, bool inconclusive); void uninitVarError(const Token *tok, bool isprivate, Function::Type functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive);
void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive); void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive);
void unusedPrivateFunctionError(const Token *tok, const std::string &classname, const std::string &funcname); void unusedPrivateFunctionError(const Token *tok, const std::string &classname, const std::string &funcname);
void memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type); void memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type);
@ -197,8 +197,10 @@ private:
c.noCopyConstructorError(nullptr, false, nullptr, false); c.noCopyConstructorError(nullptr, false, nullptr, false);
c.noOperatorEqError(nullptr, false, nullptr, false); c.noOperatorEqError(nullptr, false, nullptr, false);
c.noDestructorError(nullptr, false, nullptr); c.noDestructorError(nullptr, false, nullptr);
c.uninitVarError(nullptr, false, Function::eConstructor, "classname", "varname", false); c.uninitVarError(nullptr, false, Function::eConstructor, "classname", "varname", false, false);
c.uninitVarError(nullptr, true, Function::eConstructor, "classname", "varnamepriv", false); c.uninitVarError(nullptr, true, Function::eConstructor, "classname", "varnamepriv", false, false);
c.uninitVarError(nullptr, false, Function::eConstructor, "classname", "varname", true, false);
c.uninitVarError(nullptr, true, Function::eConstructor, "classname", "varnamepriv", true, false);
c.operatorEqVarError(nullptr, "classname", emptyString, false); c.operatorEqVarError(nullptr, "classname", emptyString, false);
c.unusedPrivateFunctionError(nullptr, "classname", "funcname"); c.unusedPrivateFunctionError(nullptr, "classname", "funcname");
c.memsetError(nullptr, "memfunc", "classname", "class"); c.memsetError(nullptr, "memfunc", "classname", "class");
@ -287,12 +289,19 @@ private:
static bool isBaseClassFunc(const Token *tok, const Scope *scope); static bool isBaseClassFunc(const Token *tok, const Scope *scope);
/** /**
* @brief assign a variable in the varlist * @brief Create usage list that contains all scope members and also members
* @param varid id of variable to mark assigned * of base classes without constructors.
* @param scope pointer to variable Scope * @param usageList usageList that will be filled up
* @param usage reference to usage vector * @param scope current class scope
*/ */
static void assignVar(nonneg int varid, const Scope *scope, std::vector<Usage> &usage); static void createUsageList(std::vector<Usage>& usageList, const Scope *scope);
/**
* @brief assign a variable in the varlist
* @param usageList reference to usage vector
* @param varid id of variable to mark assigned
*/
static void assignVar(std::vector<Usage> &usageList, nonneg int varid);
/** /**
* @brief initialize a variable in the varlist * @brief initialize a variable in the varlist

16
test/testconstructors.cpp
View File

@ -97,6 +97,7 @@ private:
TEST_CASE(initvar_union); TEST_CASE(initvar_union);
TEST_CASE(initvar_delegate); // ticket #4302 TEST_CASE(initvar_delegate); // ticket #4302
TEST_CASE(initvar_delegate2); TEST_CASE(initvar_delegate2);
TEST_CASE(initvar_derived_class); // ticket #10161
TEST_CASE(initvar_private_constructor); // BUG 2354171 - private constructor TEST_CASE(initvar_private_constructor); // BUG 2354171 - private constructor
TEST_CASE(initvar_copy_constructor); // ticket #1611 TEST_CASE(initvar_copy_constructor); // ticket #1611
@ -1180,6 +1181,21 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
} }
void initvar_derived_class() {
check("class Base {\n"
"public:\n"
" virtual void foo() = 0;\n"
" int x;\n" // <- uninitialized
"};\n"
"\n"
"class Derived: public Base {\n"
"public:\n"
" Derived() {}\n"
" void foo() override;\n"
"};");
ASSERT_EQUALS("[test.cpp:9]: (warning) Member variable 'Base::x' is not initialized in the constructor. Maybe it should be initialized directly in the class Base?\n", errout.str());
}
void initvar_private_constructor() { void initvar_private_constructor() {
settings.standards.cpp = Standards::CPP11; settings.standards.cpp = Standards::CPP11;
check("class Fred\n" check("class Fred\n"