fixed #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty

Makefile

@@ -753,7 +753,7 @@ test/testcondition.o: test/testcondition.cpp lib/addoninfo.h lib/check.h lib/che
 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
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testconstructors.cpp
 
-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/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
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testcppcheck.cpp
 
 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

@@ -432,6 +432,9 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
                                                  const std::string &premiumArgs,
                                                  const CppCheck::ExecuteCmdFn &executeCommand)
 {
+    if (!executeCommand)
+        throw InternalError(nullptr, "Failed to execute addon - no command callback provided");
+
     std::string pythonExe;
 
     if (!addonInfo.executable.empty())
@@ -441,7 +444,7 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
     else if (!defaultPythonExe.empty())
         pythonExe = cmdFileName(defaultPythonExe);
     else {
-        // store in static variable so we only look this up once
+        // store in static variable so we only look this up once - TODO: do not cache globally
         static const std::string detectedPythonExe = detectPython(executeCommand);
         if (detectedPythonExe.empty())
             throw InternalError(nullptr, "Failed to auto detect python");
@@ -686,6 +689,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex)
         mErrorLogger.reportOut(exe + " " + args2, Color::Reset);
     }
 
+    if (!mExecuteCommand) {
+        std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl;
+        return 0; // TODO: report as failure?
+    }
+
     std::string output2;
     const int exitcode = mExecuteCommand(exe,split(args2),redirect2,output2);
     if (mSettings.debugClangOutput) {
@@ -1954,6 +1962,11 @@ void CppCheck::analyseClangTidy(const FileSettings &fileSettings)
     }
 #endif
 
+    if (!mExecuteCommand) {
+        std::cerr << "Failed to execute '" << exe << "' (no command callback provided)" << std::endl;
+        return;
+    }
+
     // TODO: log this call
     // TODO: get rid of hard-coded checks
     const std::string args = "-quiet -checks=*,-clang-analyzer-*,-llvm* \"" + fileSettings.filename() + "\" -- " + allIncludes + allDefines;

lib/cppcheck.h

