From 0ad1c7f74f751175c0d474f6b70c27b05d4d7c6d Mon Sep 17 00:00:00 2001 From: GamesTrap Date: Sun, 17 Nov 2024 14:32:49 +0100 Subject: [PATCH] Utils Added SafeSystem() as a replacement for unsafe system()/shell calls 11/17/2024 | 24w46b1 --- SITREPS.txt | 6 ++ TRAP/src/Core/Base.h | 2 +- TRAP/src/FileSystem/FileSystem.cpp | 61 +++-------- TRAP/src/FileSystem/FileSystem.h | 2 +- TRAP/src/Log/Log.h | 2 +- TRAP/src/TRAPPCH.h | 1 + TRAP/src/Utils/Linux.h | 2 +- TRAP/src/Utils/SafeSystem.cpp | 156 +++++++++++++++++++++++++++++ TRAP/src/Utils/SafeSystem.h | 15 +++ TRAP/src/Utils/String/String.cpp | 4 +- 10 files changed, 197 insertions(+), 54 deletions(-) create mode 100644 TRAP/src/Utils/SafeSystem.cpp create mode 100644 TRAP/src/Utils/SafeSystem.h diff --git a/SITREPS.txt b/SITREPS.txt index 6ee2402ac..7f4f1dd76 100644 --- a/SITREPS.txt +++ b/SITREPS.txt @@ -5385,3 +5385,9 @@ SITREP 11/13/2024|24w46a3 SITREP 11/16/2024|24w46a4 - Changed TRAP Engine version to 24w46a4(0.11.42) - UnitTests Added tests for TRAP::Events ~>30 mins + +SITREP 11/17/2024|24w46b1 + - Changed TRAP Engine version to 24w46b1(0.11.43) + - Utils/String Fixed unnecessary extra null terminator in CreateUTF8StringFromWideStringWin32()/CreateWideStringFromUTF8StringWin32() ~<5 mins + - Utils Added SafeSystem() as a replacement for unsafe system()/shell calls ~>30 mins + - FileSystem Replaced calls to popen()/ShellExecuteW() with SafeSystem() ~<10 mins diff --git a/TRAP/src/Core/Base.h b/TRAP/src/Core/Base.h index 0500cc252..0fc6762ce 100644 --- a/TRAP/src/Core/Base.h +++ b/TRAP/src/Core/Base.h @@ -77,7 +77,7 @@ //-------------------------------------------------------------------------------------------------------------------// /// @brief TRAP version number created with TRAP_MAKE_VERSION -inline constexpr TRAP::SemanticVersion<0, 11, 42> TRAP_VERSION{}; +inline constexpr TRAP::SemanticVersion<0, 11, 43> TRAP_VERSION{}; //-------------------------------------------------------------------------------------------------------------------// diff --git a/TRAP/src/FileSystem/FileSystem.cpp b/TRAP/src/FileSystem/FileSystem.cpp index 2f2045485..0c33c687d 100644 --- a/TRAP/src/FileSystem/FileSystem.cpp +++ b/TRAP/src/FileSystem/FileSystem.cpp @@ -1,6 +1,7 @@ #include "TRAPPCH.h" #include "FileSystem.h" +#include "Utils/SafeSystem.h" #include "Utils/String/String.h" #include "Application.h" @@ -1048,7 +1049,7 @@ namespace while(std::getline(file, line)) { const auto commentIndex = line.find_first_of('#'); - const auto xdgDocuments = line.find(XDGDocumentsDir.data()); + const auto xdgDocuments = line.find(XDGDocumentsDir); if(xdgDocuments == std::string::npos) continue; @@ -1117,15 +1118,11 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenExternallyLinux(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenExternallyLinux(): Path is not absolute!"); - const std::string cmd = fmt::format("xdg-open {}", p.native()); - FILE* const xdg = popen(cmd.c_str(), "r"); - if(xdg == nullptr) + if(const auto result = TRAP::Utils::SafeSystem("xdg-open", {p.native()}, false); !result) { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", p, "!"); - TP_ERROR(TRAP::Log::FileSystemPrefix, TRAP::Utils::String::GetStrError()); + TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", result.Error()); return false; } - pclose(xdg); return true; } @@ -1143,15 +1140,11 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenFolderInFileBrowserLinux(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenFolderInFileBrowserLinux(): Path is not absolute!"); - const std::string cmd = fmt::format("xdg-open {}", p.string()); - FILE* const xdg = popen(cmd.c_str(), "r"); - if(xdg == nullptr) + if(const auto result = TRAP::Utils::SafeSystem("xdg-open", {p.native()}, false); !result) { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open folder in file browser: ", p, "!"); - TP_ERROR(TRAP::Log::FileSystemPrefix, TRAP::Utils::String::GetStrError()); + TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open folder in file browser: ", result.Error()); return false; } - pclose(xdg); return true; } @@ -1169,15 +1162,11 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenFileInFileBrowserLinux(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenFileInFileBrowserLinux(): Path is not absolute!"); - const std::string cmd = fmt::format("xdg-open {}", p.parent_path().string()); - FILE* const xdg = popen(cmd.c_str(), "r"); - if(xdg == nullptr) + if(const auto result = TRAP::Utils::SafeSystem("xdg-open", {p.parent_path().native()}, false); !result) { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open file in file browser: ", p, "!"); - TP_ERROR(TRAP::Log::FileSystemPrefix, TRAP::Utils::String::GetStrError()); + TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open file in file browser: ", result.Error()); return false; } - pclose(xdg); return true; } @@ -1260,8 +1249,6 @@ namespace return TRAP::NullOpt; } - folderPath.pop_back(); //Remove the extra null byte - return folderPath; } @@ -1274,20 +1261,9 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenExternallyWindows(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenExternallyWindows(): Path is not absolute!"); - TRAP::Utils::Windows::COMInitializer comInitializer{}; - if(comInitializer.IsInitialized()) - { - HINSTANCE res = ShellExecuteW(nullptr, L"open", p.c_str(), nullptr, nullptr, SW_SHOWNORMAL); - if(std::bit_cast(res) <= 32) - { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", p, "!"); - TP_ERROR(TRAP::Log::FileSystemPrefix, TRAP::Utils::String::GetStrError()); - return false; - } - } - else + if (const auto result = TRAP::Utils::SafeSystem("explorer", { TRAP::Utils::String::CreateUTF8StringFromWideStringWin32(p.native()) }, false); !result) { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", p, " (COM initialization failed)!"); + TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", result.Error()); return false; } @@ -1306,20 +1282,9 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenFolderInFileBrowserWindows(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenFolderInFileBrowserWindows(): Path is not absolute!"); - TRAP::Utils::Windows::COMInitializer comInitializer{}; - if(comInitializer.IsInitialized()) - { - HINSTANCE res = ShellExecuteW(nullptr, L"explore", p.c_str(), nullptr, nullptr, SW_SHOWNORMAL); - if(std::bit_cast(res) <= 32) - { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open folder in file browser: ", p, "!"); - TP_ERROR(TRAP::Log::FileSystemPrefix, TRAP::Utils::String::GetStrError()); - return false; - } - } - else + if (const auto result = TRAP::Utils::SafeSystem("explorer", { TRAP::Utils::String::CreateUTF8StringFromWideStringWin32(p.native())}, false); !result) { - TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open folder in file browser: ", p, " (COM initialization failed)!"); + TP_ERROR(TRAP::Log::FileSystemPrefix, "Couldn't open externally: ", result.Error()); return false; } @@ -1338,7 +1303,7 @@ namespace TRAP_ASSERT(!p.empty(), "FileSystem::OpenFileInFileBrowserWindows(): Path is empty!"); TRAP_ASSERT(TRAP::FileSystem::IsAbsolute(p), "FileSystem::OpenFileInFileBrowserWindows(): Path is not absolute!"); - const std::wstring openPath = p.wstring(); + const std::wstring& openPath = p.native(); TRAP::Utils::Windows::COMInitializer comInitializer{}; if(comInitializer.IsInitialized()) diff --git a/TRAP/src/FileSystem/FileSystem.h b/TRAP/src/FileSystem/FileSystem.h index 3c21673a9..2bd39332f 100644 --- a/TRAP/src/FileSystem/FileSystem.h +++ b/TRAP/src/FileSystem/FileSystem.h @@ -40,7 +40,7 @@ namespace TRAP::FileSystem { /// @brief Write mode to be used by writing operations. - enum class WriteMode + enum class WriteMode : u8 { Overwrite, Append diff --git a/TRAP/src/Log/Log.h b/TRAP/src/Log/Log.h index 089ac9e13..a1bb2eb7a 100644 --- a/TRAP/src/Log/Log.h +++ b/TRAP/src/Log/Log.h @@ -155,7 +155,7 @@ namespace TRAP /// @threadsafe void Clear() noexcept; - static constexpr auto WindowVersion = "[24w46a4]"; + static constexpr auto WindowVersion = "[24w46b1]"; static constexpr auto WindowPrefix = "[Window] "; static constexpr auto WindowIconPrefix = "[Window][Icon] "; static constexpr auto ConfigPrefix = "[Config] "; diff --git a/TRAP/src/TRAPPCH.h b/TRAP/src/TRAPPCH.h index 6d5c4d54a..bd7da8c58 100644 --- a/TRAP/src/TRAPPCH.h +++ b/TRAP/src/TRAPPCH.h @@ -44,6 +44,7 @@ #include #include #include +#include #include "Core/Backports.h" #include "Core/Types.h" diff --git a/TRAP/src/Utils/Linux.h b/TRAP/src/Utils/Linux.h index 08de5075d..e924ba2d9 100644 --- a/TRAP/src/Utils/Linux.h +++ b/TRAP/src/Utils/Linux.h @@ -9,7 +9,6 @@ #include #include -#include #include #include #include @@ -19,6 +18,7 @@ #include #include #include +#include #include #include #include diff --git a/TRAP/src/Utils/SafeSystem.cpp b/TRAP/src/Utils/SafeSystem.cpp new file mode 100644 index 000000000..089eb35bb --- /dev/null +++ b/TRAP/src/Utils/SafeSystem.cpp @@ -0,0 +1,156 @@ +#include "TRAPPCH.h" +#include "SafeSystem.h" + +#include "Utils/String/String.h" + +namespace +{ +#ifdef TRAP_PLATFORM_LINUX + [[nodiscard]] TRAP::Expected SafeSystemLinux(const std::string& program, + const std::vector& args, + const bool waitForChild) + { + ZoneNamedC(__tracy, tracy::Color::Violet, (GetTRAPProfileSystems() & ProfileSystems::Utils) != ProfileSystems::None && + (GetTRAPProfileSystems() & ProfileSystems::Verbose) != ProfileSystems::None); + + //Prepare arguments fpr execvp + std::vector argsCStr{}; + argsCStr.reserve(args.size() + 2u); //program + args + nullptr + argsCStr.push_back(const_cast(program.c_str())); + for(const auto& arg : args) + argsCStr.push_back(const_cast(arg.c_str())); + argsCStr.push_back(nullptr); + + //Fork a new process + const pid_t pid = fork(); + if(pid == -1) + return TRAP::MakeUnexpected("TRAP::Utils::SafeSystem(): Failed to fork a new process!"); + + if(pid == 0) + { + //Child process: Execute the program + execvp(program.c_str(), argsCStr.data()); + + //If execvp returns, an error occurred + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Failed to execute {} ({})", program, TRAP::Utils::String::GetStrError())); + } + + if(waitForChild) + { + //Parent process: Wait for the child process to complete + i32 status = 0; + if(waitpid(pid, &status, 0) == -1) + { + return TRAP::MakeUnexpected("TRAP::Utils::SafeSystem(): Failed to wait for child process!"); + } + + //Check if the child process exited successfully + if(WIFEXITED(status)) + { + const i32 exitStatus = WEXITSTATUS(status); + if(exitStatus != 0) + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Program exited with non-zero status: {}", exitStatus)); + } + else + return TRAP::MakeUnexpected("TRAP::Utils::SafeSystem(): Child process did not terminate normally!"); + } + + return {}; + } +#endif /*TRAP_PLATFORM_LINUX*/ + + //-------------------------------------------------------------------------------------------------------------------// + +#ifdef TRAP_PLATFORM_WINDOWS + [[nodiscard]] TRAP::Expected SafeSystemWindows(const std::string& program, const std::vector& args, const bool waitForChild) + { + ZoneNamedC(__tracy, tracy::Color::Violet, (GetTRAPProfileSystems() & ProfileSystems::Utils) != ProfileSystems::None && + (GetTRAPProfileSystems() & ProfileSystems::Verbose) != ProfileSystems::None); + + if (program.size() > MAX_PATH) + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): program \"{}\" length is > than MAX_PATH ({})!", program, program.size())); + + std::string command = program; + for (const auto& arg : args) + command += fmt::format(" \"{}\"", arg); //Quote arguments to handle spaces + + std::wstring commandW = TRAP::Utils::String::CreateWideStringFromUTF8StringWin32(command); + + if (commandW.size() > std::numeric_limits::max()) + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): command string is > than 32767!")); + + //Initialize the process startup and process information structures + STARTUPINFOW startupInfo{}; + startupInfo.cb = sizeof(STARTUPINFOW); + PROCESS_INFORMATION processInfo{}; + + //Create process + const bool res = CreateProcessW(nullptr, commandW.data(), nullptr, nullptr, FALSE, 0, nullptr, nullptr, &startupInfo, &processInfo); + if (!res) + { + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Failed to create process ({})!", program, TRAP::Utils::String::GetStrError())); + } + + if (waitForChild) + { + //Wait for process to finish + DWORD waitResult = WaitForSingleObject(processInfo.hProcess, INFINITE); + if (waitResult == WAIT_FAILED) + { + const auto strErr = TRAP::Utils::String::GetStrError(); + CloseHandle(processInfo.hProcess); + CloseHandle(processInfo.hThread); + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Failed to wait for process ({})!", strErr)); + } + + //Check the exit code of the process + DWORD exitCode = 0; + if (!GetExitCodeProcess(processInfo.hProcess, &exitCode)) + { + const auto strErr = TRAP::Utils::String::GetStrError(); + CloseHandle(processInfo.hProcess); + CloseHandle(processInfo.hThread); + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Failed to get exit code ({})!", strErr)); + } + + //Check if the process exited normally + if (exitCode != 0) + { + CloseHandle(processInfo.hProcess); + CloseHandle(processInfo.hThread); + return TRAP::MakeUnexpected(fmt::format("TRAP::Utils::SafeSystem(): Program exited with non-zero status {}", exitCode)); + } + } + + CloseHandle(processInfo.hProcess); + CloseHandle(processInfo.hThread); + + return {}; + } +#endif /*TRAP_PLATFORM_WINDOWS*/ +} + +//-------------------------------------------------------------------------------------------------------------------// +//-------------------------------------------------------------------------------------------------------------------// +//-------------------------------------------------------------------------------------------------------------------// + +[[nodiscard]] TRAP::Expected TRAP::Utils::SafeSystem(const std::string& program, + const std::vector& args, + const bool waitForChild) +{ + ZoneNamedC(__tracy, tracy::Color::Violet, (GetTRAPProfileSystems() & ProfileSystems::Utils) != ProfileSystems::None); + + if(program.empty()) + { + TRAP_ASSERT(false, "TRAP::Utils::SafeSystem(): program variable is empty!"); + return TRAP::MakeUnexpected("program is empty!"); + } + +#ifdef TRAP_PLATFORM_LINUX + return SafeSystemLinux(program, args, waitForChild); +#elif defined(TRAP_PLATFORM_WINDOWS) + return SafeSystemWindows(program, args, waitForChild); +#else + return TRAP::MakeUnexpected("TRAP::Utils::SafeSystem(): Not implemented for this OS!"); +#endif /*TRAP_PLATFORM_LINUX*/ +} diff --git a/TRAP/src/Utils/SafeSystem.h b/TRAP/src/Utils/SafeSystem.h new file mode 100644 index 000000000..2f7bf89a4 --- /dev/null +++ b/TRAP/src/Utils/SafeSystem.h @@ -0,0 +1,15 @@ +#ifndef TRAP_SAFESYSTEM_H +#define TRAP_SAFESYSTEM_H + +#include +#include + +#include "Utils/Expected.h" + +namespace TRAP::Utils +{ + //TODO Document + [[nodiscard]] TRAP::Expected SafeSystem(const std::string& program, const std::vector& args, bool waitForChild); +} + +#endif /*TRAP_SAFESYSTEM_H*/ diff --git a/TRAP/src/Utils/String/String.cpp b/TRAP/src/Utils/String/String.cpp index a6eafa052..f6d984a3c 100644 --- a/TRAP/src/Utils/String/String.cpp +++ b/TRAP/src/Utils/String/String.cpp @@ -114,7 +114,7 @@ return {}; } - result.resize(size); + result.resize(size - 1); //Ignore null terminator if (!WideCharToMultiByte(CP_UTF8, 0, wStr.data(), -1, result.data(), size, nullptr, nullptr)) { TP_ERROR(TRAP::Log::EngineWindowsPrefix, "Failed to convert string to UTF-8"); @@ -141,7 +141,7 @@ return {}; } - result.resize(count); + result.resize(count - 1); //Ignore null terminator if (!MultiByteToWideChar(CP_UTF8, 0, str.data(), -1, result.data(), count)) {