Skip to content

Commit 107e226

Browse files
firewavefirewave
authored
fixed #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty (#7647)
1 parent 5c818e3 commit 107e226

File tree

9 files changed

+208
-151
lines changed

9 files changed

+208
-151
lines changed

‎Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -753,7 +753,7 @@ test/testcondition.o: test/testcondition.cpp lib/addoninfo.h lib/check.h lib/che
753753
test/testconstructors.o: test/testconstructors.cpp lib/addoninfo.h lib/check.h lib/checkclass.h lib/checkers.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h
754754
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testconstructors.cpp
755755

756-
test/testcppcheck.o: test/testcppcheck.cpp externals/simplecpp/simplecpp.h lib/addoninfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h
756+
test/testcppcheck.o: test/testcppcheck.cpp externals/simplecpp/simplecpp.h lib/addoninfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h test/redirect.h
757757
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testcppcheck.cpp
758758

759759
test/testerrorlogger.o: test/testerrorlogger.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/xml.h test/fixture.h test/helpers.h

‎lib/cppcheck.cpp

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,9 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
433433
const std::string &premiumArgs,
434434
const CppCheck::ExecuteCmdFn &executeCommand)
435435
{
436+
if (!executeCommand)
437+
throw InternalError(nullptr, "Failed to execute addon - no command callback provided");
438+
436439
std::string pythonExe;
437440

438441
if (!addonInfo.executable.empty())
@@ -442,7 +445,7 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
442445
else if (!defaultPythonExe.empty())
443446
pythonExe = cmdFileName(defaultPythonExe);
444447
else {
445-
// store in static variable so we only look this up once
448+
// store in static variable so we only look this up once - TODO: do not cache globally
446449
static const std::string detectedPythonExe = detectPython(executeCommand);
447450
if (detectedPythonExe.empty())
448451
throw InternalError(nullptr, "Failed to auto detect python");
@@ -687,6 +690,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex)
687690
mErrorLogger.reportOut(exe + " " + args2, Color::Reset);
688691
}
689692

693+
if (!mExecuteCommand) {
694+
std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl;
695+
return 0; // TODO: report as failure?
696+
}
697+
690698
std::string output2;
691699
const int exitcode = mExecuteCommand(exe,split(args2),redirect2,output2);
692700
if (mSettings.debugClangOutput) {
@@ -1956,6 +1964,11 @@ void CppCheck::analyseClangTidy(const FileSettings &fileSettings)
19561964
}
19571965
#endif
19581966

1967+
if (!mExecuteCommand) {
1968+
std::cerr << "Failed to execute '" << exe << "' (no command callback provided)" << std::endl;
1969+
return;
1970+
}
1971+
19591972
// TODO: log this call
19601973
// TODO: get rid of hard-coded checks
19611974
const std::string args = "-quiet -checks=*,-clang-analyzer-*,-llvm* \"" + fileSettings.filename() + "\" -- " + allIncludes + allDefines;

