Skip to content

Commit

Permalink
Utils Added SafeSystem() as a replacement for unsafe system()/shell c…
Browse files Browse the repository at this point in the history
…alls 11/17/2024 | 24w46b1
  • Loading branch information
GamesTrap committed Nov 17, 2024
1 parent f8ab963 commit 0ad1c7f
Show file tree
Hide file tree
Showing 10 changed files with 197 additions and 54 deletions.
6 changes: 6 additions & 0 deletions SITREPS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion TRAP/src/Core/Base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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{};

//-------------------------------------------------------------------------------------------------------------------//

Expand Down
61 changes: 13 additions & 48 deletions TRAP/src/FileSystem/FileSystem.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "TRAPPCH.h"
#include "FileSystem.h"

#include "Utils/SafeSystem.h"
#include "Utils/String/String.h"
#include "Application.h"

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -1260,8 +1249,6 @@ namespace
return TRAP::NullOpt;
}

folderPath.pop_back(); //Remove the extra null byte

return folderPath;
}

Expand All @@ -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<INT_PTR>(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;
}

Expand All @@ -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<INT_PTR>(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;
}

Expand All @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion TRAP/src/FileSystem/FileSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion TRAP/src/Log/Log.h
Original file line number Diff line number Diff line change
Expand Up @@ -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] ";
Expand Down
1 change: 1 addition & 0 deletions TRAP/src/TRAPPCH.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include <source_location>
#include <csignal>
#include <ranges>
#include <regex>

#include "Core/Backports.h"
#include "Core/Types.h"
Expand Down
2 changes: 1 addition & 1 deletion TRAP/src/Utils/Linux.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

#include <linux/input.h>
#include <linux/limits.h>
#include <regex.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/inotify.h>
Expand All @@ -19,6 +18,7 @@
#include <sys/timerfd.h>
#include <sys/mman.h>
#include <sys/file.h>
#include <sys/wait.h>
#include <fcntl.h>
#include <dirent.h>
#include <unistd.h>
Expand Down
156 changes: 156 additions & 0 deletions TRAP/src/Utils/SafeSystem.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
#include "TRAPPCH.h"
#include "SafeSystem.h"

#include "Utils/String/String.h"

namespace
{
#ifdef TRAP_PLATFORM_LINUX
[[nodiscard]] TRAP::Expected<void, std::string> SafeSystemLinux(const std::string& program,
const std::vector<std::string>& 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<char*> argsCStr{};
argsCStr.reserve(args.size() + 2u); //program + args + nullptr
argsCStr.push_back(const_cast<char*>(program.c_str()));
for(const auto& arg : args)
argsCStr.push_back(const_cast<char*>(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<void, std::string> SafeSystemWindows(const std::string& program, const std::vector<std::string>& 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<i16>::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<void, std::string> TRAP::Utils::SafeSystem(const std::string& program,
const std::vector<std::string>& 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*/
}
15 changes: 15 additions & 0 deletions TRAP/src/Utils/SafeSystem.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#ifndef TRAP_SAFESYSTEM_H
#define TRAP_SAFESYSTEM_H

#include <string>
#include <span>

#include "Utils/Expected.h"

namespace TRAP::Utils
{
//TODO Document
[[nodiscard]] TRAP::Expected<void, std::string> SafeSystem(const std::string& program, const std::vector<std::string>& args, bool waitForChild);
}

#endif /*TRAP_SAFESYSTEM_H*/
4 changes: 2 additions & 2 deletions TRAP/src/Utils/String/String.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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))
{
Expand Down

0 comments on commit 0ad1c7f

Please sign in to comment.