From 986f658e39c572316f547bfb7ce3cb9b4ee800e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 1 Feb 2021 18:58:51 +0100 Subject: [PATCH] Fixed #10161 (False negative; uninitialized member variable in base class without constructor) --- lib/checkclass.cpp | 62 ++++++++++++++++++++++++--------------- lib/checkclass.h | 25 +++++++++++----- test/testconstructors.cpp | 16 ++++++++++ 3 files changed, 71 insertions(+), 32 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 9f5e44ddd..34d3dc20f 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -138,8 +138,7 @@ void CheckClass::constructors() std::vector usageList; - for (const Variable &var : scope->varlist) - usageList.push_back(Usage(&var)); + createUsageList(usageList, scope); for (const Function &func : scope->functionList) { if (!func.hasBody() || !(func.isConstructor() || func.type == Function::eOperatorEqual)) @@ -229,6 +228,7 @@ void CheckClass::constructors() continue; const Scope *varType = var.typeScope(); if (!varType || varType->type != Scope::eUnion) { + const bool derived = scope != var.scope(); if (func.type == Function::eConstructor && func.nestedIn && (func.nestedIn->numConstructors - func.nestedIn->numCopyOrMoveConstructors) > 1 && func.argCount() == 0 && func.functionScope && @@ -236,9 +236,9 @@ void CheckClass::constructors() func.functionScope->bodyStart->link() == func.functionScope->bodyStart->next()) { // don't warn about user defined default constructor when there are other constructors 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 - 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); } -void CheckClass::assignVar(nonneg int varid, const Scope *scope, std::vector &usage) +void CheckClass::createUsageList(std::vector& 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::const_iterator var = scope->varlist.begin(); var != scope->varlist.end(); ++var, ++count) { - if (var->declarationId() == varid) { - usage[count].assign = true; +void CheckClass::assignVar(std::vector &usageList, nonneg int varid) +{ + for (Usage& usage: usageList) { + if (usage.var->declarationId() == varid) { + usage.assign = true; return; } } @@ -639,7 +650,7 @@ void CheckClass::initializeVarList(const Function &func, std::list>|& %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 @@ -686,7 +697,7 @@ void CheckClass::initializeVarList(const Function &func, std::listvarlist) { if (var.declarationId() == ftok->next()->varId()) { /** @todo false negative: we assume function changes variable state */ - assignVar(ftok->next()->varId(), scope, usage); + assignVar(usage, ftok->next()->varId()); break; } } @@ -735,7 +746,7 @@ void CheckClass::initializeVarList(const Function &func, std::liststrAt(2) == "&") ++offsetToMember; - assignVar(ftok->tokAt(offsetToMember)->varId(), scope, usage); + assignVar(usage, ftok->tokAt(offsetToMember)->varId()); ftok = ftok->linkAt(1); continue; } @@ -744,7 +755,7 @@ void CheckClass::initializeVarList(const Function &func, std::liststr() == "::") ftok = ftok->next(); - assignVar(ftok->tokAt(2)->varId(), scope, usage); + assignVar(usage, ftok->tokAt(2)->varId()); ftok = ftok->linkAt(1); continue; } @@ -819,7 +830,7 @@ void CheckClass::initializeVarList(const Function &func, std::listnext(); if (tok2->str() == "&") tok2 = tok2->next(); - assignVar(tok2->varId(), scope, usage); + assignVar(usage, tok2->varId()); } } } @@ -849,7 +860,7 @@ void CheckClass::initializeVarList(const Function &func, std::listtokAt(2); tok && tok != ftok->next()->link(); tok = tok->next()) { if (tok->isName()) { - assignVar(tok->varId(), scope, usage); + assignVar(usage, tok->varId()); } } } @@ -858,7 +869,7 @@ void CheckClass::initializeVarList(const Function &func, std::listvariable() && (bailout || tok2->variable()->isArray()) && tok2->strAt(1) != "[") - assignVar(tok2->varId(), scope, usage); + assignVar(usage, tok2->varId()); } // Assignment of array item of member variable? @@ -881,19 +892,19 @@ void CheckClass::initializeVarList(const Function &func, std::liststrAt(1) == "=") - assignVar(ftok->varId(), scope, usage); + assignVar(usage, ftok->varId()); } // Assignment of array item of member variable? else if (Token::Match(ftok, "* %name% =")) { - assignVar(ftok->next()->varId(), scope, usage); + assignVar(usage, ftok->next()->varId()); } 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. 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); } -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; if ((functionType == Function::eCopyConstructor || functionType == Function::eMoveConstructor) && inconclusive) message = "Member variable '$symbol' is not assigned in the copy constructor. Should it be copied?"; else 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) diff --git a/lib/checkclass.h b/lib/checkclass.h index 8f0c8e688..6be4cf6d9 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -160,7 +160,7 @@ private: 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 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 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); @@ -197,8 +197,10 @@ private: c.noCopyConstructorError(nullptr, false, nullptr, false); c.noOperatorEqError(nullptr, false, nullptr, false); c.noDestructorError(nullptr, false, nullptr); - c.uninitVarError(nullptr, false, Function::eConstructor, "classname", "varname", false); - c.uninitVarError(nullptr, true, Function::eConstructor, "classname", "varnamepriv", false); + c.uninitVarError(nullptr, false, Function::eConstructor, "classname", "varname", false, 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.unusedPrivateFunctionError(nullptr, "classname", "funcname"); c.memsetError(nullptr, "memfunc", "classname", "class"); @@ -287,12 +289,19 @@ private: static bool isBaseClassFunc(const Token *tok, const Scope *scope); /** - * @brief assign a variable in the varlist - * @param varid id of variable to mark assigned - * @param scope pointer to variable Scope - * @param usage reference to usage vector + * @brief Create usage list that contains all scope members and also members + * of base classes without constructors. + * @param usageList usageList that will be filled up + * @param scope current class scope */ - static void assignVar(nonneg int varid, const Scope *scope, std::vector &usage); + static void createUsageList(std::vector& 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 &usageList, nonneg int varid); /** * @brief initialize a variable in the varlist diff --git a/test/testconstructors.cpp b/test/testconstructors.cpp index 14a277689..52dafa11a 100644 --- a/test/testconstructors.cpp +++ b/test/testconstructors.cpp @@ -97,6 +97,7 @@ private: TEST_CASE(initvar_union); TEST_CASE(initvar_delegate); // ticket #4302 TEST_CASE(initvar_delegate2); + TEST_CASE(initvar_derived_class); // ticket #10161 TEST_CASE(initvar_private_constructor); // BUG 2354171 - private constructor TEST_CASE(initvar_copy_constructor); // ticket #1611 @@ -1180,6 +1181,21 @@ private: 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() { settings.standards.cpp = Standards::CPP11; check("class Fred\n"