‎lib/cppcheck.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ class CPPCHECKLIB CppCheck {
6060
friend class TestCppcheck;
6161

6262
public:
63+
// exe, args, redirect, output
6364
using ExecuteCmdFn = std::function<int (std::string,std::vector<std::string>,std::string,std::string&)>;
6465

6566
/**

‎test/fixture.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,7 @@ class TestInstance {
322322
#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (const AssertFailedError&) {} catch (...) { assertNoThrowFail(__FILE__, __LINE__, false); } } } while (false)
323323

324324
#define ASSERT( CONDITION ) assert_(__FILE__, __LINE__, (CONDITION))
325+
#define ASSERT_MSG( CONDITION, MSG ) assert_(__FILE__, __LINE__, (CONDITION), MSG)
325326
#define ASSERT_LOC( CONDITION, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION))
326327
#define ASSERT_LOC_MSG( CONDITION, MSG, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION), MSG)
327328
// *INDENT-OFF*

‎test/redirect.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ class RedirectOutputError {
4343
std::cerr.rdbuf(_err.rdbuf()); // assign streambuf to cerr
4444
}
4545

46+
// TODO: if an assert failed previously this will mask that failure
4647
/** Revert cout and cerr behaviour */
4748
~RedirectOutputError() noexcept(false) {
4849
std::cout.rdbuf(_oldCout); // restore cout's original streambuf

‎test/testcppcheck.cpp

Lines changed: 187 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,19 @@
2525
#include "helpers.h"
2626
#include "path.h"
2727
#include "preprocessor.h"
28+
#include "redirect.h"
2829
#include "settings.h"
2930
#include "standards.h"
3031
#include "suppressions.h"
3132

3233
#include <algorithm>
3334
#include <cstddef>
35+
#include <cstdlib>
3436
#include <list>
3537
#include <sstream>
3638
#include <string>
39+
#include <unordered_set>
40+
#include <utility>
3741
#include <vector>
3842

3943
#include <simplecpp.h>
@@ -66,7 +70,11 @@ class TestCppcheck : public TestFixture {
6670
void run() override {
6771
TEST_CASE(getErrorMessages);
6872
TEST_CASE(checkWithFile);
73+
TEST_CASE(checkWithFileWithTools);
74+
TEST_CASE(checkWithFileWithToolsNoCommand);
6975
TEST_CASE(checkWithFS);
76+
TEST_CASE(checkWithFSWithTools);
77+
TEST_CASE(checkWithFSWithToolsNoCommand);
7078
TEST_CASE(suppress_error_library);
7179
TEST_CASE(unique_errors);
7280
TEST_CASE(unique_errors_2);
@@ -114,49 +122,218 @@ class TestCppcheck : public TestFixture {
114122
ASSERT(foundMissingIncludeSystem);
115123
}
116124

117-
void checkWithFile() const
125+
static std::string exename_(const std::string& exe)
118126
{
127+
#ifdef _WIN32
128+
return exe + ".exe";
129+
#else
130+
return exe;
131+
#endif
132+
}
133+
134+
CppCheck::ExecuteCmdFn getExecuteCommand(int& called) const
135+
{
136+
// cppcheck-suppress passedByValue - used as callback so we need to preserve the signature
137+
// NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
138+
return [&](std::string exe, std::vector<std::string> args, std::string redirect, std::string& /*output*/) -> int {
139+
++called;
140+
if (exe == exename_("clang-tidy"))
141+
{
142+
ASSERT_EQUALS(4, args.size());
143+
ASSERT_EQUALS("-quiet", args[0]);
144+
ASSERT_EQUALS("-checks=*,-clang-analyzer-*,-llvm*", args[1]);
145+
ASSERT_EQUALS("test.cpp", args[2]);
146+
ASSERT_EQUALS("--", args[3]);
147+
ASSERT_EQUALS("2>&1", redirect);
148+
return EXIT_SUCCESS;
149+
}
150+
if (exe == exename_("python3"))
151+
{
152+
ASSERT_EQUALS(1, args.size());
153+
ASSERT_EQUALS("--version", args[0]);
154+
ASSERT_EQUALS("2>&1", redirect);
155+
return EXIT_SUCCESS;
156+
}
157+
if (exe == exename_("python"))
158+
{
159+
ASSERT_EQUALS(1, args.size());
160+
ASSERT_EQUALS("--version", args[0]);
161+
ASSERT_EQUALS("2>&1", redirect);
162+
return EXIT_SUCCESS;
163+
}
164+
ASSERT_MSG(false, "unhandled exe: " + exe);
165+
return EXIT_FAILURE;
166+
};
167+
}
168+
169+
void checkWithFileInternal(bool tools, bool nocmd = false) const
170+
{
171+
REDIRECT;
119172
ScopedFile file("test.cpp",
120173
"int main()\n"
121174
"{\n"
122175
" int i = *((int*)0);\n"
123176
" return 0;\n"
124177
"}");
125178

126-
const auto s = dinit(Settings, $.templateFormat = templateFormat);
179+
int called = 0;
180+
std::unordered_set<std::string> addons;
181+
std::vector<AddonInfo> addonInfo;
182+
if (tools)
183+
{
184+
addons.emplace("testcppcheck");
185+
addonInfo.emplace_back(/*AddonInfo()*/);
186+
}
187+
const auto s = dinit(Settings,
188+
$.templateFormat = templateFormat,
189+
$.clangTidy = tools,
190+
$.addons = std::move (addons),
191+
$.addonInfos = std::move (addonInfo));
127192
Suppressions supprs;
128193
ErrorLogger2 errorLogger;
129-
CppCheck cppcheck(s, supprs, errorLogger, false, {});
194+
CppCheck::ExecuteCmdFn f;
195+
if (tools && !nocmd) {
196+
f = getExecuteCommand(called);
197+
}
198+
CppCheck cppcheck(s, supprs, errorLogger, false, f);
130199
ASSERT_EQUALS(1, cppcheck.check(FileWithDetails(file.path(), Path::identify(file.path(), false), 0)));
131200
// TODO: how to properly disable these warnings?
132201
errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
133202
return id == "logChecker";
134203
}), errorLogger.ids.end());
135-
ASSERT_EQUALS(1, errorLogger.ids.size());
136-
ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
204+
errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& msg) {
205+
return msg.id == "logChecker";
206+
}), errorLogger.errmsgs.end());
207+
if (tools)
208+
{
209+
ASSERT_EQUALS(2, errorLogger.ids.size());
210+
auto it = errorLogger.errmsgs.cbegin();
211+
ASSERT_EQUALS("nullPointer", it->id);
212+
++it;
213+
214+
if (nocmd)
215+
{
216+
ASSERT_EQUALS("internalError", it->id);
217+
ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to execute addon - no command callback provided", it->shortMessage()); // TODO: add addon name
218+
219+
// TODO: clang-tidy is currently not invoked for file inputs - see #12053
220+
// TODO: needs to become a proper error
221+
TODO_ASSERT_EQUALS("Failed to execute '" + exename_("clang-tidy") + "' (no command callback provided)\n", "", GET_REDIRECT_ERROUT);
222+
223+
ASSERT_EQUALS(0, called); // not called because we check if the callback exists
224+
}
225+
else
226+
{
227+
ASSERT_EQUALS("internalError", it->id);
228+
ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to auto detect python", it->shortMessage()); // TODO: clarify what python is used for
229+
230+
// TODO: we cannot check this because the python detection is cached globally so this result will different dependent on how the test is called
231+
//ASSERT_EQUALS(2, called);
232+
}
233+
}
234+
else
235+
{
236+
ASSERT_EQUALS(0, called);
237+
ASSERT_EQUALS(1, errorLogger.ids.size());
238+
ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
239+
}
240+
}
241+
242+
void checkWithFile() const {
243+
checkWithFileInternal(false);
137244
}
138245

