From b716ef16a0b5ab6d13f6d4c1114c396986169e38 Mon Sep 17 00:00:00 2001 From: bjoern-sobolewski <33577559+bjoern-sobolewski@users.noreply.github.com> Date: Wed, 9 Sep 2026 12:51:54 +0200 Subject: [PATCH] Fix PBO path resolution Support paths with a leading slash and make PBO path lookup case-insensitive to match Arma3 behavior. Check both the archive and member name when detecting recursive includes so files in the same PBO can include one another. Fix the filename shown in read errors and return a nonzero CLI exit code when an archive cannot be opened or a member read fails. --- src/cli/cli.cpp | 12 ++- src/cli/cli.hpp | 15 ++- src/fileio/default.cpp | 154 ++++++++++------------------ src/fileio/default.h | 5 +- src/parser/preprocessor/default.cpp | 5 +- src/runtime/logging.h | 2 +- src/rvutils/pbofile.hpp | 4 +- tests/CMakeLists.txt | 1 + tests/fileio/CMakeLists.txt | 8 ++ tests/fileio/pbo.py | 137 +++++++++++++++++++++++++ 10 files changed, 233 insertions(+), 110 deletions(-) create mode 100644 tests/fileio/CMakeLists.txt create mode 100644 tests/fileio/pbo.py diff --git a/src/cli/cli.cpp b/src/cli/cli.cpp index ac60d372..757877ef 100644 --- a/src/cli/cli.cpp +++ b/src/cli/cli.cpp @@ -553,13 +553,19 @@ int cli::run(size_t argc, const char** argv) { std::filesystem::path path(*rit); path = path.lexically_normal(); - rvutils::pbo::pbofile pbo(path); + rvutils::pbo::pbofile pbo; + pbo.open(path); if (!pbo.good()) { std::cout << "Failed to parse PBO '" << path << "'."; + m_good = false; + continue; + } + if (!static_cast(m_runtime.fileio()).add_pbo_mapping(pbo)) + { + m_good = false; continue; } - static_cast(m_runtime.fileio()).add_pbo_mapping(pbo); rvutils::pbo::pbofile::reader reader; if (pbo.read("config.cpp", reader)) { @@ -765,6 +771,7 @@ int cli::run(size_t argc, const char** argv) handle_files(); + m_good = m_good && !m_logger.has_fatal_error(); if (!m_good || m_parse_only) { if (verbose() && !m_good) { std::cout << "Exiting due to error." << std::endl; } @@ -909,6 +916,7 @@ int cli::run(size_t argc, const char** argv) } } while (!m_automated && !m_runtime.is_exit_requested()); } + if (m_logger.has_fatal_error()) { return -1; } auto exitcode = m_runtime.exit_code(); if (exitcode.has_value()) { diff --git a/src/cli/cli.hpp b/src/cli/cli.hpp index 677fce4e..2fc00d00 100644 --- a/src/cli/cli.hpp +++ b/src/cli/cli.hpp @@ -16,7 +16,18 @@ namespace TCLAP class cli { - StdOutLogger m_logger; + class cli_logger : public StdOutLogger + { + bool m_fatal_error = false; + public: + void log(const LogMessageBase& message) override + { + if (message.getLevel() == loglevel::fatal) { m_fatal_error = true; } + StdOutLogger::log(message); + } + bool has_fatal_error() const { return m_fatal_error; } + }; + cli_logger m_logger; sqf::runtime::runtime m_runtime; bool m_cli_file; bool m_parse_only; @@ -32,4 +43,4 @@ class cli int run(size_t argc, const char** argv); bool verbose() const { return m_logger.isEnabled(loglevel::verbose); } -}; \ No newline at end of file +}; diff --git a/src/fileio/default.cpp b/src/fileio/default.cpp index 50f54d21..8a0ddc2d 100644 --- a/src/fileio/default.cpp +++ b/src/fileio/default.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #ifdef DF__SQF_FILEIO__TRACE_REESOLVE #include @@ -31,6 +32,18 @@ inline bool file_exists(std::filesystem::path p) return infile.good(); } +static std::string pbo_virtual_path(std::string path) +{ + std::replace(path.begin(), path.end(), '\\', '/'); + auto first = path.find_first_not_of('/'); + path = first == std::string::npos ? "/" : "/" + path.substr(first); + path = std::filesystem::path(path).lexically_normal().generic_string(); + std::transform(path.begin(), path.end(), path.begin(), [](unsigned char c) { + return static_cast(c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c); + }); + return path; +} + std::optional sqf::fileio::impl_default::get_info_virtual(std::string_view viewVirtual, sqf::runtime::fileio::pathinfo current) const { // Create & Cleanse stuff @@ -48,6 +61,19 @@ std::optional sqf::fileio::impl_default::get_inf return {}; } + auto pbo_path = virt; + if (virt.front() != '/' && !current.virtual_.empty()) + { + auto parent = current.virtual_; + std::replace(parent.begin(), parent.end(), '\\', '/'); + pbo_path = std::filesystem::path(parent).parent_path().generic_string() + "/" + virt; + } + auto pbo_entry = m_pbo_files.find(pbo_virtual_path(pbo_path)); + if (pbo_entry != m_pbo_files.end()) + { + return pbo_entry->second; + } + // Prepare local tree-node list std::vector> nodes; nodes.push_back(m_virtual_file_root); @@ -202,96 +228,49 @@ std::optional sqf::fileio::impl_default::get_inf return {}; } -void sqf::fileio::impl_default::add_pbo_mapping(rvutils::pbo::pbofile& pbo) +bool sqf::fileio::impl_default::add_pbo_mapping(rvutils::pbo::pbofile& pbo) { - if (m_pbos.find(pbo.path().string()) != m_pbos.end()) + const auto archive = pbo.path().lexically_normal().string(); + if (m_pbos.find(archive) != m_pbos.end()) { log(logmessage::fileio::PBOAlreadyAdded(pbo.path().string())); - return; + return true; } auto prefix_optional = pbo.attribute("prefix"); if (!prefix_optional.has_value()) { log(logmessage::fileio::PBOHasNoPrefixAttribute(pbo.path().string())); - return; + return false; } - m_pbos[pbo.path().lexically_normal().string()] = pbo; - std::filesystem::path prefix(*prefix_optional); - - - // We need to register all files with the virtual pathing - for (auto& file_desc : pbo.files()) + m_pbos[archive] = pbo; + for (const auto& file : pbo.files()) { - // Construct file path - auto file_path = (prefix / file_desc.name).lexically_normal(); - auto path_iter = file_path.begin(); - - // Navigate to last available virtual file node from root node - auto nav = m_virtual_file_root->next.find(path_iter->string()); - if (nav == m_virtual_file_root->next.end()) - { - // Setup path_element for inserting - auto el = std::make_shared(); - auto tmp = path_iter; tmp++; // Fancy hack because for some reason std::filesystem::path lacks `path + number` - el->virtual_full = std::accumulate(file_path.begin(), tmp, std::filesystem::path{}, std::divides{}).string(); - el->physical.push_back(pbo.path().string()); - - // Insert next nav node - auto [iter, success] = m_virtual_file_root->next.emplace(std::make_pair(path_iter->string(), el)); - nav = iter; - - ++path_iter; - } - else + const auto virtual_path = pbo_virtual_path(*prefix_optional + "/" + file.name); + auto [entry, inserted] = m_pbo_files.emplace(virtual_path, + sqf::runtime::fileio::pathinfo(archive, file.name, virtual_path)); + if (!inserted) { - ++path_iter; - std::unordered_map>::iterator nextnav; - while ((nextnav = nav->second->next.find(path_iter->string())) != nav->second->next.end() && path_iter != file_path.end()) - { - nav = nextnav; - path_iter++; - } - } - - // Check we did not iterated the whole of file_path - if (path_iter == file_path.end()) - { - log(logmessage::fileio::PBOFileAlreadyRegistered(pbo.path().string(), file_path.string())); - continue; - } - - // Insert until we hit the file-element - while (path_iter != file_path.end()) - { - // Setup path_element for inserting - auto el = std::make_shared(); - auto tmp = path_iter; tmp++; // Fancy hack because for some reason std::filesystem::path lacks `path + number` - el->virtual_full = std::accumulate(file_path.begin(), tmp, std::filesystem::path{}, std::divides{}).string(); - el->physical.push_back(pbo.path().string()); - - // Insert next nav node - auto [iter, success] = nav->second->next.emplace(std::make_pair(path_iter->string(), el)); - nav = iter; - - ++path_iter; + log(logmessage::fileio::PBOFileAlreadyRegistered(archive, virtual_path)); } } + return true; } -void sqf::fileio::impl_default::add_pbo_mapping(std::filesystem::path p) +bool sqf::fileio::impl_default::add_pbo_mapping(std::filesystem::path p) { if (m_pbos.find(p.string()) != m_pbos.end()) { log(logmessage::fileio::PBOAlreadyAdded(p.string())); - return; + return true; } - rvutils::pbo::pbofile pbo(p); + rvutils::pbo::pbofile pbo; + pbo.open(p); if (!pbo.good()) { log(logmessage::fileio::FailedToParsePBO(p.string())); - return; + return false; } - add_pbo_mapping(pbo); + return add_pbo_mapping(pbo); } void sqf::fileio::impl_default::add_mapping(std::string_view viewPhysical, std::string_view viewVirtual) @@ -337,45 +316,20 @@ void sqf::fileio::impl_default::add_mapping(std::string_view viewPhysical, std:: std::string sqf::fileio::impl_default::read_file(sqf::runtime::fileio::pathinfo info) const { std::filesystem::path physical = info.physical; - if (physical.extension() == ".pbo") + if (!info.additional.empty()) { - auto res = m_pbos.find(physical.lexically_normal().string()); - if (res == m_pbos.end()) - { - log(logmessage::fileio::PBOFileNotFound(physical.lexically_normal().string())); - return {}; - } - else + auto archive = m_pbos.find(physical.lexically_normal().string()); + rvutils::pbo::pbofile::reader reader; + if (archive != m_pbos.end() && archive->second.read(info.additional, reader)) { - auto prefix_optional = res->second.attribute("prefix"); - if (!prefix_optional.has_value()) - { - log(logmessage::fileio::PBOHasNoPrefixAttribute(physical.lexically_normal().string())); - return {}; - } - auto prefix = prefix_optional.value(); - auto pbo_path = info.virtual_; - - if (pbo_path.length() > prefix.length() + 1) - { - pbo_path = pbo_path.substr(prefix.length() + 1); - } - std::transform(pbo_path.begin(), pbo_path.end(), pbo_path.begin(), [](char c) -> char { return c == '/' ? '\\' : c; }); - - rvutils::pbo::pbofile::reader reader; - if (res->second.read(pbo_path, reader)) - { - std::string str; - str.resize(reader.descriptor().size); - reader.read(str.data(), reader.descriptor().size); - return str; - } - else + std::string contents(reader.descriptor().size, '\0'); + if (reader.read(contents.data(), contents.size()) == contents.size()) { - log(logmessage::fileio::PBOFailedToReadFile(physical.lexically_normal().string(), pbo_path)); - return {}; + return contents; } } + log(logmessage::fileio::PBOFailedToReadFile(info.physical, info.additional)); + throw std::runtime_error("Failed to read PBO member '" + info.additional + "' from '" + info.physical + "'."); } else { diff --git a/src/fileio/default.h b/src/fileio/default.h index 01106c7c..8a3101fb 100644 --- a/src/fileio/default.h +++ b/src/fileio/default.h @@ -24,6 +24,7 @@ namespace sqf::fileio std::vector> m_path_elements; std::unordered_map m_pbos; + std::unordered_map m_pbo_files; void get_directories_recursive(std::vector& paths, const std::shared_ptr& el) const @@ -59,8 +60,8 @@ namespace sqf::fileio m_virtual_file_root->virtual_full = "/"; m_path_elements.push_back(m_virtual_file_root); } - void add_pbo_mapping(rvutils::pbo::pbofile& pbo); - void add_pbo_mapping(std::filesystem::path p); + bool add_pbo_mapping(rvutils::pbo::pbofile& pbo); + bool add_pbo_mapping(std::filesystem::path p); virtual std::optional get_info(std::string_view view, sqf::runtime::fileio::pathinfo current) const override { auto res = get_info_virtual(view, current); diff --git a/src/parser/preprocessor/default.cpp b/src/parser/preprocessor/default.cpp index 9b7f1f2d..4cecea57 100644 --- a/src/parser/preprocessor/default.cpp +++ b/src/parser/preprocessor/default.cpp @@ -712,7 +712,10 @@ std::string sqf::parser::preprocessor::impl_default::instance::parse_ppinstructi } const auto& physical = include_path_info->physical; auto res = std::find_if(m_file_scopes.begin(), m_file_scopes.end(), - [physical](file_scope& parent) -> bool { return parent.path.physical == physical; }); + [&include_path_info](file_scope& parent) -> bool { + return parent.path.physical == include_path_info->physical && + parent.path.additional == include_path_info->additional; + }); if (res != m_file_scopes.end()) { m_errflag = true; diff --git a/src/runtime/logging.h b/src/runtime/logging.h index f1c3a4e5..5ef62202 100644 --- a/src/runtime/logging.h +++ b/src/runtime/logging.h @@ -2090,7 +2090,7 @@ namespace logmessage { PBOFailedToReadFile(std::string_view path, std::string_view file) : FileIoBase(level, errorCode, {}), m_path(path), - m_file(path) + m_file(file) { } [[nodiscard]] std::string formatMessage() const override; diff --git a/src/rvutils/pbofile.hpp b/src/rvutils/pbofile.hpp index 93eb1fb0..d3432c45 100644 --- a/src/rvutils/pbofile.hpp +++ b/src/rvutils/pbofile.hpp @@ -157,7 +157,7 @@ namespace rvutils::pbo } remaining = remaining < bytes ? remaining : bytes; m_file.read(arr, remaining); - return (size_t)remaining; + return static_cast(m_file.gcount()); } std::streampos tell() { @@ -1465,4 +1465,4 @@ namespace rvutils::pbo return true; } }; -} \ No newline at end of file +} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 354ba9e8..b8d2b4b3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -33,3 +33,4 @@ endfunction() add_subdirectory(sqf) add_subdirectory(preprocess) add_subdirectory(cba) +add_subdirectory(fileio) diff --git a/tests/fileio/CMakeLists.txt b/tests/fileio/CMakeLists.txt new file mode 100644 index 00000000..fdd2badf --- /dev/null +++ b/tests/fileio/CMakeLists.txt @@ -0,0 +1,8 @@ +find_package(Python3 COMPONENTS Interpreter QUIET) +if(Python3_Interpreter_FOUND) + add_test(NAME fileio.pbo + COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/pbo.py $) + set_tests_properties(fileio.pbo PROPERTIES TIMEOUT 60) +else() + message(STATUS "Python3 not found: PBO CLI regression tests are unavailable") +endif() diff --git a/tests/fileio/pbo.py b/tests/fileio/pbo.py new file mode 100644 index 00000000..e1941822 --- /dev/null +++ b/tests/fileio/pbo.py @@ -0,0 +1,137 @@ +import pathlib +import struct +import subprocess +import sys +import tempfile +import unittest + +VM = str(pathlib.Path(sys.argv.pop(1)).resolve()) + +def write_pbo(path, files, prefix=r"a3\ui_f", extra_size=0): + header = b"\0" + struct.pack("<5I", 0x56657273, 0, 0, 0, 0) + if prefix is not None: + header += b"prefix\0" + prefix.encode("ascii") + b"\0" + header += b"\0" + body = b"" + for name, content in files.items(): + payload = content.encode("ascii") + header += name.encode("ascii") + b"\0" + struct.pack("<5I", 0, 0, 0, 0, len(payload) + extra_size) + body += payload + path.write_bytes(header + b"\0" + struct.pack("<5I", 0, 0, 0, 0, 0) + body) + return path + + +class PboTests(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory(prefix="sqfvm-pbo-") + self.addCleanup(self.directory.cleanup) + self.root = pathlib.Path(self.directory.name) + self.pbo = write_pbo(self.root / "fixture.pbo", { + r"hpp\defineDIKCodes.inc": "#define DIK_ESCAPE 1\n", + r"hpp\Base.inc": "#define PBO_VALUE 42\n", + r"hpp\Empty.inc": "", + r"hpp\Outer.inc": '#include "sub\\Inner.inc"\n', + r"hpp\sub\Inner.inc": '#include "..\\Base.inc"\n', + r"hpp\Rooted.inc": '#include "\\A3\\UI_F\\HPP\\BASE.INC"\n', + r"hpp\Cycle.inc": '#include "./CYCLE.INC"\n', + r"hpp\Missing.inc": '#include "DoesNotExist.inc"\n', + r"text\Message.txt": "PBO contents", + }) + + def run_vm(self, code, archives=None, mappings=()): + command = [VM, "-a", "--suppress-welcome", "--no-work-print", "--no-load-executable-dir"] + for archive in [self.pbo] if archives is None else archives: + command += ["--input-pbo", str(archive)] + for mapping in mappings: + command += ["-v", mapping] + return subprocess.run(command + ["--sqf", code], cwd=self.root, + capture_output=True, text=True, timeout=5) + + def assert_success(self, process, expected): + output = process.stdout + process.stderr + self.assertEqual(process.returncode, 0, output) + self.assertNotRegex(output, r"\[(?:FAT|ERR|WRN)\]") + self.assertIn("[DIAG_LOG] " + str(expected), output) + + def test_rooted_unrooted_mixed_case_and_separators(self): + for path in [r"a3\ui_f\hpp\defineDIKCodes.inc", r"\a3\ui_f\hpp\defineDIKCodes.inc", + "a3/ui_f/hpp/defineDIKCodes.inc", "/a3/ui_f/hpp/defineDIKCodes.inc", + r"\A3\UI_F\HPP\DEFINEDIKCODES.INC", "///A3/UI_F/HPP/defineDIKCodes.inc", + "/a3/ui_f/hpp/./sub/../defineDIKCodes.inc"]: + with self.subTest(path=path): + self.assert_success(self.run_vm('#include "' + path + '"\ndiag_log DIK_ESCAPE;'), 1) + + def test_loadfile_uses_original_member_name(self): + for path in [r"\A3\UI_F\TEXT\MESSAGE.TXT", "a3/ui_f/text/Message.txt"]: + with self.subTest(path=path): + self.assert_success(self.run_vm('diag_log loadFile "' + path + '";'), "PBO contents") + + def test_nested_relative_includes_in_one_archive(self): + self.assert_success(self.run_vm('#include "\\a3\\ui_f\\hpp\\Outer.inc"\ndiag_log PBO_VALUE;'), 42) + + def test_nested_rooted_include_in_one_archive(self): + self.assert_success(self.run_vm('#include "\\a3\\ui_f\\hpp\\Rooted.inc"\ndiag_log PBO_VALUE;'), 42) + + def test_empty_member_is_valid(self): + self.assert_success(self.run_vm('#include "\\a3\\ui_f\\hpp\\Empty.inc"\ndiag_log 42;'), 42) + + def test_real_include_cycle_is_still_rejected(self): + result = self.run_vm('#include "\\a3\\ui_f\\hpp\\Cycle.inc"\ndiag_log 42;') + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertIn("[ERR]", result.stdout) + self.assertNotIn("[DIAG_LOG] 42", result.stdout) + + def test_missing_nested_include_is_not_an_empty_success(self): + result = self.run_vm('#include "\\a3\\ui_f\\hpp\\Missing.inc"\ndiag_log 42;') + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertIn("DoesNotExist.inc", result.stdout) + self.assertNotIn("[DIAG_LOG] 42", result.stdout) + + def test_prefix_must_match_a_complete_component(self): + result = self.run_vm('#include "\\a3\\ui_f_other\\hpp\\Base.inc"\ndiag_log 42;') + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertNotIn("[DIAG_LOG] 42", result.stdout) + + def test_several_archives_can_share_a_prefix(self): + second = write_pbo(self.root / "second.pbo", {r"hpp\Second.inc": '#include "Base.inc"\n'}) + self.assert_success(self.run_vm('#include "\\a3\\ui_f\\hpp\\Second.inc"\ndiag_log PBO_VALUE;', + archives=[self.pbo, second]), 42) + + def test_forward_slash_prefix(self): + archive = write_pbo(self.root / "forward.pbo", {"hpp/Forward.inc": "#define PBO_VALUE 42\n"}, prefix="/a3/forward/") + self.assert_success(self.run_vm('#include "\\A3\\FORWARD\\HPP\\FORWARD.INC"\ndiag_log PBO_VALUE;', archives=[archive]), 42) + + def test_missing_archive_fails_even_without_an_include(self): + missing = self.root / "missing.pbo" + result = self.run_vm('diag_log 42;', archives=[missing]) + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertNotIn("[DIAG_LOG] 42", result.stdout) + self.assertFalse(missing.exists(), "Mounting a missing PBO must not create it") + + def test_archive_without_prefix_is_not_silently_ignored(self): + archive = write_pbo(self.root / "no-prefix.pbo", {"empty.inc": ""}, prefix=None) + result = self.run_vm('diag_log 42;', archives=[archive]) + self.assertNotEqual(result.returncode, 0, result.stdout) + + def test_truncated_include_reports_member_and_fails(self): + archive = write_pbo(self.root / "truncated.pbo", {"broken.inc": ""}, extra_size=100) + result = self.run_vm('#include "\\a3\\ui_f\\broken.inc"\ndiag_log 42;', archives=[archive]) + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertIn("broken.inc", result.stdout + result.stderr) + self.assertNotIn("[DIAG_LOG] 42", result.stdout) + + def test_truncated_runtime_read_cannot_exit_successfully(self): + archive = write_pbo(self.root / "truncated.pbo", {"broken.inc": ""}, extra_size=100) + result = self.run_vm('diag_log loadFile "\\a3\\ui_f\\broken.inc"; exitcode__ 0;', archives=[archive]) + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertIn("[FAT]", result.stdout) + self.assertIn("broken.inc", result.stdout) + + def test_directory_mapping_continues_to_resolve_files(self): + (self.root / "Local.inc").write_text("#define LOCAL_VALUE 7\n", encoding="ascii") + self.assert_success(self.run_vm('#include "\\local\\Local.inc"\ndiag_log LOCAL_VALUE;', + mappings=[str(self.root) + "|/local"]), 7) + + +if __name__ == "__main__": + unittest.main()