report `internalError` when command execution returns errorcode / also some related cleanups and tests (#5037)

Encountered while investigating https://trac.cppcheck.net/ticket/11708.

This has been like this since the introduction of `internalError` in
b6bcdf2936 (almost ten years ago to the
day). Logging internal errors which bail out(!) of the analysis simply
to `std::cout` for them possibly never to be seen (and also not affected
the exitcode) is pretty bad IMO. They should always be visible.

I also removed the filename from the message as it is already available
(and thus redundant) and its existence should be defined by the
template.

cli/cppcheckexecutor.cpp

@@ -692,6 +692,12 @@ bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string>
 {
     output_.clear();
 
+#ifdef _WIN32
+    // Extra quoutes are needed in windows if filename has space
+    if (exe.find(" ") != std::string::npos)
+        exe = "\"" + exe + "\"";
+#endif
+
     std::string joinedArgs;
     for (const std::string &arg : args) {
         if (!joinedArgs.empty())
@@ -702,21 +708,34 @@ bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string>
             joinedArgs += arg;
     }
 
+    const std::string cmd = exe + " " + joinedArgs + " " + redirect;
+
 #ifdef _WIN32
-    // Extra quoutes are needed in windows if filename has space
+    FILE* p = _popen(cmd.c_str(), "r");
-    if (exe.find(" ") != std::string::npos)
-        exe = "\"" + exe + "\"";
-    const std::string cmd = exe + " " + joinedArgs + " " + redirect;
-    std::unique_ptr<FILE, decltype(&_pclose)> pipe(_popen(cmd.c_str(), "r"), _pclose);
 #else
-    const std::string cmd = exe + " " + joinedArgs + " " + redirect;
+    FILE *p = popen(cmd.c_str(), "r");
-    std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(cmd.c_str(), "r"), pclose);
 #endif
-    if (!pipe)
+    if (!p) {
+        // TODO: read errno
         return false;
+    }
     char buffer[1024];
-    while (fgets(buffer, sizeof(buffer), pipe.get()) != nullptr)
+    while (fgets(buffer, sizeof(buffer), p) != nullptr)
         output_ += buffer;
+
+#ifdef _WIN32
+    const int res = _pclose(p);
+#else
+    const int res = pclose(p);
+#endif
+    if (res == -1) { // error occured
+        // TODO: read errno
+        return false;
+    }
+    if (res != 0) { // process failed
+        // TODO: need to get error details
+        return false;
+    }
     return true;
 }
 

lib/cppcheck.cpp