139-
void checkWithFS() const
246+
void checkWithFileWithTools() const {
247+
checkWithFileInternal(true);
248+
}
249+
250+
void checkWithFileWithToolsNoCommand() const {
251+
checkWithFileInternal(true, true);
252+
}
253+
254+
void checkWithFSInternal(bool tools, bool nocmd = false) const
140255
{
256+
REDIRECT;
141257
ScopedFile file("test.cpp",
142258
"int main()\n"
143259
"{\n"
144260
" int i = *((int*)0);\n"
145261
" return 0;\n"
146262
"}");
147263

148-
const auto s = dinit(Settings, $.templateFormat = templateFormat);
264+
int called = 0;
265+
std::unordered_set<std::string> addons;
266+
std::vector<AddonInfo> addonInfo;
267+
if (tools)
268+
{
269+
addons.emplace("testcppcheck");
270+
addonInfo.emplace_back(/*AddonInfo()*/);
271+
}
272+
const auto s = dinit(Settings,
273+
$.templateFormat = templateFormat,
274+
$.clangTidy = tools,
275+
$.addons = std::move (addons),
276+
$.addonInfos = std::move (addonInfo));
149277
Suppressions supprs;
150278
ErrorLogger2 errorLogger;
151-
CppCheck cppcheck(s, supprs, errorLogger, false, {});
279+
CppCheck::ExecuteCmdFn f;
280+
if (tools && !nocmd) {
281+
f = getExecuteCommand(called);
282+
}
283+
CppCheck cppcheck(s, supprs, errorLogger, false, f);
152284
FileSettings fs{file.path(), Path::identify(file.path(), false), 0};
153285
ASSERT_EQUALS(1, cppcheck.check(fs));
154286
// TODO: how to properly disable these warnings?
155287
errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
156288
return id == "logChecker";
157289
}), errorLogger.ids.end());
158-
ASSERT_EQUALS(1, errorLogger.ids.size());
159-
ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
290+
errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& msg) {
291+
return msg.id == "logChecker";
292+
}), errorLogger.errmsgs.end());
293+
if (tools)
294+
{
295+
ASSERT_EQUALS(2, errorLogger.ids.size());
296+
auto it = errorLogger.errmsgs.cbegin();
297+
ASSERT_EQUALS("nullPointer", it->id);
298+
++it;
299+
300+
if (nocmd)
301+
{
302+
ASSERT_EQUALS("internalError", it->id);
303+
ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to execute addon - no command callback provided", it->shortMessage()); // TODO: add addon name
304+
305+
// TODO: needs to become a proper error
306+
ASSERT_EQUALS("Failed to execute '" + exename_("clang-tidy") + "' (no command callback provided)\n", GET_REDIRECT_ERROUT);
307+
308+
ASSERT_EQUALS(0, called); // not called because we check if the callback exists
309+
}
310+
else
311+
{
312+
ASSERT_EQUALS("internalError", it->id);
313+
ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to auto detect python", it->shortMessage()); // TODO: clarify what python is used for
314+
315+
// TODO: we cannot check this because the python detection is cached globally so this result will different dependent on how the test is called
316+
//ASSERT_EQUALS(3, called);
317+
}
318+
}
319+
else
320+
{
321+
ASSERT_EQUALS(0, called);
322+
ASSERT_EQUALS(1, errorLogger.ids.size());
323+
ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
324+
}
325+
}
326+
327+
void checkWithFS() const {
328+
checkWithFSInternal(false);
329+
}
330+
331+
void checkWithFSWithTools() const {
332+
checkWithFSInternal(true);
333+
}
334+
335+
void checkWithFSWithToolsNoCommand() const {
336+
checkWithFSInternal(true, true);
160337
}
161338

162339
void suppress_error_library() const

0 commit comments

Comments
 (0)