Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/check.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,12 @@ class CPPCHECKLIB Check {
/** Base class used for whole-program analysis */
class CPPCHECKLIB FileInfo {
public:
FileInfo() = default;
explicit FileInfo(std::string f0 = {}) : file0(std::move(f0)) {}
virtual ~FileInfo() = default;
virtual std::string toString() const {
return std::string();
}
std::string file0;
};

virtual FileInfo * getFileInfo(const Tokenizer& /*tokenizer*/, const Settings& /*settings*/) const {
Expand Down
12 changes: 7 additions & 5 deletions lib/checkbufferoverrun.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ namespace
/** data for multifile checking */
class MyFileInfo : public Check::FileInfo {
public:
using Check::FileInfo::FileInfo;
Comment thread
chrchr-github marked this conversation as resolved.
/** unsafe array index usage */
std::list<CTU::FileInfo::UnsafeUsage> unsafeArrayIndex;

Expand Down Expand Up @@ -951,7 +952,7 @@ Check::FileInfo *CheckBufferOverrun::getFileInfo(const Tokenizer &tokenizer, con
if (unsafeArrayIndex.empty() && unsafePointerArith.empty()) {
return nullptr;
}
auto *fileInfo = new MyFileInfo;
auto *fileInfo = new MyFileInfo(tokenizer.list.getFiles()[0]);
fileInfo->unsafeArrayIndex = unsafeArrayIndex;
fileInfo->unsafePointerArith = unsafePointerArith;
return fileInfo;
Expand Down Expand Up @@ -998,14 +999,15 @@ bool CheckBufferOverrun::analyseWholeProgram(const CTU::FileInfo *ctu, const std
if (!fi)
continue;
for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafeArrayIndex)
foundErrors |= analyseWholeProgram1(callsMap, unsafeUsage, 1, errorLogger, settings.maxCtuDepth);
foundErrors |= analyseWholeProgram1(callsMap, unsafeUsage, 1, errorLogger, settings.maxCtuDepth, fi->file0);
for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafePointerArith)
foundErrors |= analyseWholeProgram1(callsMap, unsafeUsage, 2, errorLogger, settings.maxCtuDepth);
foundErrors |= analyseWholeProgram1(callsMap, unsafeUsage, 2, errorLogger, settings.maxCtuDepth, fi->file0);
}
return foundErrors;
}

bool CheckBufferOverrun::analyseWholeProgram1(const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage, int type, ErrorLogger &errorLogger, int maxCtuDepth)
bool CheckBufferOverrun::analyseWholeProgram1(const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage,
int type, ErrorLogger &errorLogger, int maxCtuDepth, const std::string& file0)
{
const CTU::FileInfo::FunctionCall *functionCall = nullptr;

Expand Down Expand Up @@ -1038,7 +1040,7 @@ bool CheckBufferOverrun::analyseWholeProgram1(const std::map<std::string, std::l
}

const ErrorMessage errorMessage(locationList,
emptyString,
file0,
Severity::error,
errmsg,
errorId,
Expand Down
3 changes: 2 additions & 1 deletion lib/checkbufferoverrun.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
static bool isCtuUnsafePointerArith(const Settings &settings, const Token *argtok, MathLib::bigint *offset);

Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const override;
static bool analyseWholeProgram1(const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage, int type, ErrorLogger &errorLogger, int maxCtuDepth);
static bool analyseWholeProgram1(const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage,
int type, ErrorLogger &errorLogger, int maxCtuDepth, const std::string& file0);


static std::string myName() {
Expand Down
5 changes: 3 additions & 2 deletions lib/checkclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3566,6 +3566,7 @@ namespace
/* multifile checking; one definition rule violations */
class MyFileInfo : public Check::FileInfo {
public:
using Check::FileInfo::FileInfo;
struct NameLoc {
std::string className;
std::string fileName;
Expand Down Expand Up @@ -3662,7 +3663,7 @@ Check::FileInfo *CheckClass::getFileInfo(const Tokenizer &tokenizer, const Setti
if (classDefinitions.empty())
return nullptr;

auto *fileInfo = new MyFileInfo;
auto *fileInfo = new MyFileInfo(tokenizer.list.getFiles()[0]);
fileInfo->classDefinitions.swap(classDefinitions);
return fileInfo;
}
Expand Down Expand Up @@ -3728,7 +3729,7 @@ bool CheckClass::analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<C
locationList.emplace_back(it->second.fileName, it->second.lineNumber, it->second.column);

const ErrorMessage errmsg(std::move(locationList),
emptyString,
fi->file0,
Severity::error,
"$symbol:" + nameLoc.className +
"\nThe one definition rule is violated, different classes/structs have the same name '$symbol'",
Expand Down
5 changes: 3 additions & 2 deletions lib/checknullpointer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ namespace
/* data for multifile checking */
class MyFileInfo : public Check::FileInfo {
public:
using Check::FileInfo::FileInfo;
/** function arguments that are dereferenced without checking if they are null */
std::list<CTU::FileInfo::UnsafeUsage> unsafeUsage;

Expand All @@ -617,7 +618,7 @@ Check::FileInfo *CheckNullPointer::getFileInfo(const Tokenizer &tokenizer, const
if (unsafeUsage.empty())
return nullptr;

auto *fileInfo = new MyFileInfo;
auto *fileInfo = new MyFileInfo(tokenizer.list.getFiles()[0]);
fileInfo->unsafeUsage = unsafeUsage;
return fileInfo;
}
Expand Down Expand Up @@ -667,7 +668,7 @@ bool CheckNullPointer::analyseWholeProgram(const CTU::FileInfo *ctu, const std::
continue;

const ErrorMessage errmsg(locationList,
emptyString,
fi->file0,
warning ? Severity::warning : Severity::error,
"Null pointer dereference: " + unsafeUsage.myArgumentName,
"ctunullpointer",
Expand Down
5 changes: 3 additions & 2 deletions lib/checkuninitvar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1708,6 +1708,7 @@ namespace
/* data for multifile checking */
class MyFileInfo : public Check::FileInfo {
public:
using Check::FileInfo::FileInfo;
/** function arguments that data are unconditionally read */
std::list<CTU::FileInfo::UnsafeUsage> unsafeUsage;

Expand All @@ -1725,7 +1726,7 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer &tokenizer, const S
if (unsafeUsage.empty())
return nullptr;

auto *fileInfo = new MyFileInfo;
auto *fileInfo = new MyFileInfo(tokenizer.list.getFiles()[0]);
fileInfo->unsafeUsage = unsafeUsage;
return fileInfo;
}
Expand Down Expand Up @@ -1769,7 +1770,7 @@ bool CheckUninitVar::analyseWholeProgram(const CTU::FileInfo *ctu, const std::li
continue;

const ErrorMessage errmsg(locationList,
emptyString,
fi->file0,
Severity::error,
"Using argument " + unsafeUsage.myArgumentName + " that points at uninitialized variable " + functionCall->callArgumentExpression,
"ctuuninitvar",
Expand Down
7 changes: 5 additions & 2 deletions lib/cppcheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2041,8 +2041,11 @@ unsigned int CppCheck::analyseWholeProgram(const std::string &buildDir, const st
}
// cppcheck-suppress shadowFunction - TODO: fix this
for (const Check *check : Check::instances()) {
if (checkClassAttr == check->name())
fileInfoList.push_back(check->loadFileInfoFromXml(e));
if (checkClassAttr == check->name()) {
Check::FileInfo* fi = check->loadFileInfoFromXml(e);
fi->file0 = filesTxtLine.substr(firstColon + 2);
fileInfoList.push_back(fi);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions test/cli/whole-program/nullpointer1.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include "nullpointer1.h"
8 changes: 8 additions & 0 deletions test/cli/whole-program/nullpointer1.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "nullpointer1_1.h"

template<typename T>
void f(T* p) {
if (sizeof(T) == 4)
p = nullptr;
g(p);
}
4 changes: 4 additions & 0 deletions test/cli/whole-program/nullpointer1_1.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
template<typename T>
void g(T* p) {
*p = 0;
};
33 changes: 33 additions & 0 deletions test/cli/whole-program_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import json
import shutil
from testutils import cppcheck
import xml.etree.ElementTree as ET

__script_dir = os.path.dirname(os.path.abspath(__file__))

Expand Down Expand Up @@ -358,3 +359,35 @@ def test_checkclass_project_builddir_j(tmpdir):
build_dir = os.path.join(tmpdir, 'b1')
os.mkdir(build_dir)
__test_checkclass_project(tmpdir, ['-j2', '--cppcheck-build-dir={}'.format(build_dir)])

def __test_nullpointer_file0(extra_args):
args = [
'-q',
'--xml',
'--error-exitcode=1',
'whole-program/nullpointer1.cpp'
]

args += extra_args

ret, stdout, stderr = cppcheck(args, cwd=__script_dir)
results = ET.fromstring(stderr)
file0 = ''
for e in results.findall('errors/error[@id="ctunullpointer"]'):
file0 = e.attrib['file0']
assert file0 == 'whole-program/nullpointer1.cpp'
assert stdout == ''
assert ret == 1, stdout


def test_nullpointer_file0():
__test_nullpointer_file0(['-j1'])
Comment thread
chrchr-github marked this conversation as resolved.

@pytest.mark.skip(reason="flaky!?")
Copy link
Copy Markdown
Collaborator

@firewave firewave Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert file0 == 'whole-program/nullpointer1.cpp'
assert stdout == ''
assert ret == 1, stdout
def test_nullpointer_file0():
__test_nullpointer_file0(['-j1'])
@pytest.mark.skip(reason="flaky!?")
assert ret == 1, stdout if stdout else stderr
assert stdout == ''
assert file0 == 'whole-program/nullpointer1.cpp', stderr
def test_nullpointer_file0():
__test_nullpointer_file0(['-j1'])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I would assume it is an issue with running the test in parallel but there is nothing in this test which could cause that.

But this test is build with UBSAN so it might indicate such an issue. The asserts are in a bad order so that would be obfuscated. I adjusted them so we should get more details (that needs to be applied across all the tests - there are already some initial helpers but something for later).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The xfail is still in, so it might pass now but we might not see the error reported by the sanitizer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With xfail in, Run test/cli (--cppcheck-build-dir) fails (test succeeds). Without xfail, Run test/cli fails as expected.
How is this injection stuff supposed to work? I also see extraargs being used in the test.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The injection stuff checks if any of the parameters in question already exists and only injects those is necessary. The -j2 injection will not be done if -j1 already exists. Same with --cppcheck-build-dir and --executor. So if you have to specify them explicitly they will not return the same no matter how it is called (e.g. Windows does not support --executor=process) and you need to provide an override.

This looks weird though. Will take a look later.

def test_nullpointer_file0_j():
__test_nullpointer_file0(['-j2'])

def test_nullpointer_file0_builddir_j(tmpdir):
build_dir = os.path.join(tmpdir, 'b1')
Comment thread
chrchr-github marked this conversation as resolved.
os.mkdir(build_dir)
__test_nullpointer_file0(['-j2', '--cppcheck-build-dir={}'.format(build_dir)])