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

New check: Detect one definition rule violations

This commit is contained in:
Daniel Marjamäki 2021-02-09 21:35:08 +01:00
parent 0e871c178f
commit ef726aaece
3 changed files with 221 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

145
lib/checkclass.cpp
View File

@ -24,13 +24,17 @@
#include "settings.h"
#include "standards.h"
#include "symboldatabase.h"
#include "errorlogger.h"
#include "errortypes.h"
#include "token.h"
#include "tokenize.h"
#include "utils.h"
#include "tinyxml2.h"
#include <algorithm>
#include <cstdlib>
#include <functional>
#include <utility>
//---------------------------------------------------------------------------
@ -45,6 +49,8 @@ static const CWE CWE665(665U); // Improper Initialization
static const CWE CWE758(758U); // Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
static const CWE CWE762(762U); // Mismatched Memory Management Routines
static const CWE CWE_ONE_DEFINITION_RULE(758U);
static const char * getFunctionTypeName(Function::Type type)
{
switch (type) {
@ -2827,3 +2833,142 @@ void CheckClass::unsafeClassRefMemberError(const Token *tok, const std::string &
"Unsafe class checking: The const reference member '$symbol' is initialized by a const reference constructor argument. You need to be careful about lifetime issues. If you pass a local variable or temporary value in this constructor argument, be extra careful. If the argument is always some global object that is never destroyed then this is safe usage. However it would be defensive to make the member '$symbol' a non-reference variable or a smart pointer.",
CWE(0), false);
}
Check::FileInfo *CheckClass::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
{
if (!tokenizer->isCPP())
return nullptr;
(void)settings;
// One definition rule
std::vector<MyFileInfo::NameLoc> classDefinitions;
for (const Scope * classScope : tokenizer->getSymbolDatabase()->classAndStructScopes) {
// the full definition must be compared
bool fullDefinition = true;
for (const Function &f: classScope->functionList)
fullDefinition &= f.hasBody();
if (!fullDefinition)
continue;
std::string name;
const Scope *scope = classScope;
while (scope->isClassOrStruct() && !classScope->className.empty()) {
name = scope->className + "::" + name;
scope = scope->nestedIn;
}
name.erase(name.size() - 2);
if (scope->type != Scope::ScopeType::eGlobal)
continue;
MyFileInfo::NameLoc nameLoc;
nameLoc.className = name;
nameLoc.fileName = tokenizer->list.file(classScope->classDef);
nameLoc.lineNumber = classScope->classDef->linenr();
nameLoc.column = classScope->classDef->column();
// Calculate hash from the full class/struct definition
std::string def;
for (const Token *tok = classScope->classDef; tok != classScope->bodyEnd; tok = tok->next())
def += tok->str();
for (const Function &f: classScope->functionList) {
if (f.functionScope->nestedIn != classScope) {
for (const Token *tok = f.functionScope->bodyStart; tok != f.functionScope->bodyEnd; tok = tok->next())
def += tok->str();
}
}
nameLoc.hash = std::hash<std::string> {}(def);
classDefinitions.push_back(nameLoc);
}
if (classDefinitions.empty())
return nullptr;
MyFileInfo *fileInfo = new MyFileInfo;
fileInfo->classDefinitions.swap(classDefinitions);
return fileInfo;
}
std::string CheckClass::MyFileInfo::toString() const
{
std::string ret;
for (const MyFileInfo::NameLoc &nameLoc: classDefinitions) {
ret += "<class name=\"" + nameLoc.className +
"\" file=\"" + ErrorLogger::toxml(nameLoc.fileName) +
"\" line=\"" + std::to_string(nameLoc.lineNumber) +
"\" col=\"" + std::to_string(nameLoc.column) +
"\" hash=\"" + std::to_string(nameLoc.hash) +
"\"/>\n";
}
return ret;
}
Check::FileInfo * CheckClass::loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const
{
MyFileInfo *fileInfo = new MyFileInfo;
for (const tinyxml2::XMLElement *e = xmlElement->FirstChildElement(); e; e = e->NextSiblingElement()) {
if (std::strcmp(e->Name(), "class") != 0)
continue;
const char *name = e->Attribute("name");
const char *file = e->Attribute("file");
const char *line = e->Attribute("line");
const char *col = e->Attribute("col");
const char *hash = e->Attribute("hash");
if (name && file && line && col && hash) {
MyFileInfo::NameLoc nameLoc;
nameLoc.className = name;
nameLoc.fileName = file;
nameLoc.lineNumber = std::atoi(line);
nameLoc.column = std::atoi(col);
nameLoc.hash = MathLib::toULongNumber(hash);
fileInfo->classDefinitions.push_back(nameLoc);
}
}
if (fileInfo->classDefinitions.empty()) {
delete fileInfo;
fileInfo = nullptr;
}
return fileInfo;
}
bool CheckClass::analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger)
{
bool foundErrors = false;
(void)ctu; // This argument is unused
(void)settings; // This argument is unused
std::unordered_map<std::string, MyFileInfo::NameLoc> all;
for (Check::FileInfo *fi1 : fileInfo) {
const MyFileInfo *fi = dynamic_cast<MyFileInfo*>(fi1);
if (!fi)
continue;
for (const MyFileInfo::NameLoc &nameLoc : fi->classDefinitions) {
auto it = all.find(nameLoc.className);
if (it == all.end()) {
all[nameLoc.className] = nameLoc;
continue;
}
if (it->second.hash == nameLoc.hash)
continue;
std::list<ErrorMessage::FileLocation> locationList;
locationList.emplace_back(nameLoc.fileName, nameLoc.lineNumber, nameLoc.column);
locationList.emplace_back(it->second.fileName, it->second.lineNumber, it->second.column);
const ErrorMessage errmsg(locationList,
emptyString,
Severity::error,
"$symbol:" + nameLoc.className +
"\nThe one definition rule is violated, different classes/structs have the same name '$symbol'",
"ctuOneDefinitionRuleViolation",
CWE_ONE_DEFINITION_RULE,
false);
errorLogger.reportErr(errmsg);
foundErrors = true;
}
}
return foundErrors;
}

