Skip to content

Commit fe8a5aa

Browse files
firewavefirewave
authored
fixed #13660 - SuppressionList::Suppression::checked was not properly set in all cases (danmar#7380)
This was initially introduced for inline suppressions but was actually used beyond that.
1 parent 2cea0d0 commit fe8a5aa

File tree

10 files changed

+284
-51
lines changed

10 files changed

+284
-51
lines changed

‎cli/processexecutor.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ ProcessExecutor::ProcessExecutor(const std::list<FileWithDetails> &files, const
7878
namespace {
7979
class PipeWriter : public ErrorLogger {
8080
public:
81-
enum PipeSignal : std::uint8_t {REPORT_OUT='1',REPORT_ERROR='2',REPORT_SUPPR_INLINE='3',CHILD_END='5'};
81+
enum PipeSignal : std::uint8_t {REPORT_OUT='1',REPORT_ERROR='2',REPORT_SUPPR_INLINE='3',REPORT_SUPPR='4',CHILD_END='5'};
8282

8383
explicit PipeWriter(int pipe) : mWpipe(pipe) {}
8484

@@ -93,12 +93,11 @@ namespace {
9393
void writeSuppr(const SuppressionList &supprs) const {
9494
for (const auto& suppr : supprs.getSuppressions())
9595
{
96-
if (!suppr.isInline)
97-
continue;
98-
99-
writeToPipe(REPORT_SUPPR_INLINE, suppressionToString(suppr));
96+
if (suppr.isInline)
97+
writeToPipe(REPORT_SUPPR_INLINE, suppressionToString(suppr));
98+
else if (suppr.checked)
99+
writeToPipe(REPORT_SUPPR, suppressionToString(suppr));
100100
}
101-
// TODO: update suppression states?
102101
}
103102

104103
void writeEnd(const std::string& str) const {
@@ -179,6 +178,7 @@ bool ProcessExecutor::handleRead(int rpipe, unsigned int &result, const std::str
179178
if (type != PipeWriter::REPORT_OUT &&
180179
type != PipeWriter::REPORT_ERROR &&
181180
type != PipeWriter::REPORT_SUPPR_INLINE &&
181+
type != PipeWriter::REPORT_SUPPR &&
182182
type != PipeWriter::CHILD_END) {
183183
std::cerr << "#### ThreadExecutor::handleRead(" << filename << ") invalid type " << int(type) << std::endl;
184184
std::exit(EXIT_FAILURE);
@@ -230,7 +230,7 @@ bool ProcessExecutor::handleRead(int rpipe, unsigned int &result, const std::str
230230

231231
if (hasToLog(msg))
232232
mErrorLogger.reportErr(msg);
233-
} else if (type == PipeWriter::REPORT_SUPPR_INLINE) {
233+
} else if (type == PipeWriter::REPORT_SUPPR_INLINE || type == PipeWriter::REPORT_SUPPR) {
234234
if (!buf.empty()) {
235235
// TODO: avoid string splitting
236236
auto parts = splitString(buf, ';');
@@ -241,7 +241,7 @@ bool ProcessExecutor::handleRead(int rpipe, unsigned int &result, const std::str
241241
std::exit(EXIT_FAILURE);
242242
}
243243
auto suppr = SuppressionList::parseLine(parts[0]);
244-
suppr.isInline = true;
244+
suppr.isInline = (type == PipeWriter::REPORT_SUPPR_INLINE);
245245
suppr.checked = parts[1] == "1";
246246
suppr.matched = parts[2] == "1";
247247
const std::string err = mSuppressions.nomsg.addSuppression(suppr);

‎gui/checkthread.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -443,8 +443,8 @@ void CheckThread::parseClangErrors(const QString &tool, const QString &file0, QS
443443

444444
bool CheckThread::isSuppressed(const SuppressionList::ErrorMessage &errorMessage) const
445445
{
446-
return std::any_of(mSuppressionsUi.cbegin(), mSuppressionsUi.cend(), [&](const SuppressionList::Suppression& s) {
447-
return s.isSuppressed(errorMessage);
446+
return std::any_of(mSuppressionsUi.cbegin(), mSuppressionsUi.cend(), [&](const SuppressionList::Suppression& s) -> bool {
447+
return s.isSuppressed(errorMessage) == SuppressionList::Suppression::Result::Matched;
448448
});
449449
}
450450

‎lib/cppcheck.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,14 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file)
765765

766766
unsigned int CppCheck::check(const FileWithDetails &file)
767767
{
768+
// TODO: handle differently?
769+
// dummy call to make sure wildcards are being flagged as checked in case isSuppressed() is never called
770+
{
771+
// the empty ID is intentional for now - although it should not be allowed
772+
ErrorMessage msg({}, file.spath(), Severity::information, "", "", Certainty::normal);
773+
(void)mSuppressions.nomsg.isSuppressed(SuppressionList::ErrorMessage::fromErrorMessage(msg, {}), true);
774+
}
775+
768776
if (mSettings.clang)
769777
return checkClang(file);
770778

‎lib/suppressions.cpp‎

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,13 @@ bool SuppressionList::updateSuppressionState(const SuppressionList::Suppression&
311311
auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(),
312312
std::bind(&Suppression::isSameParameters, &suppression, std::placeholders::_1));
313313
if (foundSuppression != mSuppressions.end()) {
314-
// Update matched state of existing global suppression
315-
if (!suppression.isLocal() && suppression.matched)
316-
foundSuppression->matched = suppression.matched;
314+
// Update state of existing global suppression
315+
if (!suppression.isLocal()) {
316+
if (suppression.checked)
317+
foundSuppression->checked = true;
318+
if (suppression.matched)
319+
foundSuppression->matched = true;
320+
}
317321
return true;
318322
}
319323

@@ -373,26 +377,32 @@ bool SuppressionList::Suppression::parseComment(std::string comment, std::string
373377
return true;
374378
}
375379

376-
bool SuppressionList::Suppression::isSuppressed(const SuppressionList::ErrorMessage &errmsg) const
380+
SuppressionList::Suppression::Result SuppressionList::Suppression::isSuppressed(const SuppressionList::ErrorMessage &errmsg) const
377381
{
378-
if (hash > 0 && hash != errmsg.hash)
379-
return false;
380-
if (!errorId.empty() && !matchglob(errorId, errmsg.errorId))
381-
return false;
382382
if (type == SuppressionList::Type::macro) {
383383
if (errmsg.macroNames.count(macroName) == 0)
384-
return false;
384+
return Result::None;
385+
if (hash > 0 && hash != errmsg.hash)
386+
return Result::Checked;
387+
if (!errorId.empty() && !matchglob(errorId, errmsg.errorId))
388+
return Result::Checked;
385389
} else {
386390
if (!fileName.empty() && !matchglob(fileName, errmsg.getFileName()))
387-
return false;
391+
return Result::None;
388392
if ((SuppressionList::Type::unique == type) && (lineNumber != NO_LINE) && (lineNumber != errmsg.lineNumber)) {
389393
if (!thisAndNextLine || lineNumber + 1 != errmsg.lineNumber)
390-
return false;
394+
return Result::None;
391395
}
396+
if (hash > 0 && hash != errmsg.hash)
397+
return Result::Checked;
398+
// the empty check is a hack to allow wildcard suppressions on IDs to be marked as checked
399+
if (!errorId.empty() && (errmsg.errorId.empty() || !matchglob(errorId, errmsg.errorId)))
400+
return Result::Checked;
392401
if ((SuppressionList::Type::block == type) && ((errmsg.lineNumber < lineBegin) || (errmsg.lineNumber > lineEnd)))
393-
return false;
402+
return Result::Checked;
394403
}
395404
if (!symbolName.empty()) {
405+
bool matchedSymbol = false;
396406
for (std::string::size_type pos = 0; pos < errmsg.symbolNames.size();) {
397407
const std::string::size_type pos2 = errmsg.symbolNames.find('\n',pos);
398408
std::string symname;
@@ -403,21 +413,31 @@ bool SuppressionList::Suppression::isSuppressed(const SuppressionList::ErrorMess
403413
symname = errmsg.symbolNames.substr(pos,pos2-pos);
404414
pos = pos2+1;
405415
}
406-
if (matchglob(symbolName, symname))
407-
return true;
416+
if (matchglob(symbolName, symname)) {
417+
matchedSymbol = true;
418+
break;
419+
}
408420
}
409-
return false;
421+
if (!matchedSymbol)
422+
return Result::Checked;
410423
}
411-
return true;
424+
return Result::Matched;
412425
}
413426

414427
bool SuppressionList::Suppression::isMatch(const SuppressionList::ErrorMessage &errmsg)
415428
{
416-
if (!isSuppressed(errmsg))
429+
switch (isSuppressed(errmsg)) {
430+
case Result::None:
417431
return false;
418-
matched = true;
419-
checked = true;
420-
return true;
432+
case Result::Checked:
433+
checked = true;
434+
return false;
435+
case Result::Matched:
436+
checked = true;
437+
matched = true;
438+
return true;
439+
}
440+
cppcheck::unreachable();
421441
}
422442

423443
// cppcheck-suppress unusedFunction - used by GUI only
@@ -525,7 +545,9 @@ std::list<SuppressionList::Suppression> SuppressionList::getUnmatchedLocalSuppre
525545
for (const Suppression &s : mSuppressions) {
526546
if (s.isInline)
527547
continue;
528-
if (s.matched || ((s.lineNumber != Suppression::NO_LINE) && !s.checked))
548+
if (s.matched)
549+
continue;
550+
if ((s.lineNumber != Suppression::NO_LINE) && !s.checked)
529551
continue;
530552
if (s.type == SuppressionList::Type::macro)
531553
continue;
@@ -550,7 +572,9 @@ std::list<SuppressionList::Suppression> SuppressionList::getUnmatchedGlobalSuppr
550572
for (const Suppression &s : mSuppressions) {
551573
if (s.isInline)
552574
continue;
553-
if (s.matched || ((s.lineNumber != Suppression::NO_LINE) && !s.checked))
575+
if (s.matched)
576+
continue;
577+
if (!s.checked && s.isWildcard())
554578
continue;
555579
if (s.hash > 0)
556580
continue;
@@ -571,6 +595,7 @@ std::list<SuppressionList::Suppression> SuppressionList::getUnmatchedInlineSuppr
571595
for (const SuppressionList::Suppression &s : SuppressionList::mSuppressions) {
572596
if (!s.isInline)
573597
continue;
598+
// TODO: remove this and markUnmatchedInlineSuppressionsAsChecked()?
574599
if (!s.checked)
575600
continue;
576601
if (s.matched)

‎lib/suppressions.h‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,14 +118,24 @@ class CPPCHECKLIB SuppressionList {
118118
*/
119119
bool parseComment(std::string comment, std::string *errorMessage);
120120

121-
bool isSuppressed(const ErrorMessage &errmsg) const;
121+
enum class Result : std::uint8_t {
122+
None,
123+
Checked,
124+
Matched
125+
};
126+
127+
Result isSuppressed(const ErrorMessage &errmsg) const;
122128

123129
bool isMatch(const ErrorMessage &errmsg);
124130

125131
std::string getText() const;
126132

133+
bool isWildcard() const {
134+
return fileName.find_first_of("?*") != std::string::npos;
135+
}
136+
127137
bool isLocal() const {
128-
return !fileName.empty() && fileName.find_first_of("?*") == std::string::npos;
138+
return !fileName.empty() && !isWildcard();
129139
}
130140

131141
bool isSameParameters(const Suppression &other) const {
@@ -149,8 +159,8 @@ class CPPCHECKLIB SuppressionList {
149159
std::string macroName;
150160
std::size_t hash{};
151161
bool thisAndNextLine{}; // Special case for backwards compatibility: { // cppcheck-suppress something
152-
bool matched{};
153-
bool checked{}; // for inline suppressions, checked or not
162+
bool matched{}; /** This suppression was fully matched in an isSuppressed() call */
163+
bool checked{}; /** This suppression applied to code which was being analyzed but did not match the error in an isSuppressed() call */
154164
bool isInline{};
155165

156166
enum : std::int8_t { NO_LINE = -1 };

‎test/cli/inline-suppress_test.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,4 +437,23 @@ def test_unused_function_disabled_unmatched():
437437
'{}unusedFunctionUnmatched.cpp:5:0: information: Unmatched suppression: uninitvar [unmatchedSuppression]'.format(__proj_inline_suppres_path)
438438
]
439439
assert stdout == ''
440+
assert ret == 0, stdout
441+
442+
443+
def test_unmatched_cfg():
444+
# make sure we do not report unmatched inline suppressions from inactive code blocks
445+
args = [
446+
'-q',
447+
'--template=simple',
448+
'--enable=warning,information',
449+
'--inline-suppr',
450+
'proj-inline-suppress/cfg.c'
451+
]
452+
453+
ret, stdout, stderr = cppcheck(args, cwd=__script_dir)
454+
assert stderr.splitlines() == [
455+
'{}cfg.c:10:0: information: Unmatched suppression: id [unmatchedSuppression]'.format(__proj_inline_suppres_path),
456+
'{}cfg.c:14:0: information: Unmatched suppression: id [unmatchedSuppression]'.format(__proj_inline_suppres_path),
457+
]
458+
assert stdout == ''
440459
assert ret == 0, stdout

‎test/cli/other_test.py‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3417,4 +3417,69 @@ def test_clang_tidy(tmpdir): # #12053
34173417

34183418
@pytest.mark.skipif(not has_clang_tidy, reason='clang-tidy is not available')
34193419
def test_clang_tidy_project(tmpdir):
3420-
__test_clang_tidy(tmpdir, True)
3420+
__test_clang_tidy(tmpdir, True)
3421+
3422+
3423+
def test_suppress_unmatched_wildcard(tmp_path): # #13660
3424+
test_file = tmp_path / 'test.c'
3425+
with open(test_file, 'wt') as f:
3426+
f.write(
3427+
"""void f()
3428+
{
3429+
(void)(*((int*)0));
3430+
}
3431+
""")
3432+
3433+
# need to run in the temporary folder because the path of the suppression has to match
3434+
args = [
3435+
'-q',
3436+
'--template=simple',
3437+
'--enable=information',
3438+
'--suppress=nullPointer:test*.c', # checked and matched
3439+
'--suppress=id:test*.c', # checked and unmatched
3440+
'--suppress=id2:test*.c', # checked and unmatched
3441+
'--suppress=id2:tes?.c', # checked and unmatched
3442+
'--suppress=*:test*.c', # checked and unmatched
3443+
'--suppress=id:test*.cpp', # unchecked
3444+
'--suppress=id2:test?.cpp', # unchecked
3445+
'test.c'
3446+
]
3447+
exitcode, stdout, stderr = cppcheck(args, cwd=tmp_path)
3448+
assert exitcode == 0, stdout
3449+
assert stdout.splitlines() == []
3450+
# TODO: invalid locations - see #13659
3451+
assert stderr.splitlines() == [
3452+
'test*.c:-1:0: information: Unmatched suppression: id [unmatchedSuppression]',
3453+
'test*.c:-1:0: information: Unmatched suppression: id2 [unmatchedSuppression]',
3454+
'tes?.c:-1:0: information: Unmatched suppression: id2 [unmatchedSuppression]'
3455+
]
3456+
3457+
3458+
def test_suppress_unmatched_wildcard_unchecked(tmp_path):
3459+
# make sure that unmatched wildcards suppressions are reported if files matching the expressions were processesd
3460+
# but isSuppressed() has never been called (i.e. no findings in file at all)
3461+
test_file = tmp_path / 'test.c'
3462+
with open(test_file, 'wt') as f:
3463+
f.write("""void f() {}""")
3464+
3465+
# need to run in the temporary folder because the path of the suppression has to match
3466+
args = [
3467+
'-q',
3468+
'--template=simple',
3469+
'--enable=information',
3470+
'--suppress=id:test*.c',
3471+
'--suppress=id:tes?.c',
3472+
'--suppress=id2:*',
3473+
'--suppress=*:test*.c',
3474+
'test.c'
3475+
]
3476+
exitcode, stdout, stderr = cppcheck(args, cwd=tmp_path)
3477+
assert exitcode == 0, stdout
3478+
assert stdout.splitlines() == []
3479+
# TODO: invalid locations - see #13659
3480+
assert stderr.splitlines() == [
3481+
'test*.c:-1:0: information: Unmatched suppression: id [unmatchedSuppression]',
3482+
'tes?.c:-1:0: information: Unmatched suppression: id [unmatchedSuppression]',
3483+
'*:-1:0: information: Unmatched suppression: id2 [unmatchedSuppression]',
3484+
'test*.c:-1:0: information: Unmatched suppression: * [unmatchedSuppression]'
3485+
]

‎test/cli/proj-inline-suppress/cfg.c‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
void f()
2+
{
3+
#if VER_CHECK(3, 1, 6)
4+
// cppcheck-suppress id
5+
(void)0;
6+
#endif
7+
8+
#if DEF_1
9+
// cppcheck-suppress id
10+
(void)0;
11+
#endif
12+
13+
// cppcheck-suppress id
14+
(void)0;
15+
}

0 commit comments

Comments
 (0)