@@ -60,6 +60,7 @@ class CPPCHECKLIB CppCheck {
     friend class TestCppcheck;
 
 public:
+    // exe, args, redirect, output
     using ExecuteCmdFn = std::function<int (std::string,std::vector<std::string>,std::string,std::string&)>;
 
     /**

test/fixture.h

@@ -320,6 +320,7 @@ class TestInstance {
 #define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (const AssertFailedError&) {} catch (...) { assertNoThrowFail(__FILE__, __LINE__, false); } } } while (false)
 
 #define ASSERT( CONDITION ) assert_(__FILE__, __LINE__, (CONDITION))
+#define ASSERT_MSG( CONDITION, MSG ) assert_(__FILE__, __LINE__, (CONDITION), MSG)
 #define ASSERT_LOC( CONDITION, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION))
 #define ASSERT_LOC_MSG( CONDITION, MSG, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION), MSG)
 // *INDENT-OFF*

test/redirect.h

@@ -43,6 +43,7 @@ class RedirectOutputError {
         std::cerr.rdbuf(_err.rdbuf()); // assign streambuf to cerr
     }
 
+    // TODO: if an assert failed previously this will mask that failure
     /** Revert cout and cerr behaviour */
     ~RedirectOutputError() noexcept(false) {
         std::cout.rdbuf(_oldCout); // restore cout's original streambuf

test/testcppcheck.cpp

@@ -25,15 +25,19 @@
 #include "helpers.h"
 #include "path.h"
 #include "preprocessor.h"
+#include "redirect.h"
 #include "settings.h"
 #include "standards.h"
 #include "suppressions.h"
 
 #include <algorithm>
 #include <cstddef>
+#include <cstdlib>
 #include <list>
 #include <sstream>
 #include <string>
+#include <unordered_set>
+#include <utility>
 #include <vector>
 
 #include <simplecpp.h>
@@ -66,7 +70,11 @@ class TestCppcheck : public TestFixture {
     void run() override {
         TEST_CASE(getErrorMessages);
         TEST_CASE(checkWithFile);
+        TEST_CASE(checkWithFileWithTools);
+        TEST_CASE(checkWithFileWithToolsNoCommand);
         TEST_CASE(checkWithFS);
+        TEST_CASE(checkWithFSWithTools);
+        TEST_CASE(checkWithFSWithToolsNoCommand);
         TEST_CASE(suppress_error_library);
         TEST_CASE(unique_errors);
         TEST_CASE(unique_errors_2);
@@ -114,49 +122,218 @@ class TestCppcheck : public TestFixture {
         ASSERT(foundMissingIncludeSystem);
     }
 
-    void checkWithFile() const
+    static std::string exename_(const std::string& exe)
     {
+#ifdef _WIN32
+        return exe + ".exe";
+#else
+        return exe;
+#endif
+    }
+
+    CppCheck::ExecuteCmdFn getExecuteCommand(int& called) const
+    {
+        // cppcheck-suppress passedByValue - used as callback so we need to preserve the signature
+        // NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
+        return [&](std::string exe, std::vector<std::string> args, std::string redirect, std::string& /*output*/) -> int {
+            ++called;
+            if (exe == exename_("clang-tidy"))
+            {
+                ASSERT_EQUALS(4, args.size());
+                ASSERT_EQUALS("-quiet", args[0]);
+                ASSERT_EQUALS("-checks=*,-clang-analyzer-*,-llvm*", args[1]);
+                ASSERT_EQUALS("test.cpp", args[2]);
+                ASSERT_EQUALS("--", args[3]);
+                ASSERT_EQUALS("2>&1", redirect);
+                return EXIT_SUCCESS;
+            }
+            if (exe == exename_("python3"))
+            {
+                ASSERT_EQUALS(1, args.size());
+                ASSERT_EQUALS("--version", args[0]);
+                ASSERT_EQUALS("2>&1", redirect);
+                return EXIT_SUCCESS;
+            }
+            if (exe == exename_("python"))
+            {
+                ASSERT_EQUALS(1, args.size());
+                ASSERT_EQUALS("--version", args[0]);
+                ASSERT_EQUALS("2>&1", redirect);
+                return EXIT_SUCCESS;
+            }
+            ASSERT_MSG(false, "unhandled exe: " + exe);
+            return EXIT_FAILURE;
+        };
+    }
+
+    void checkWithFileInternal(bool tools, bool nocmd = false) const
+    {
+        REDIRECT;
         ScopedFile file("test.cpp",
                         "int main()\n"
                         "{\n"
                         "  int i = *((int*)0);\n"
                         "  return 0;\n"
                         "}");
 
-        const auto s = dinit(Settings, $.templateFormat = templateFormat);
+        int called = 0;
+        std::unordered_set<std::string> addons;
+        std::vector<AddonInfo> addonInfo;
+        if (tools)
+        {
+            addons.emplace("testcppcheck");
+            addonInfo.emplace_back(/*AddonInfo()*/);
+        }
+        const auto s = dinit(Settings,
+                             $.templateFormat = templateFormat,
+                                 $.clangTidy = tools,
+                                 $.addons = std::move (addons),
+                                 $.addonInfos = std::move (addonInfo));
         Suppressions supprs;
         ErrorLogger2 errorLogger;
-        CppCheck cppcheck(s, supprs, errorLogger, false, {});
+        CppCheck::ExecuteCmdFn f;
+        if (tools && !nocmd) {
+            f = getExecuteCommand(called);
+        }
+        CppCheck cppcheck(s, supprs, errorLogger, false, f);
         ASSERT_EQUALS(1, cppcheck.check(FileWithDetails(file.path(), Path::identify(file.path(), false), 0)));
         // TODO: how to properly disable these warnings?
         errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
             return id == "logChecker";
         }), errorLogger.ids.end());
-        ASSERT_EQUALS(1, errorLogger.ids.size());
-        ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+        errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& msg) {
+            return msg.id == "logChecker";
+        }), errorLogger.errmsgs.end());
+        if (tools)
+        {
+            ASSERT_EQUALS(2, errorLogger.ids.size());
+            auto it = errorLogger.errmsgs.cbegin();
+            ASSERT_EQUALS("nullPointer", it->id);
+            ++it;
+
+            if (nocmd)
+            {
+                ASSERT_EQUALS("internalError", it->id);
+                ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to execute addon - no command callback provided", it->shortMessage()); // TODO: add addon name
+
+                // TODO: clang-tidy is currently not invoked for file inputs - see #12053
+                // TODO: needs to become a proper error
+                TODO_ASSERT_EQUALS("Failed to execute '" + exename_("clang-tidy") + "' (no command callback provided)\n", "", GET_REDIRECT_ERROUT);
+
+                ASSERT_EQUALS(0, called); // not called because we check if the callback exists
+            }
+            else
+            {
+                ASSERT_EQUALS("internalError", it->id);
+                ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to auto detect python", it->shortMessage()); // TODO: clarify what python is used for
+
+                // TODO: we cannot check this because the python detection is cached globally so this result will different dependent on how the test is called
+                //ASSERT_EQUALS(2, called);
+            }
+        }
+        else
+        {
+            ASSERT_EQUALS(0, called);
+            ASSERT_EQUALS(1, errorLogger.ids.size());
+            ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+        }
+    }
+
+    void checkWithFile() const {
+        checkWithFileInternal(false);
     }
 