36
lib/checkclass.h
View File

@ -149,6 +149,38 @@ public:
/** @brief Unsafe class check - const reference member */
void checkUnsafeClassRefMember();
/* multifile checking; one definition rule violations */
class MyFileInfo : public Check::FileInfo {
public:
struct NameLoc {
std::string className;
std::string fileName;
int lineNumber;
int column;
std::size_t hash;
bool operator==(const NameLoc& other) const {
return fileName == other.fileName &&
lineNumber == other.lineNumber &&
column == other.column &&
hash == other.hash;
}
};
std::vector<NameLoc> classDefinitions;
/** Convert MyFileInfo data into xml string */
std::string toString() const OVERRIDE;
};
/** @brief Parse current TU and extract file info */
Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const OVERRIDE;
Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const OVERRIDE;
/** @brief Analyse all file infos for all TU */
bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE;
private:
const SymbolDatabase *mSymbolDatabase;
@ -254,7 +286,8 @@ private:
// disabled for now "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n"
"- Check that arbitrary usage of public interface does not result in division by zero\n"
"- Delete \"self pointer\" and then access 'this'\n"
"- Check that the 'override' keyword is used when overriding virtual functions\n";
"- Check that the 'override' keyword is used when overriding virtual functions\n"
"- Check that the 'one definition rule' is not violated\n";
}
// operatorEqRetRefThis helper functions
@ -364,7 +397,6 @@ private:
* @brief Helper for checkThisUseAfterFree
*/
bool checkThisUseAfterFreeRecursive(const Scope *classScope, const Function *func, const Variable *selfPointer, std::set<const Function *> callstack, const Token **freeToken);
};
/// @}
//---------------------------------------------------------------------------

42
test/testclass.cpp
View File

@ -19,6 +19,7 @@
#include <tinyxml2.h>
#include "checkclass.h"
#include "ctu.h"
#include "library.h"
#include "settings.h"
#include "testsuite.h"
@ -222,6 +223,8 @@ private:
TEST_CASE(checkThisUseAfterFree);
TEST_CASE(unsafeClassRefMember);
TEST_CASE(ctuOneDefinitionRule);
}
void checkCopyCtorAndEqOperator(const char code[]) {
@ -7398,6 +7401,45 @@ private:
"}");
ASSERT_EQUALS("", errout.str());
}
void ctu(const std::vector<std::string> &code) {
Settings settings;
CheckClass check;
// getFileInfo
std::list<Check::FileInfo*> fileInfo;
for (const std::string& c: code) {
Tokenizer tokenizer(&settings, this);
std::istringstream istr(c);
tokenizer.tokenize(istr, (std::to_string(fileInfo.size()) + ".cpp").c_str());
fileInfo.push_back(check.getFileInfo(&tokenizer, &settings));
}
// Check code..
errout.str("");
check.analyseWholeProgram(nullptr, fileInfo, settings, *this);
while (!fileInfo.empty()) {
delete fileInfo.back();
fileInfo.pop_back();
}
}
void ctuOneDefinitionRule() {
ctu({"class C { C() { std::cout << 0; } };", "class C { C() { std::cout << 1; } };"});
ASSERT_EQUALS("[1.cpp:1] -> [0.cpp:1]: (error) The one definition rule is violated, different classes/structs have the same name 'C'\n", errout.str());
ctu({"class C { C(); }; C::C() { std::cout << 0; }", "class C { C(); }; C::C() { std::cout << 1; }"});
ASSERT_EQUALS("[1.cpp:1] -> [0.cpp:1]: (error) The one definition rule is violated, different classes/structs have the same name 'C'\n", errout.str());
ctu({"class C { C() {} };\n", "class C { C() {} };\n"});
ASSERT_EQUALS("", errout.str());
ctu({"class C { C(); }; C::C(){}", "class C { C(); }; C::C(){}"});
ASSERT_EQUALS("", errout.str());
}
};
REGISTER_TEST(TestClass)