@@ -352,6 +352,14 @@ static std::string executeAddon(const AddonInfo &addonInfo,
 #endif
         for (const char* py_exe : py_exes) {
             std::string out;
+#ifdef _MSC_VER
+            // FIXME: hack to avoid debug assertion with _popen() in executeCommand() for non-existing commands
+            const std::string cmd = std::string(py_exe) + " --version >NUL";
+            if (system(cmd.c_str()) != 0) {
+                // TODO: get more detailed error?
+                break;
+            }
+#endif
             if (executeCommand(py_exe, split("--version"), redirect, out) && out.compare(0, 7, "Python ") == 0 && std::isdigit(out[7])) {
                 pythonExe = py_exe;
                 break;
@@ -373,7 +381,7 @@ static std::string executeAddon(const AddonInfo &addonInfo,
 
     std::string result;
     if (!executeCommand(pythonExe, split(args), redirect, result)) {
-        std::string message("Failed to execute addon (command: '" + pythonExe + " " + args + "'). Exitcode is nonzero.");
+        std::string message("Failed to execute addon '" + addonInfo.name + "' (command: '" + pythonExe + " " + args + "'). Exitcode is nonzero.");
         if (result.size() > 2) {
             message = message + "\n" + message + "\nOutput:\n" + result;
             message.resize(message.find_last_not_of("\n\r"));
@@ -588,13 +596,12 @@ unsigned int CppCheck::check(const std::string &path)
             executeAddons(dumpFile);
 
         } catch (const InternalError &e) {
-            internalError(path, e.errorMessage);
+            internalError(path, "Processing Clang AST dump failed: " + e.errorMessage);
-            mExitCode = 1; // e.g. reflect a syntax error
         } catch (const TerminateException &) {
             // Analysis is terminated
             return mExitCode;
         } catch (const std::exception &e) {
-            internalError(path, e.what());
+            internalError(path, std::string("Processing Clang AST dump failed: ") + e.what());
         }
 
         return mExitCode;
@@ -975,22 +982,7 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string
                 return mExitCode;
 
             } catch (const InternalError &e) {
-                std::list<ErrorMessage::FileLocation> locationList;
+                ErrorMessage errmsg = ErrorMessage::fromInternalError(e, &tokenizer.list, filename);
-                if (e.token) {
-                    locationList.emplace_back(e.token, &tokenizer.list);
-                } else {
-                    locationList.emplace_back(filename, 0, 0);
-                    if (filename != tokenizer.list.getSourceFilePath()) {
-                        locationList.emplace_back(tokenizer.list.getSourceFilePath(), 0, 0);
-                    }
-                }
-                ErrorMessage errmsg(std::move(locationList),
-                                    tokenizer.list.getSourceFilePath(),
-                                    Severity::error,
-                                    e.errorMessage,
-                                    e.id,
-                                    Certainty::normal);
-
                 reportErr(errmsg);
             }
         }
@@ -1026,12 +1018,11 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string
         // Analysis is terminated
         return mExitCode;
     } catch (const std::runtime_error &e) {
-        internalError(filename, e.what());
+        internalError(filename, std::string("Checking file failed: ") + e.what());
-    } catch (const std::bad_alloc &e) {
+    } catch (const std::bad_alloc &) {
-        internalError(filename, e.what());
+        internalError(filename, "Checking file failed: out of memory");
     } catch (const InternalError &e) {
-        internalError(filename, e.errorMessage);
+        internalError(filename, "Checking file failed: " + e.errorMessage);
-        mExitCode=1; // e.g. reflect a syntax error
     }
 
     if (!mSettings.buildDir.empty()) {
@@ -1050,9 +1041,10 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string
     return mExitCode;
 }
 
+// TODO: replace with ErrorMessage::fromInternalError()
 void CppCheck::internalError(const std::string &filename, const std::string &msg)
 {
-    const std::string fullmsg("Bailing out from checking since there was an internal error: " + msg);
+    const std::string fullmsg("Bailing out from analysis: " + msg);
 
     const ErrorMessage::FileLocation loc1(filename, 0, 0);
     std::list<ErrorMessage::FileLocation> callstack(1, loc1);
@@ -1525,8 +1517,7 @@ void CppCheck::executeAddonsWholeProgram(const std::map<std::string, std::size_t
     try {
         executeAddons(ctuInfoFiles);
     } catch (const InternalError& e) {
-        internalError("", "Internal error during whole program analysis: " + e.errorMessage);
+        internalError("", "Whole program analysis failed: " + e.errorMessage);
-        mExitCode = 1;
     }
 
     if (mSettings.buildDir.empty()) {

lib/errorlogger.cpp

@@ -230,6 +230,32 @@ static void serializeString(std::string &oss, const std::string & str)
     oss += str;
 }
 
+ErrorMessage ErrorMessage::fromInternalError(const InternalError &internalError, const TokenList *tokenList, const std::string &filename)
+{
+    if (internalError.token)
+        assert(tokenList != nullptr); // we need to make sure we can look up the provided token
+
+    std::list<ErrorMessage::FileLocation> locationList;
+    if (tokenList && internalError.token) {
+        ErrorMessage::FileLocation loc(internalError.token, tokenList);
+        locationList.push_back(std::move(loc));
+    } else {
+        ErrorMessage::FileLocation loc2(filename, 0, 0);
+        locationList.push_back(std::move(loc2));
+        if (tokenList && (filename != tokenList->getSourceFilePath())) {
+            ErrorMessage::FileLocation loc(tokenList->getSourceFilePath(), 0, 0);
+            locationList.push_back(std::move(loc));
+        }
+    }
+    ErrorMessage errmsg(std::move(locationList),
+                        tokenList ? tokenList->getSourceFilePath() : filename,
+                        Severity::error,
+                        internalError.errorMessage,
+                        internalError.id,
+                        Certainty::normal);
+    return errmsg;
+}
+
 std::string ErrorMessage::serialize() const
 {
     // Serialize this message into a simple string

lib/errorlogger.h

@@ -198,6 +198,8 @@ public:
         return mSymbolNames;
     }
 
+    static ErrorMessage fromInternalError(const InternalError &internalError, const TokenList *tokenList, const std::string &filename);
+
 private:
     static std::string fixInvalidChars(const std::string& raw);
 

releasenotes.txt

@@ -20,3 +20,4 @@ Deprecations:
 
 Other:
 - "USE_QT6=On" will no longer fallback to Qt5 when Qt6 is not found.
+- When the execution of an addon fails with an exitcode it will now result in an 'internalError' instead of being silently ignored.

test/cli/test-other.py

@@ -154,3 +154,56 @@ def test_slow_array_many_strings(tmpdir):
             f.write('    "abc",\n')
         f.write("};\n")
     cppcheck([filename]) # should not take more than ~1 second
+
+
+def test_execute_addon_failure(tmpdir):
+    test_file = os.path.join(tmpdir, 'test.cpp')
+    with open(test_file, 'wt') as f:
+        f.write("""
+                void f();
+                """)
+
+    args = ['--addon=naming', test_file]
+
+    # provide empty PATH environment variable so python is not found and execution of addon fails
+    env = {'PATH': ''}
+    _, _, stderr = cppcheck(args, env)
+    assert stderr == '{}:0:0: error: Bailing out from analysis: Checking file failed: Failed to auto detect python [internalError]\n\n^\n'.format(test_file)
+
+
+def test_execute_addon_failure_2(tmpdir):
+    test_file = os.path.join(tmpdir, 'test.cpp')
+    with open(test_file, 'wt') as f:
+        f.write("""
+                void f();
+                """)
+
+    # specify non-existent python executbale so execution of addon fails
+    args = ['--addon=naming', '--addon-python=python5.x', test_file]
+
+    _, _, stderr = cppcheck(args)
+    # /tmp/pytest-of-sshuser/pytest-215/test_execute_addon_failure_20/test.cpp:0:0: error: Bailing out from analysis: Checking file failed: Failed to execute addon 'naming' (command: 'python5.x /mnt/s/GitHub/cppcheck-fw/addons/runaddon.py /mnt/s/GitHub/cppcheck-fw/addons/naming.py --cli /tmp/pytest-of-sshuser/pytest-215/test_execute_addon_failure_20/test.cpp.7306.dump'). Exitcode is nonzero. [internalError]\n\n^\n
+    # "C:\\Users\\Quotenjugendlicher\\AppData\\Local\\Temp\\pytest-of-Quotenjugendlicher\\pytest-15\\test_execute_addon_failure_20\\test.cpp:0:0: error: Bailing out from analysis: Checking file failed: Failed to execute addon (command: 'python5.x S:\\GitHub\\cppcheck-fw\\bin\\debug\\addons\\runaddon.py S:\\GitHub\\cppcheck-fw\\bin\\debug\\addons\\naming.py --cli C:\\Users\\Quotenjugendlicher\\AppData\\Local\\Temp\\pytest-of-Quotenjugendlicher\\pytest-15\\test_execute_addon_failure_20\\test.cpp.9892.dump'). Exitcode is nonzero. [internalError]\n\n^\n
+    assert stderr.startswith('{}:0:0: error: Bailing out from analysis: Checking file failed: Failed to execute addon \'naming\' (command: \'python5.x '.format(test_file))
+    assert stderr.endswith(' [internalError]\n\n^\n')
+
+
+# TODO: find a test case which always fails
+@pytest.mark.skip
+def test_internal_error(tmpdir):
+    test_file = os.path.join(tmpdir, 'test.cpp')
+    with open(test_file, 'wt') as f:
+        f.write("""
+#include <cstdio>
+
+void f() {
+    double gc = 3333.3333;
+    char stat[80];
+    sprintf(stat,"'%2.1f'",gc);
+}
+                """)
+
+    args = [test_file]
+
+    _, _, stderr = cppcheck(args)
+    assert stderr == '{}:0:0: error: Bailing from out analysis: Checking file failed: converting \'1f\' to integer failed - not an integer [internalError]\n\n^\n'.format(test_file)

test/cli/testutils.py

@@ -63,12 +63,12 @@ def lookup_cppcheck_exe():
 
 
 # Run Cppcheck with args
-def cppcheck(args):
+def cppcheck(args, env=None):
     exe = lookup_cppcheck_exe()
     assert exe is not None, 'no cppcheck binary found'
 
     logging.info(exe + ' ' + ' '.join(args))
-    p = subprocess.Popen([exe] + args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    p = subprocess.Popen([exe] + args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
     comm = p.communicate()
     stdout = comm[0].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
     stderr = comm[1].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')