-    void checkWithFS() const
+    void checkWithFileWithTools() const {
+        checkWithFileInternal(true);
+    }
+
+    void checkWithFileWithToolsNoCommand() const {
+        checkWithFileInternal(true, true);
+    }
+
+    void checkWithFSInternal(bool tools, bool nocmd = false) const
     {
+        REDIRECT;
         ScopedFile file("test.cpp",
                         "int main()\n"
                         "{\n"
                         "  int i = *((int*)0);\n"
                         "  return 0;\n"
                         "}");
 
-        const auto s = dinit(Settings, $.templateFormat = templateFormat);
+        int called = 0;
+        std::unordered_set<std::string> addons;
+        std::vector<AddonInfo> addonInfo;
+        if (tools)
+        {
+            addons.emplace("testcppcheck");
+            addonInfo.emplace_back(/*AddonInfo()*/);
+        }
+        const auto s = dinit(Settings,
+                             $.templateFormat = templateFormat,
+                                 $.clangTidy = tools,
+                                 $.addons = std::move (addons),
+                                 $.addonInfos = std::move (addonInfo));
         Suppressions supprs;
         ErrorLogger2 errorLogger;
-        CppCheck cppcheck(s, supprs, errorLogger, false, {});
+        CppCheck::ExecuteCmdFn f;
+        if (tools && !nocmd) {
+            f = getExecuteCommand(called);
+        }
+        CppCheck cppcheck(s, supprs, errorLogger, false, f);
         FileSettings fs{file.path(), Path::identify(file.path(), false), 0};
         ASSERT_EQUALS(1, cppcheck.check(fs));
         // TODO: how to properly disable these warnings?
         errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
             return id == "logChecker";
         }), errorLogger.ids.end());
-        ASSERT_EQUALS(1, errorLogger.ids.size());
-        ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+        errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& msg) {
+            return msg.id == "logChecker";
+        }), errorLogger.errmsgs.end());
+        if (tools)
+        {
+            ASSERT_EQUALS(2, errorLogger.ids.size());
+            auto it = errorLogger.errmsgs.cbegin();
+            ASSERT_EQUALS("nullPointer", it->id);
+            ++it;
+
+            if (nocmd)
+            {
+                ASSERT_EQUALS("internalError", it->id);
+                ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to execute addon - no command callback provided", it->shortMessage()); // TODO: add addon name
+
+                // TODO: needs to become a proper error
+                ASSERT_EQUALS("Failed to execute '" + exename_("clang-tidy") + "' (no command callback provided)\n", GET_REDIRECT_ERROUT);
+
+                ASSERT_EQUALS(0, called); // not called because we check if the callback exists
+            }
+            else
+            {
+                ASSERT_EQUALS("internalError", it->id);
+                ASSERT_EQUALS("Bailing out from analysis: Checking file failed: Failed to auto detect python", it->shortMessage()); // TODO: clarify what python is used for
+
+                // TODO: we cannot check this because the python detection is cached globally so this result will different dependent on how the test is called
+                //ASSERT_EQUALS(3, called);
+            }
+        }
+        else
+        {
+            ASSERT_EQUALS(0, called);
+            ASSERT_EQUALS(1, errorLogger.ids.size());
+            ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+        }
+    }
+
+    void checkWithFS() const {
+        checkWithFSInternal(false);
+    }
+
+    void checkWithFSWithTools() const {
+        checkWithFSInternal(true);
+    }
+
+    void checkWithFSWithToolsNoCommand() const {
+        checkWithFSInternal(true, true);
     }
 
     void suppress_error_library() const