From 0e0288a25c29a1c6f40aa95712ea08832053c521 Mon Sep 17 00:00:00 2001 From: glank Date: Mon, 16 Jun 2025 17:03:41 +0200 Subject: [PATCH 1/7] Check for license error in skipAnalysis --- lib/analyzerinfo.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/analyzerinfo.cpp b/lib/analyzerinfo.cpp index dfb953c9445..d4668d27bf0 100644 --- a/lib/analyzerinfo.cpp +++ b/lib/analyzerinfo.cpp @@ -98,6 +98,13 @@ static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash, if (!attr || attr != std::to_string(hash)) return false; + // Check for invalid license error, in which case we should retry analysis + // Any kind of internal error should be checked for here + for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { + if (std::strcmp(e->Name(), "error") == 0 && e->Attribute("id", "premium-invalidLicense")) + return false; + } + for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { if (std::strcmp(e->Name(), "error") == 0) errors.emplace_back(e); From 6d35a001c8caa525408964cd93d6b5f1f79e5f44 Mon Sep 17 00:00:00 2001 From: glank Date: Tue, 17 Jun 2025 11:31:47 +0200 Subject: [PATCH 2/7] Add test --- test/cli/premium_test.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/cli/premium_test.py b/test/cli/premium_test.py index 12dd7212450..e8d503fbbc2 100644 --- a/test/cli/premium_test.py +++ b/test/cli/premium_test.py @@ -33,7 +33,6 @@ def __copy_cppcheck_premium(tmpdir): return exe - def test_misra_c_builtin_style_checks(tmpdir): # FIXME this test does not work in ci-windows.yml (release build) if sys.platform == 'win32': @@ -130,3 +129,28 @@ def test_misra_py(tmpdir): _, stdout, _ = cppcheck(['--enable=style', '--premium=misra-c-2012', test_file], cppcheck_exe=exe) assert 'misra.py' not in stdout # Did not find misra.py assert 'Checking' in stdout + +def test_invalid_license_retry(tmpdir): + # Trac 13832 - cppcheck build dir: do not reuse cached results if there were invalidLicense errors + build_dir = os.path.join(tmpdir, 'b') + test_file = os.path.join(tmpdir, 'test.c') + addon_file = os.path.join(tmpdir, 'premiumaddon.py') + + os.mkdir(build_dir) + + with open(test_file, 'wt') as f: + f.write('void foo();\n') + + args = [f"--addon={addon_file}", f"--cppcheck-build-dir={build_dir}", '--xml', '--enable=all', test_file] + + with open(addon_file, 'wt') as f: + f.write('print(\'{"addon":"premium","column":0,"errorId":"invalidLicense","extra":"","file":"Cppcheck Premium","linenr":0,"message":"Invalid license: No license file was found, contact sales@cppchecksolutions.com","severity":"error"}\')') + + _, _, stderr = cppcheck(args) + assert 'Invalid license' in stderr + + with open(addon_file, 'wt') as f: + f.write('') + + _, _, stderr = cppcheck(args) + assert 'Invalid license' not in stderr From 4eb0fe5c7d2c4cc04493418b72b92b4db1645e5c Mon Sep 17 00:00:00 2001 From: glank Date: Tue, 17 Jun 2025 14:47:52 +0200 Subject: [PATCH 3/7] Refactor skipAnalysis and add unit tests --- lib/analyzerinfo.cpp | 22 +++--- lib/analyzerinfo.h | 9 +++ test/testanalyzerinformation.cpp | 113 +++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 11 deletions(-) diff --git a/lib/analyzerinfo.cpp b/lib/analyzerinfo.cpp index d4668d27bf0..205125603fe 100644 --- a/lib/analyzerinfo.cpp +++ b/lib/analyzerinfo.cpp @@ -83,14 +83,9 @@ void AnalyzerInformation::close() } } -static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash, std::list &errors) +bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors) { - tinyxml2::XMLDocument doc; - const tinyxml2::XMLError error = doc.LoadFile(analyzerInfoFile.c_str()); - if (error != tinyxml2::XML_SUCCESS) - return false; - - const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement(); + const tinyxml2::XMLElement * const rootNode = analyzerInfoDoc.FirstChildElement(); if (rootNode == nullptr) return false; @@ -98,10 +93,12 @@ static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash, if (!attr || attr != std::to_string(hash)) return false; - // Check for invalid license error, in which case we should retry analysis - // Any kind of internal error should be checked for here + // Check for invalid license error or internal error, in which case we should retry analysis for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { - if (std::strcmp(e->Name(), "error") == 0 && e->Attribute("id", "premium-invalidLicense")) + if (std::strcmp(e->Name(), "error") == 0 && + (e->Attribute("id", "premium-invalidLicense") || + e->Attribute("id", "internal") + )) return false; } @@ -154,7 +151,10 @@ bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::st mAnalyzerInfoFile = AnalyzerInformation::getAnalyzerInfoFile(buildDir,sourcefile,cfg,fileIndex); - if (skipAnalysis(mAnalyzerInfoFile, hash, errors)) + tinyxml2::XMLDocument analyzerInfoDoc; + analyzerInfoDoc.LoadFile(mAnalyzerInfoFile.c_str()); + + if (skipAnalysis(analyzerInfoDoc, hash, errors)) return false; mOutputStream.open(mAnalyzerInfoFile); diff --git a/lib/analyzerinfo.h b/lib/analyzerinfo.h index e5466906707..a67a2291a29 100644 --- a/lib/analyzerinfo.h +++ b/lib/analyzerinfo.h @@ -31,6 +31,10 @@ class ErrorMessage; struct FileSettings; +namespace tinyxml2 { + class XMLDocument; +}; + /// @addtogroup Core /// @{ @@ -49,6 +53,8 @@ struct FileSettings; */ class CPPCHECKLIB AnalyzerInformation { public: + friend class TestAnalyzerInformation; + ~AnalyzerInformation(); static std::string getFilesTxt(const std::list &sourcefiles, const std::string &userDefines, const std::list &fileSettings); @@ -75,7 +81,10 @@ class CPPCHECKLIB AnalyzerInformation { protected: static std::string getAnalyzerInfoFileFromFilesTxt(std::istream& filesTxt, const std::string &sourcefile, const std::string &cfg, int fileIndex); + private: + static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors); + std::ofstream mOutputStream; std::string mAnalyzerInfoFile; }; diff --git a/test/testanalyzerinformation.cpp b/test/testanalyzerinformation.cpp index f1d94d4af3c..db76033cf99 100644 --- a/test/testanalyzerinformation.cpp +++ b/test/testanalyzerinformation.cpp @@ -20,8 +20,10 @@ #include "analyzerinfo.h" #include "filesettings.h" #include "fixture.h" +#include "xml.h" #include +#include class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation { public: @@ -34,6 +36,7 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation TEST_CASE(duplicateFile); TEST_CASE(filesTextDuplicateFile); TEST_CASE(parse); + TEST_CASE(skipAnalysis); } void getAnalyzerInfoFile() const { @@ -95,6 +98,116 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation ASSERT_EQUALS(0, info.fileIndex); ASSERT_EQUALS("C:/dm/cppcheck-fix-13333/test/cli/whole-program/odr1.cpp", info.sourceFile); } + + void skipAnalysis() const { + // Matching hash with license error (don't skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + "" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); + ASSERT_EQUALS(errorList.size(), 0); + } + + // Matching hash with internal error (don't skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + "" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); + ASSERT_EQUALS(errorList.size(), 0); + } + + // Matching hash with normal error (skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + "" + "" + "ptr" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), true); + ASSERT_EQUALS(errorList.size(), 1); + } + + // Matching hash with no error (skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), true); + ASSERT_EQUALS(errorList.size(), 0); + } + + // Different hash with normal error (don't skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + "" + "" + "ptr" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 99, errorList), false); + ASSERT_EQUALS(errorList.size(), 0); + } + + // Empty document (don't skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse(""); + ASSERT_EQUALS(xmlError, tinyxml2::XML_ERROR_EMPTY_DOCUMENT); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); + ASSERT_EQUALS(errorList.size(), 0); + } + } }; REGISTER_TEST(TestAnalyzerInformation) From 8f63971c6a372705914c2d9dcb4034f4dd73d72c Mon Sep 17 00:00:00 2001 From: glank Date: Tue, 17 Jun 2025 14:57:24 +0200 Subject: [PATCH 4/7] Run dmake --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index fa1f6624783..da21105539d 100644 --- a/Makefile +++ b/Makefile @@ -710,7 +710,7 @@ test/options.o: test/options.cpp test/options.h test/test64bit.o: test/test64bit.cpp lib/addoninfo.h lib/check.h lib/check64bit.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/test64bit.cpp -test/testanalyzerinformation.o: test/testanalyzerinformation.cpp lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/utils.h test/fixture.h +test/testanalyzerinformation.o: test/testanalyzerinformation.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/utils.h lib/xml.h test/fixture.h $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testanalyzerinformation.cpp test/testassert.o: test/testassert.cpp lib/addoninfo.h lib/check.h lib/checkassert.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 From 0c6b1a54eb24dafec259525ae56325852e3ec1d9 Mon Sep 17 00:00:00 2001 From: glank Date: Tue, 17 Jun 2025 16:25:36 +0200 Subject: [PATCH 5/7] Change internal -> internalError, add premium-internalError with test --- lib/analyzerinfo.cpp | 3 ++- test/testanalyzerinformation.cpp | 21 ++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/analyzerinfo.cpp b/lib/analyzerinfo.cpp index 205125603fe..925d33ca2bb 100644 --- a/lib/analyzerinfo.cpp +++ b/lib/analyzerinfo.cpp @@ -97,7 +97,8 @@ bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfo for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { if (std::strcmp(e->Name(), "error") == 0 && (e->Attribute("id", "premium-invalidLicense") || - e->Attribute("id", "internal") + e->Attribute("id", "premium-internalError") || + e->Attribute("id", "internalError") )) return false; } diff --git a/test/testanalyzerinformation.cpp b/test/testanalyzerinformation.cpp index db76033cf99..1b8e56c3cc2 100644 --- a/test/testanalyzerinformation.cpp +++ b/test/testanalyzerinformation.cpp @@ -119,6 +119,25 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation ASSERT_EQUALS(errorList.size(), 0); } + // Matching hash with premium internal error (don't skip) + { + std::list errorList; + tinyxml2::XMLDocument doc; + + const tinyxml2::XMLError xmlError = doc.Parse( + "" + "" + "" + "" + "" + "" + ); + ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + + ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); + ASSERT_EQUALS(errorList.size(), 0); + } + // Matching hash with internal error (don't skip) { std::list errorList; @@ -127,7 +146,7 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation const tinyxml2::XMLError xmlError = doc.Parse( "" "" - "" + "" "" "" "" From d73cde7f1735825cd78fe393134cf6aefc78a48d Mon Sep 17 00:00:00 2001 From: glank Date: Wed, 18 Jun 2025 11:29:31 +0200 Subject: [PATCH 6/7] Add explicit check for LoadFile success --- lib/analyzerinfo.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/analyzerinfo.cpp b/lib/analyzerinfo.cpp index 925d33ca2bb..f303783c737 100644 --- a/lib/analyzerinfo.cpp +++ b/lib/analyzerinfo.cpp @@ -153,9 +153,8 @@ bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::st mAnalyzerInfoFile = AnalyzerInformation::getAnalyzerInfoFile(buildDir,sourcefile,cfg,fileIndex); tinyxml2::XMLDocument analyzerInfoDoc; - analyzerInfoDoc.LoadFile(mAnalyzerInfoFile.c_str()); - - if (skipAnalysis(analyzerInfoDoc, hash, errors)) + const tinyxml2::XMLError xmlError = analyzerInfoDoc.LoadFile(mAnalyzerInfoFile.c_str()); + if (xmlError == tinyxml2::XML_SUCCESS && skipAnalysis(analyzerInfoDoc, hash, errors)) return false; mOutputStream.open(mAnalyzerInfoFile); From 8ce78f957ae4ef356ea85ec57893520be7c65ae4 Mon Sep 17 00:00:00 2001 From: glank Date: Wed, 18 Jun 2025 18:37:58 +0200 Subject: [PATCH 7/7] Fix nits --- test/cli/premium_test.py | 2 ++ test/testanalyzerinformation.cpp | 42 ++++++++++++++++---------------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/test/cli/premium_test.py b/test/cli/premium_test.py index e8d503fbbc2..b0df148f22b 100644 --- a/test/cli/premium_test.py +++ b/test/cli/premium_test.py @@ -33,6 +33,7 @@ def __copy_cppcheck_premium(tmpdir): return exe + def test_misra_c_builtin_style_checks(tmpdir): # FIXME this test does not work in ci-windows.yml (release build) if sys.platform == 'win32': @@ -130,6 +131,7 @@ def test_misra_py(tmpdir): assert 'misra.py' not in stdout # Did not find misra.py assert 'Checking' in stdout + def test_invalid_license_retry(tmpdir): # Trac 13832 - cppcheck build dir: do not reuse cached results if there were invalidLicense errors build_dir = os.path.join(tmpdir, 'b') diff --git a/test/testanalyzerinformation.cpp b/test/testanalyzerinformation.cpp index 1b8e56c3cc2..e6dab0fd156 100644 --- a/test/testanalyzerinformation.cpp +++ b/test/testanalyzerinformation.cpp @@ -113,10 +113,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(false, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(0, errorList.size()); } // Matching hash with premium internal error (don't skip) @@ -132,10 +132,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(false, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(0, errorList.size()); } // Matching hash with internal error (don't skip) @@ -151,10 +151,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(false, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(0, errorList.size()); } // Matching hash with normal error (skip) @@ -172,10 +172,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), true); - ASSERT_EQUALS(errorList.size(), 1); + ASSERT_EQUALS(true, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(1, errorList.size()); } // Matching hash with no error (skip) @@ -188,10 +188,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), true); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(true, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(0, errorList.size()); } // Different hash with normal error (don't skip) @@ -209,10 +209,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation "" "" ); - ASSERT_EQUALS(xmlError, tinyxml2::XML_SUCCESS); + ASSERT_EQUALS(tinyxml2::XML_SUCCESS, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 99, errorList), false); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(false, AnalyzerInformation::skipAnalysis(doc, 99, errorList)); + ASSERT_EQUALS(0, errorList.size()); } // Empty document (don't skip) @@ -221,10 +221,10 @@ class TestAnalyzerInformation : public TestFixture, private AnalyzerInformation tinyxml2::XMLDocument doc; const tinyxml2::XMLError xmlError = doc.Parse(""); - ASSERT_EQUALS(xmlError, tinyxml2::XML_ERROR_EMPTY_DOCUMENT); + ASSERT_EQUALS(tinyxml2::XML_ERROR_EMPTY_DOCUMENT, xmlError); - ASSERT_EQUALS(AnalyzerInformation::skipAnalysis(doc, 100, errorList), false); - ASSERT_EQUALS(errorList.size(), 0); + ASSERT_EQUALS(false, AnalyzerInformation::skipAnalysis(doc, 100, errorList)); + ASSERT_EQUALS(0, errorList.size()); } } };