diff --git a/.travis_suppressions b/.travis_suppressions index 23c9413da..f802c6392 100644 --- a/.travis_suppressions +++ b/.travis_suppressions @@ -7,6 +7,7 @@ shadowFunction functionConst functionStatic bitwiseOnBoolean +hidingInheritedPublic # temporary suppressions - fix the warnings! duplicateBranch:lib/checkunusedvar.cpp diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 398b844db..fd6875e41 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2625,6 +2625,36 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI false); } +void CheckClass::checkAccessModifierVirtualFunctions() +{ + // Motivation: + // isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public + // stackoverflow.com/questions/484592/overriding-public-virtual-functions-with-private-functions-in-c + if (!mSettings->isEnabled(Settings::STYLE)) + return; + for (const Scope * classScope : mSymbolDatabase->classAndStructScopes) { + if (!classScope->definedType || classScope->definedType->derivedFrom.empty()) + continue; + for (const Function &func : classScope->functionList) { + const Function *baseFunc = func.getOverriddenFunction(); + if(baseFunc) { + if(AccessControl::Public == baseFunc->access && AccessControl::Public != func.access) { + checkAccessModifierVirtualFunctionsError(func.tokenDef, func.name()); + } + } + } + } +} + +void CheckClass::checkAccessModifierVirtualFunctionsError(const Token *tok, const std::string& func) +{ + reportError(tok, Severity::style, "hidingInheritedPublic", + "$symbol:" + func + "\n" + "The function '$symbol' has more narrow access modifier in a derived class. It could violate a LSP principle.", + CWE(0U) /* Unknown CWE! */, + false); +} + void CheckClass::checkUnsafeClassRefMember() { if (!mSettings->safeChecks.classes || !mSettings->isEnabled(Settings::WARNING)) diff --git a/lib/checkclass.h b/lib/checkclass.h index 4477d3be7..3904bf245 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -79,6 +79,7 @@ public: checkClass.checkCopyCtorAndEqOperator(); checkClass.checkOverride(); checkClass.checkUnsafeClassRefMember(); + checkClass.checkAccessModifierVirtualFunctions(); } /** @brief %Check that all class constructors are ok */ @@ -146,6 +147,8 @@ public: /** @brief Unsafe class check - const reference member */ void checkUnsafeClassRefMember(); + /** @brief Check that virtuial function has not least access in derived class */ + void checkAccessModifierVirtualFunctions(); private: const SymbolDatabase *mSymbolDatabase; @@ -183,6 +186,7 @@ private: void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor); void overrideError(const Function *funcInBase, const Function *funcInDerived); void unsafeClassRefMemberError(const Token *tok, const std::string &varname); + void checkAccessModifierVirtualFunctionsError(const Token *tok, const std::string& func); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckClass c(nullptr, settings, errorLogger); @@ -220,6 +224,7 @@ private: c.virtualFunctionCallInConstructorError(nullptr, std::list(), "f"); c.overrideError(nullptr, nullptr); c.unsafeClassRefMemberError(nullptr, "UnsafeClass::var"); + c.checkAccessModifierVirtualFunctionsError(nullptr, "f"); } static std::string myName() { diff --git a/test/testclass.cpp b/test/testclass.cpp index 2ab038125..87c490cd6 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -221,6 +221,8 @@ private: TEST_CASE(overrideCVRefQualifiers); TEST_CASE(unsafeClassRefMember); + + TEST_CASE(accessModifierVirtualFunctions); } void checkCopyCtorAndEqOperator(const char code[]) { @@ -7146,6 +7148,32 @@ private: ASSERT_EQUALS("", errout.str()); } + void checkAccessModifierVirtualFunctions(const char code[]) { + // Clear the error log + errout.str(""); + Settings settings; + settings.addEnabled("style"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.checkAccessModifierVirtualFunctions(); + } + + void accessModifierVirtualFunctions() { + checkAccessModifierVirtualFunctions("struct Base { virtual void f(); };\n" + "struct Derived : Base { private: virtual void f(); };"); + ASSERT_EQUALS("[test.cpp:2]: (style) The function 'f' has more narrow access modifier in a derived class. It could violate a LSP principle.\n", errout.str()); + + checkAccessModifierVirtualFunctions("struct Base { virtual void f(); };\n" + "struct Derived : Base { virtual void f(); };"); + ASSERT_EQUALS("", errout.str()); + } + void checkUnsafeClassRefMember(const char code[]) { // Clear the error log errout.str("");