From 2bd60c82cb00136000162447aaa1af3ab6ba4a8e Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 09:54:14 -0800 Subject: [PATCH 1/7] Fix CodeQL violations of type 'cpp/path-injection in lib/pal --- lib/pal/PAL.cpp | 31 ++++++++++++ lib/pal/PAL.hpp | 10 ++++ tests/unittests/PalTests.cpp | 91 ++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 98a4833f1..016df98d6 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -47,6 +47,7 @@ #include #pragma comment(lib, "Ole32.Lib") /* CoCreateGuid */ #include +#include #endif #ifdef ANDROID @@ -113,8 +114,28 @@ namespace PAL_NS_BEGIN { return result; } + // Check if the path exists +#if defined(_WIN32) || defined(_WIN64) + DWORD fileAttr = GetFileAttributesA(traceFolderPath.c_str()); + bool pathExists = (fileAttr != INVALID_FILE_ATTRIBUTES && (fileAttr & FILE_ATTRIBUTE_DIRECTORY)); +#else + bool pathExists = (access(traceFolderPath.c_str(), F_OK) != -1); +#endif + // Check if the path contains ".." + bool containsParentDirectory = (traceFolderPath.find("..") != std::string::npos); + + if (!pathExists || containsParentDirectory) + { + assert(false && "Invalid trace folder path."); + return false; + } + debugLogMutex.lock(); debugLogPath = traceFolderPath; + if (debugLogPath.back() != '/' && debugLogPath.back() != '\\') + { + debugLogPath += "/"; + } debugLogPath += "mat-debug-"; debugLogPath += std::to_string(MAT::GetCurrentProcessId()); debugLogPath += ".log"; @@ -131,6 +152,16 @@ namespace PAL_NS_BEGIN { return result; } + const std::unique_ptr& getDebugLogStream() noexcept + { + return debugLogStream; + } + + const std::string& getDebugLogPath() noexcept + { + return debugLogPath; + } + void log_done() { debugLogMutex.lock(); diff --git a/lib/pal/PAL.hpp b/lib/pal/PAL.hpp index 64dbb1240..f79cdd080 100644 --- a/lib/pal/PAL.hpp +++ b/lib/pal/PAL.hpp @@ -239,6 +239,16 @@ namespace PAL_NS_BEGIN return GetPAL().RegisterIkeyWithWindowsTelemetry(ikeyin, storageSize, uploadQuotaSize); } +#ifdef HAVE_MAT_LOGGING + namespace detail + { + bool log_init(bool isTraceEnabled, const std::string& traceFolderPath); + const std::unique_ptr& getDebugLogStream() noexcept; + const std::string& getDebugLogPath() noexcept; + void log_done(); + } +#endif + } PAL_NS_END #endif diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index ca660b33f..8506de403 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -7,6 +7,21 @@ #include "pal/PseudoRandomGenerator.hpp" #include "Version.hpp" +#ifdef HAVE_MAT_LOGGING +#include "pal/PAL.hpp" +#include +#include +#include +#include +#include +#if defined(_WIN32) || defined(_WIN64) +#include +#else +#include +#endif +using namespace PAL::detail; +#endif + using namespace testing; class PalTests : public Test {}; @@ -127,3 +142,79 @@ TEST_F(PalTests, SdkVersion) EXPECT_THAT(PAL::getSdkVersion(), Eq(v)); } + +#ifdef HAVE_MAT_LOGGING +class LogInitTest : public Test +{ + protected: + const std::string validPath = "valid/path/"; + + void SetUp() override + { + // Create the valid path directory + #if defined(_WIN32) || defined(_WIN64) + CreateDirectoryA(validPath, NULL); + #else + mkdir(validPath.c_str(), 0777); + #endif + } + + void TearDown() override + { + PAL::detail::log_done(); + if (!PAL::detail::getDebugLogPath().empty()) + { + std::remove(PAL::detail::getDebugLogPath().c_str()); + } + + // Remove the valid path directory + #if defined(_WIN32) || defined(_WIN64) + RemoveDirectoryA(validPath); + #else + rmdir(validPath.c_str()); + #endif + } +}; + +TEST_F(LogInitTest, LogInitDisabled) +{ + EXPECT_FALSE(log_init(false, validPath)); +} + +TEST_F(LogInitTest, LogInitValidPath) +{ + EXPECT_TRUE(PAL::detail::log_init(true, validPath)); + EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open()); +} + +TEST_F(LogInitTest, LogInitParentDirectoryInvalidPath) +{ + EXPECT_FALSE(PAL::detail::log_init(true, "invalid/../path/")); +} + +TEST_F(LogInitTest, LogInitPathDoesNotExist) +{ + EXPECT_FALSE(PAL::detail::log_init(true, "nonexistent/path/")); +} + +TEST_F(LogInitTest, LogInitAlreadyInitialized) +{ + EXPECT_TRUE(PAL::detail::log_init(true, validPath)); + EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open()); + EXPECT_TRUE(PAL::detail::log_init(true, validPath)); // Should return true as it's already initialized +} + +TEST_F(LogInitTest, LogInitPathWithoutTrailingSlash) +{ + std::string pathWithoutSlash = "valid/path"; + EXPECT_TRUE(PAL::detail::log_init(true, pathWithoutSlash)); + EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open()); +} + +TEST_F(LogInitTest, LogInitPathWithoutTrailingBackslash) +{ + std::string pathWithoutBackslash = "valid\\path"; + EXPECT_TRUE(PAL::detail::log_init(true, pathWithoutBackslash)); + EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open()); +} +#endif From bb91937ea41570be2cfbf237cda0f6c73c9f97f2 Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 10:31:29 -0800 Subject: [PATCH 2/7] fix build error and test --- lib/pal/PAL.cpp | 2 +- tests/unittests/PalTests.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 016df98d6..002f8bbfb 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -126,7 +126,7 @@ namespace PAL_NS_BEGIN { if (!pathExists || containsParentDirectory) { - assert(false && "Invalid trace folder path."); + std::cerr << "Invalid trace folder path." << std::endl; return false; } diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 8506de403..449269bd8 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -153,7 +153,7 @@ class LogInitTest : public Test { // Create the valid path directory #if defined(_WIN32) || defined(_WIN64) - CreateDirectoryA(validPath, NULL); + CreateDirectoryA(validPath.c_str(), NULL); #else mkdir(validPath.c_str(), 0777); #endif @@ -169,7 +169,7 @@ class LogInitTest : public Test // Remove the valid path directory #if defined(_WIN32) || defined(_WIN64) - RemoveDirectoryA(validPath); + RemoveDirectoryA(validPath.c_str()); #else rmdir(validPath.c_str()); #endif From 78e030aa855dad04b492b3a25fbaf8d536f37ac4 Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 11:05:26 -0800 Subject: [PATCH 3/7] fix tests --- tests/unittests/PalTests.cpp | 68 ++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 449269bd8..17361ba25 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -147,33 +147,63 @@ TEST_F(PalTests, SdkVersion) class LogInitTest : public Test { protected: - const std::string validPath = "valid/path/"; + const std::string validPath = "valid/path/"; - void SetUp() override + void SetUp() override + { + // Create the valid path directory and any intermediate directories +#if defined(_WIN32) || defined(_WIN64) + if (CreateDirectoryA("valid", NULL) || GetLastError() == ERROR_ALREADY_EXISTS) { - // Create the valid path directory - #if defined(_WIN32) || defined(_WIN64) - CreateDirectoryA(validPath.c_str(), NULL); - #else - mkdir(validPath.c_str(), 0777); - #endif + if (CreateDirectoryA(validPath.c_str(), NULL) || GetLastError() == ERROR_ALREADY_EXISTS) + { + std::cerr << "Directory created: " << validPath << std::endl; + } + else + { + std::cerr << "Error creating directory: " << validPath << std::endl; + } } - - void TearDown() override + else { - PAL::detail::log_done(); - if (!PAL::detail::getDebugLogPath().empty()) + std::cerr << "Error creating directory: valid" << std::endl; + } +#else + if (mkdir("valid", 0777) == 0 || errno == EEXIST) + { + if (mkdir(validPath.c_str(), 0777) == 0 || errno == EEXIST) + { + std::cerr << "Directory created: " << validPath << std::endl; + } + else { - std::remove(PAL::detail::getDebugLogPath().c_str()); + std::cerr << "Error creating directory: " << validPath << std::endl; } + } + else + { + std::cerr << "Error creating directory: valid" << std::endl; + } +#endif + } - // Remove the valid path directory - #if defined(_WIN32) || defined(_WIN64) - RemoveDirectoryA(validPath.c_str()); - #else - rmdir(validPath.c_str()); - #endif + void TearDown() override + { + PAL::detail::log_done(); + if (!PAL::detail::getDebugLogPath().empty()) + { + std::remove(PAL::detail::getDebugLogPath().c_str()); } + + // Remove the valid path directory +#if defined(_WIN32) || defined(_WIN64) + RemoveDirectoryA(validPath.c_str()); + RemoveDirectoryA("valid"); +#else + rmdir(validPath.c_str()); + rmdir("valid"); +#endif + } }; TEST_F(LogInitTest, LogInitDisabled) From d52164a273afcc9ec7e1b50a8af64ed89a20847a Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 12:03:19 -0800 Subject: [PATCH 4/7] fix tests --- tests/unittests/PalTests.cpp | 77 +++++++++++++----------------------- 1 file changed, 28 insertions(+), 49 deletions(-) diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 17361ba25..777af54f1 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -147,63 +147,37 @@ TEST_F(PalTests, SdkVersion) class LogInitTest : public Test { protected: - const std::string validPath = "valid/path/"; + const std::string validPath = "valid/path/"; - void SetUp() override - { - // Create the valid path directory and any intermediate directories -#if defined(_WIN32) || defined(_WIN64) - if (CreateDirectoryA("valid", NULL) || GetLastError() == ERROR_ALREADY_EXISTS) - { - if (CreateDirectoryA(validPath.c_str(), NULL) || GetLastError() == ERROR_ALREADY_EXISTS) - { - std::cerr << "Directory created: " << validPath << std::endl; - } - else - { - std::cerr << "Error creating directory: " << validPath << std::endl; - } - } - else + void SetUp() override { - std::cerr << "Error creating directory: valid" << std::endl; + // Create the valid path directory and any intermediate directories + #if defined(_WIN32) || defined(_WIN64) + CreateDirectoryA("valid", NULL); + CreateDirectoryA(validPath.c_str(), NULL); + #else + mkdir("valid", 0777); + mkdir(validPath.c_str(), 0777); + #endif } -#else - if (mkdir("valid", 0777) == 0 || errno == EEXIST) + + void TearDown() override { - if (mkdir(validPath.c_str(), 0777) == 0 || errno == EEXIST) - { - std::cerr << "Directory created: " << validPath << std::endl; - } - else + PAL::detail::log_done(); + if (!PAL::detail::getDebugLogPath().empty()) { - std::cerr << "Error creating directory: " << validPath << std::endl; + std::remove(PAL::detail::getDebugLogPath().c_str()); } - } - else - { - std::cerr << "Error creating directory: valid" << std::endl; - } -#endif - } - void TearDown() override - { - PAL::detail::log_done(); - if (!PAL::detail::getDebugLogPath().empty()) - { - std::remove(PAL::detail::getDebugLogPath().c_str()); + // Remove the valid path directory + #if defined(_WIN32) || defined(_WIN64) + RemoveDirectoryA(validPath.c_str()); + RemoveDirectoryA("valid"); + #else + rmdir(validPath.c_str()); + rmdir("valid"); + #endif } - - // Remove the valid path directory -#if defined(_WIN32) || defined(_WIN64) - RemoveDirectoryA(validPath.c_str()); - RemoveDirectoryA("valid"); -#else - rmdir(validPath.c_str()); - rmdir("valid"); -#endif - } }; TEST_F(LogInitTest, LogInitDisabled) @@ -243,8 +217,13 @@ TEST_F(LogInitTest, LogInitPathWithoutTrailingSlash) TEST_F(LogInitTest, LogInitPathWithoutTrailingBackslash) { +#if defined(_WIN32) || defined(_WIN64) std::string pathWithoutBackslash = "valid\\path"; +#else + std::string pathWithoutBackslash = "valid//path"; +#endif EXPECT_TRUE(PAL::detail::log_init(true, pathWithoutBackslash)); EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open()); } + #endif From 96309ee06e68e0fe0f10503864e968130ae8768a Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 13:51:11 -0800 Subject: [PATCH 5/7] use expect_throw for runtime_error --- lib/pal/PAL.cpp | 2 +- tests/unittests/PalTests.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 002f8bbfb..9bd37c1c2 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -126,7 +126,7 @@ namespace PAL_NS_BEGIN { if (!pathExists || containsParentDirectory) { - std::cerr << "Invalid trace folder path." << std::endl; + throw std::runtime_error("Invalid trace folder path."); return false; } diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 777af54f1..e07346335 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -193,12 +193,12 @@ TEST_F(LogInitTest, LogInitValidPath) TEST_F(LogInitTest, LogInitParentDirectoryInvalidPath) { - EXPECT_FALSE(PAL::detail::log_init(true, "invalid/../path/")); + EXPECT_THROW(PAL::detail::log_init(true, "invalid/../path/"), std::runtime_error); } TEST_F(LogInitTest, LogInitPathDoesNotExist) { - EXPECT_FALSE(PAL::detail::log_init(true, "nonexistent/path/")); + EXPECT_THROW(PAL::detail::log_init(true, "nonexistent/path/"), std::runtime_error); } TEST_F(LogInitTest, LogInitAlreadyInitialized) From ce831c22444e4344c2bce00f9d6f9f937766b4fb Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 13:56:25 -0800 Subject: [PATCH 6/7] fix build error --- lib/pal/PAL.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 9bd37c1c2..a2f6161f6 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -127,7 +127,6 @@ namespace PAL_NS_BEGIN { if (!pathExists || containsParentDirectory) { throw std::runtime_error("Invalid trace folder path."); - return false; } debugLogMutex.lock(); From dbc66661d0288b469ab74ca0fc39afb1906d2cf2 Mon Sep 17 00:00:00 2001 From: Abu Hasnat Mohd Sayem Date: Tue, 4 Mar 2025 17:20:54 -0800 Subject: [PATCH 7/7] address comments --- lib/pal/PAL.cpp | 2 +- tests/unittests/PalTests.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index a2f6161f6..01c6e6f75 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -126,7 +126,7 @@ namespace PAL_NS_BEGIN { if (!pathExists || containsParentDirectory) { - throw std::runtime_error("Invalid trace folder path."); + return false; } debugLogMutex.lock(); diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index e07346335..777af54f1 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -193,12 +193,12 @@ TEST_F(LogInitTest, LogInitValidPath) TEST_F(LogInitTest, LogInitParentDirectoryInvalidPath) { - EXPECT_THROW(PAL::detail::log_init(true, "invalid/../path/"), std::runtime_error); + EXPECT_FALSE(PAL::detail::log_init(true, "invalid/../path/")); } TEST_F(LogInitTest, LogInitPathDoesNotExist) { - EXPECT_THROW(PAL::detail::log_init(true, "nonexistent/path/"), std::runtime_error); + EXPECT_FALSE(PAL::detail::log_init(true, "nonexistent/path/")); } TEST_F(LogInitTest, LogInitAlreadyInitialized)