PackChk: issue on relative paths during case-sense check (#452)

PackChk: issue on relative paths during case-sense check excluding files from case sense check that have relative path elements "./" or "../" added test if files are below pack root directory – added test Co-authored-by: Thorsten de Buhr <[email protected]>
Open-CMSIS-Pack · Jan 31, 2023 · 6b5dc76 · 6b5dc76
1 parent c327305
commit 6b5dc76
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 2 deletions.
diff --git a/tools/packchk/include/CheckFiles.h b/tools/packchk/include/CheckFiles.h
@@ -32,6 +32,7 @@ class CheckFiles {
   bool CheckFile(RteItem* item);
   bool CheckFileExists(const std::string& fileName, int lineNo, bool associated = false);
   bool CheckCaseSense(const std::string& fileName, int lineNo);
+  bool CheckFileIsInPack(const std::string& fileName, int lineNo);
   bool FindGetExactFileSystemName(const std::string& path, const std::string& fileNameIn, std::string& fileNameOut);
   bool CheckFileHasVersion(RteItem* item);
   bool CheckFileExtension(RteItem* item);

diff --git a/tools/packchk/src/CheckFiles.cpp b/tools/packchk/src/CheckFiles.cpp
@@ -236,6 +236,7 @@ bool CheckFiles::CheckFile(RteItem* item)
   if(!fileName.empty()) {
     if(CheckFileExists(fileName, lineNo)) {
       CheckCaseSense(fileName, lineNo);
+      CheckFileIsInPack(fileName, lineNo);
     }
 
     if(tag == "environment" && envName == "DS5") {
@@ -261,6 +262,7 @@ bool CheckFiles::CheckFile(RteItem* item)
   if(!fileName2.empty()) {
     if(CheckFileExists(fileName2, lineNo)) {
       CheckCaseSense(fileName2, lineNo);   // File must exist for this check!
+      CheckFileIsInPack(fileName2, lineNo);
     }
   }
 
@@ -354,6 +356,35 @@ bool CheckFiles::FindGetExactFileSystemName(const std::string& path, const std::
   return false;
 }
 
+
+/**
+ * @brief check if file is below pack root folder
+ * @param fileName filename as written in PDSC
+ * @param lineNo line number for error reporting
+ * @return true/false
+*/
+bool CheckFiles::CheckFileIsInPack(const string& fileName, int lineNo)
+{
+  if (fileName.empty()) {
+    return true;
+  }
+
+  string fullFileName = GetFullFilename(fileName);
+  string absPath = RteFsUtils::MakePathCanonical(fullFileName);
+  if(absPath.empty()) {
+    return true;
+  }
+
+  const auto& packPath = GetPackagePath();
+  if(absPath.find(packPath, 0) != 0) {
+    LogMsg("M313", PATH(fileName), lineNo);
+    return false;
+  }
+
+  return true;
+}
+
+
 /**
  * @brief check name as written in PDSC against it's counterpart on the filesystem, for case sensitivity
  * @param fileName filename as written in PDSC
@@ -366,6 +397,12 @@ bool CheckFiles::CheckCaseSense(const string& fileName, int lineNo)
     return true;
   }
 
+  // not testing relative paths. They must be interpreted first to be comparable to FS path,
+  // and there is no function doing this without changing the case of path characters.
+  if(fileName.find("./") != string::npos || fileName.find("../") != string::npos) {
+    return true;
+  }
+
   LogMsg("M058", PATH(fileName));
 
   string filePath, testPath, outPath, packPath;

diff --git a/tools/packchk/src/PackChk_Msgs.cpp b/tools/packchk/src/PackChk_Msgs.cpp
@@ -121,7 +121,7 @@ const MsgTable PackChk::msgTable = {
                                                 "  Filename  : '%SYSTEM%'" } },
   { "M311", { MsgLevel::LEVEL_ERROR,    CRLF_B, "Redefinition of '%TAG%' : '%NAME%', see Line %LINE%" } },
   { "M312", { MsgLevel::LEVEL_WARNING3, CRLF_B, "No '%TAG%' found for device '%NAME%'" } },
-  { "M313", { MsgLevel::LEVEL_WARNING,  CRLF_B, "" } },
+  { "M313", { MsgLevel::LEVEL_ERROR,    CRLF_B, "Associated File/Path is outside PACK root: '%PATH%'" } },
   { "M314", { MsgLevel::LEVEL_ERROR,    CRLF_B, "" } },
   { "M315", { MsgLevel::LEVEL_ERROR,    CRLF_B, "Invalid URL / Paths to Drives are not allowed in Package URL: '%URL%'" } },
   { "M316", { MsgLevel::LEVEL_WARNING,  CRLF_B, "URL must end with slash '/': '%URL%'" } },

diff --git a/tools/packchk/test/data/TestPackRoot/ExtFiles/test2.c b/tools/packchk/test/data/TestPackRoot/ExtFiles/test2.c
@@ -0,0 +1 @@
+// tests1.c
diff --git a/tools/packchk/test/data/TestPackRoot/Pack/Files/header1.h b/tools/packchk/test/data/TestPackRoot/Pack/Files/header1.h
@@ -0,0 +1 @@
+//header 1
diff --git a/tools/packchk/test/data/TestPackRoot/Pack/Files/test1.c b/tools/packchk/test/data/TestPackRoot/Pack/Files/test1.c
@@ -0,0 +1 @@
+// tests1.c
diff --git a/tools/packchk/test/data/TestPackRoot/Pack/TestVendor.TestPackRoot.pdsc b/tools/packchk/test/data/TestPackRoot/Pack/TestVendor.TestPackRoot.pdsc
@@ -0,0 +1,36 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<package schemaVersion="1.4" xmlns:xs="http://www.w3.org/2001/XMLSchema-instance" xs:noNamespaceSchemaLocation="PACK.xsd">
+  <vendor>TestVendor</vendor>
+  <url>http://www.testurl.com/pack/</url>
+  <name>TestPackRoot</name>
+  <description>TestPackRoot</description>
+
+  <releases>
+    <release version="0.0.1" date="2021-09-06">>
+      Initial release of TestPackRoot.
+    </release>
+  </releases>
+
+  <keywords>
+    <keyword>TestPackRoot</keyword>
+  </keywords>
+
+  <conditions>
+    <condition id="Test_Condition">
+      <description>Test Device</description>
+      <require Dvendor="ARM:82"/>
+    </condition>
+  </conditions>
+
+  <components>
+    <component Cclass="TestClass" Cgroup="TestGlobal" Cversion="1.0.0" condition="Test_Condition">
+      <description>TestGlobal</description>
+      <files>
+        <file category="source" name="Files/test1.c"/>
+        <file category="source" name="../ExtFiles/test2.c"/>
+      </files>
+    </component>
+  </components>
+
+</package>
diff --git a/tools/packchk/test/integtests/src/PackChkIntegTests.cpp b/tools/packchk/test/integtests/src/PackChkIntegTests.cpp
@@ -495,3 +495,31 @@ TEST_F(PackChkIntegTests, CheckAllowSuppressError) {
     FAIL() << "error: found error M323";
   }
 }
+
+// Validate files are inside pack root
+TEST_F(PackChkIntegTests, CheckTestPackRoot) {
+  const char* argv[2];
+
+  const string& pdscFile = PackChkIntegTestEnv::localtestdata_dir +
+    "/TestPackRoot/Pack/TestVendor.TestPackRoot.pdsc";
+  ASSERT_TRUE(RteFsUtils::Exists(pdscFile));
+
+  argv[0] = (char*)"";
+  argv[1] = (char*)pdscFile.c_str();
+
+  PackChk packChk;
+  EXPECT_EQ(1, packChk.Check(2, argv, nullptr));
+
+  auto errMsgs = ErrLog::Get()->GetLogMessages();
+  int foundCnt = 0;
+  for (const string& msg : errMsgs) {
+    size_t s;
+    if ((s = msg.find("M313")) != string::npos) {
+      foundCnt++;
+    }
+  }
+
+  if (foundCnt != 1) {
+    FAIL() << "error: missing message M313";
+  }
+}
diff --git a/tools/packchk/test/unittests/src/TestCheckFiles.cpp b/tools/packchk/test/unittests/src/TestCheckFiles.cpp
@@ -175,7 +175,7 @@ TEST_F(TestCheckFiles, CheckCaseSense)
     { ".test1/NonExclusive.h",       true},
     { ".test1/../Api/Exclusive.h",   true},
     { "../testdata/Api/Exclusive.h", true},
-    { "../Invalid/Path/Exclusive.h", false},
+    { "../Invalid/Path/Exclusive.h", true},   // result is true now because relative paths are currently not checked
     { "api\\exclusive.h",            false},
     { "api/exclusive.h",             false}
   };