From eea6d7cf8c33ae6f9c46d62270342cc4de6baadf Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:08:20 +1200 Subject: [PATCH 1/6] Fixed create crash when adding files from RAM disk and non-NTFS volumes --- src/mpq.cpp | 5 +++- test/test_create.py | 65 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/mpq.cpp b/src/mpq.cpp index 5086d9a..1cd84ca 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -269,7 +269,10 @@ int AddFiles(HANDLE archive, const std::string &input_path, const std::string &p int files_failed = 0; for (const auto &entry : entries) { - fs::path input_file_path = fs::relative(entry, target_path); + // Relativise lexically rather than with fs::relative, which resolves both + // paths through the OS and throws on volumes that cannot report real paths + // (RAM disks, some network shares). + fs::path input_file_path = entry.path().lexically_relative(target_path); std::string archive_file_path; if (path_prefix.empty()) { diff --git a/test/test_create.py b/test/test_create.py index f62b836..d3adfbf 100644 --- a/test/test_create.py +++ b/test/test_create.py @@ -703,6 +703,71 @@ def test_create_mpq_skips_special_files(binary_path, tmp_path): assert name not in listing.stdout, f"Special file {name!r} unexpectedly found in archive listing" +def test_create_mpq_folder_structure_with_trailing_slash(binary_path, tmp_path): + """ + Test MPQ archive creation from a directory path with a trailing slash. + + Regression test for the lexical relative path handling: archive paths must + stay relative to the target directory even when the target path carries a + trailing separator. + + This test checks: + - The archive is created successfully. + - Nested files keep their folder structure in the archive. + """ + source_dir = tmp_path / "src" + (source_dir / "sub").mkdir(parents=True) + (source_dir / "root.txt").write_text("root file") + (source_dir / "sub" / "nested.txt").write_text("nested file") + + output_file = tmp_path / "output.mpq" + + result = subprocess.run( + [str(binary_path), "create", str(source_dir) + "/", "-o", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert output_file.exists(), "MPQ file was not created" + + verify_archive_file_content( + binary_path, output_file, {"enUS root.txt", "enUS sub\\nested.txt"} + ) + + +def test_create_mpq_folder_structure_with_dot_relative_path(binary_path, tmp_path): + """ + Test MPQ archive creation from a "./" prefixed relative path. + + Regression test for the lexical relative path handling: archive paths must + be relative to the target directory, with no leading "./" segments. + + This test checks: + - The archive is created successfully. + - Nested files keep their folder structure in the archive. + """ + source_dir = tmp_path / "src" + (source_dir / "sub").mkdir(parents=True) + (source_dir / "sub" / "nested.txt").write_text("nested file") + + output_file = tmp_path / "output.mpq" + + result = subprocess.run( + [str(binary_path), "create", "./src", "-o", str(output_file)], + cwd=str(tmp_path), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert output_file.exists(), "MPQ file was not created" + + verify_archive_file_content(binary_path, output_file, {"enUS sub\\nested.txt"}) + + def verify_archive_file_content(binary_path, test_file, expected_output): result = subprocess.run( [str(binary_path), "list", str(test_file), "-d", "-p", "locale"], From c6abe1a7991cca63b4c57ade78ba539690eedf82 Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:20:05 +1200 Subject: [PATCH 2/6] Converted extract filesystem calls to non-throwing error code overloads --- src/commands.cpp | 32 ++++++++++++++++++---- src/mpq.cpp | 71 ++++++++++++++++++++++++++++++++++-------------- 2 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index 0c0ebd1..ba3ff3d 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -280,17 +281,36 @@ int HandleExtract(const std::string &target, const std::optional &o const std::optional &locale) { // If no output directory specified, use MPQ path without extension // If output directory specified, create it if it doesn't exist + std::error_code ec; std::string effective_output; if (!output.has_value()) { - fs::path output_path_absolute = fs::canonical(target); - fs::path output_path = output_path_absolute.parent_path() / output_path_absolute.stem(); - effective_output = output_path.u8string(); + fs::path target_path = fs::absolute(target, ec); + if (ec) { + std::cerr << "[!] Failed to resolve archive path: (" << ec.value() << ") " + << ec.message() << ": " << target << std::endl; + return 1; + } + effective_output = (target_path.parent_path() / target_path.stem()).u8string(); } else { effective_output = output.value(); } - if (!fs::create_directory(effective_output) && !fs::is_directory(effective_output)) { - std::cerr << "[!] Failed to create output directory: " << effective_output << std::endl; - return 1; + fs::create_directory(effective_output, ec); + if (ec) { + std::error_code query_ec; + if (!fs::is_directory(effective_output, query_ec)) { + std::cerr << "[!] Failed to create output directory: (" << ec.value() << ") " + << ec.message() << ": " << effective_output << std::endl; + return 1; + } + } + + // ExtractFile can only check for symlink traversal where the OS can resolve + // real paths; warn once up front on volumes where it cannot (RAM disks) + static_cast(fs::canonical(effective_output, ec)); + if (ec) { + std::cout << "[!] Warning: Output directory cannot be fully resolved, symlinks will not " + "be checked during extraction: " + << effective_output << std::endl; } HANDLE archive; diff --git a/src/mpq.cpp b/src/mpq.cpp index 1cd84ca..fd5708b 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -67,6 +68,10 @@ bool SignMpqArchive(HANDLE archive) { return true; } +static bool IsWithinDirectory(const fs::path &base, const fs::path &path) { + return std::mismatch(base.begin(), base.end(), path.begin(), path.end()).first == base.end(); +} + int ExtractFiles(HANDLE archive, const std::string &output, const std::optional &listfile_name, LCID preferred_locale) { SFileSetLocale(preferred_locale); @@ -112,29 +117,55 @@ int ExtractFile(HANDLE archive, const std::string &output, const std::string &fi file_name_string = file_name_path.filename().u8string(); } - // Create output directory - fs::path output_path_absolute = fs::canonical(output); - fs::path output_path_base = - output_path_absolute.parent_path() / output_path_absolute.filename(); - std::filesystem::create_directories(fs::path(output_path_base).parent_path()); - - // Ensure sub-directories for folder-nested files exist before calling canonical - fs::path output_file_path_name = output_path_base / file_name_string; - std::filesystem::create_directories(output_file_path_name.parent_path()); + std::error_code ec; + fs::path output_path_base = fs::absolute(output, ec).lexically_normal(); + if (ec) { + std::cerr << "[!] Failed to resolve output directory: (" << ec.value() << ") " + << ec.message() << ": " << output << std::endl; + return 1; + } + if (output_path_base.filename().empty()) { + output_path_base = output_path_base.parent_path(); + } - // Guard against path traversal attacks: resolve symlinks and ".." with canonical - // (requires path to exist, hence create_directories above) - fs::path resolved_output = - fs::canonical(output_file_path_name.parent_path()) / output_file_path_name.filename(); - if (std::mismatch(output_path_base.begin(), output_path_base.end(), resolved_output.begin(), - resolved_output.end()) - .first != output_path_base.end()) { + // Guard against path traversal attacks in two stages. First lexically, so a + // ".." entry is rejected before anything is created on disk + fs::path output_file_path_name = (output_path_base / file_name_string).lexically_normal(); + if (!IsWithinDirectory(output_path_base, output_file_path_name)) { std::cerr << "[!] Blocked: path traversal attempt detected: " << file_name_string << std::endl; return 1; } - std::string output_file_name{resolved_output.u8string()}; + // Ensure sub-directories for folder-nested files exist before resolving + fs::create_directories(output_file_path_name.parent_path(), ec); + if (ec) { + std::cerr << "[!] Failed to create output directory: (" << ec.value() << ") " + << ec.message() << ": " << output_file_path_name.parent_path().u8string() + << std::endl; + return 1; + } + + // Second, through the OS to also catch symlinks. Volumes that cannot report + // real paths (RAM disks) fail here, in which case the lexical check above is + // the only guard; HandleExtract warns about this once + fs::path resolved_base = fs::canonical(output_path_base, ec); + if (!ec) { + fs::path resolved_output = fs::canonical(output_file_path_name.parent_path(), ec) / + output_file_path_name.filename(); + if (ec) { + std::cerr << "[!] Failed to resolve output path: (" << ec.value() << ") " + << ec.message() << ": " << output_file_path_name.u8string() << std::endl; + return 1; + } + if (!IsWithinDirectory(resolved_base, resolved_output)) { + std::cerr << "[!] Blocked: path traversal attempt detected: " << file_name_string + << std::endl; + return 1; + } + } + + std::string output_file_name{output_file_path_name.u8string()}; if (SFileExtractFile(archive, file_name.c_str(), output_file_name.c_str(), 0)) { std::cout << "[*] Extracted: " << file_name_string << std::endl; @@ -269,9 +300,9 @@ int AddFiles(HANDLE archive, const std::string &input_path, const std::string &p int files_failed = 0; for (const auto &entry : entries) { - // Relativise lexically rather than with fs::relative, which resolves both - // paths through the OS and throws on volumes that cannot report real paths - // (RAM disks, some network shares). + // Determine relative path lexically rather than with fs::relative, which + // resolves paths through the OS and throws on volumes that cannot report + // real paths (RAM disks, some network shares). fs::path input_file_path = entry.path().lexically_relative(target_path); std::string archive_file_path; From ca2e3e29fad104081adc092b219eff9658b7d56f Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:21:51 +1200 Subject: [PATCH 3/6] Removed redundant path resolution and handled file size errors in verify --- src/mpq.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/mpq.cpp b/src/mpq.cpp index fd5708b..ad45ea0 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -829,8 +829,14 @@ int32_t PrintMpqSignature(HANDLE archive, const std::string &target) { int64_t archive_size = GetFileInfo(archive, SFileMpqArchiveSize64); int64_t archive_offset = GetFileInfo(archive, SFileMpqHeaderOffset); - const fs::path archive_path = fs::canonical(target); - std::uintmax_t file_size = fs::file_size(archive_path); + const fs::path archive_path(target); + std::error_code ec; + const std::uintmax_t file_size = fs::file_size(archive_path, ec); + if (ec) { + std::cerr << "[!] Failed to read archive size: (" << ec.value() << ") " << ec.message() + << ": " << target << std::endl; + return -1; + } int64_t signature_length = file_size - archive_offset - archive_size; if (signature_length <= 0) { From 806af87b67b36c27074353f02971b05920e87005 Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:25:27 +1200 Subject: [PATCH 4/6] Converted create and add filesystem calls to error code overloads --- src/commands.cpp | 14 ++++++++------ src/mpq.cpp | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index ba3ff3d..a1c7dca 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -127,11 +127,12 @@ int HandleCreate(const std::string &target, const std::optional &pa if (file_compression_next >= 0) add_overrides.compression_next = static_cast(file_compression_next); - if (fs::is_directory(target)) { + std::error_code ec; + if (fs::is_directory(target, ec)) { const std::string prefix = path.value_or(""); result |= AddFiles(archive, target, prefix, lcid, game_rules, add_overrides); - } else if (fs::is_regular_file(target)) { + } else if (fs::is_regular_file(target, ec)) { std::string archive_path = ResolveArchiveName(target, path); result |= AddFile(archive, target, archive_path, lcid, game_rules, add_overrides); @@ -181,9 +182,10 @@ int HandleAdd(const std::vector &files, const std::string &target, if (file_compression_next >= 0) add_overrides.compression_next = static_cast(file_compression_next); + std::error_code ec; bool has_directory = false; for (const auto &f : files) { - if (fs::is_directory(f)) { + if (fs::is_directory(f, ec)) { has_directory = true; break; } @@ -192,18 +194,18 @@ int HandleAdd(const std::vector &files, const std::string &target, int result = 0; int files_skipped = 0; for (const auto &f : files) { - if (!fs::exists(f)) { + if (!fs::exists(f, ec)) { std::cerr << "[!] Path does not exist: " << f << std::endl; result |= 1; continue; } - if (fs::is_directory(f)) { + if (fs::is_directory(f, ec)) { std::string prefix = path.value_or(""); result |= AddFiles(archive, f, prefix, lcid, game_rules, add_overrides, overwrite, update, &files_skipped); - } else if (fs::is_regular_file(f)) { + } else if (fs::is_regular_file(f, ec)) { const bool treat_as_directory = has_directory || files.size() > 1; std::string archive_path = ResolveArchiveName(f, path, treat_as_directory); result |= AddFile(archive, f, archive_path, lcid, game_rules, add_overrides, overwrite, diff --git a/src/mpq.cpp b/src/mpq.cpp index ad45ea0..f23ed8c 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -182,7 +182,8 @@ int ExtractFile(HANDLE archive, const std::string &output, const std::string &fi HANDLE CreateMpqArchive(const std::string &output_archive_name, const uint32_t file_count, const GameRules &game_rules) { // Check if file already exists - if (fs::exists(output_archive_name)) { + std::error_code ec; + if (fs::exists(output_archive_name, ec)) { std::cerr << "[!] File already exists: " << output_archive_name << " Exiting..." << std::endl; return nullptr; @@ -221,8 +222,9 @@ HANDLE CreateMpqArchive(const std::string &output_archive_name, const uint32_t f static bool ArchivedFileMatches(HANDLE archive, HANDLE file, const fs::path &local_file, std::string &match_reason) { const DWORD archived_size = SFileGetFileSize(file, nullptr); - const uintmax_t disk_size = fs::file_size(local_file); - if (disk_size != static_cast(archived_size)) { + std::error_code ec; + const uintmax_t disk_size = fs::file_size(local_file, ec); + if (ec || disk_size != static_cast(archived_size)) { return false; } @@ -348,7 +350,8 @@ int AddFile(HANDLE archive, const fs::path &local_file, const std::string &archi const CompressionSettingsOverrides &overrides, bool overwrite, bool update, int *skipped) { // Return if file doesn't exist on disk - if (!fs::exists(local_file)) { + std::error_code ec; + if (!fs::exists(local_file, ec)) { std::cerr << "[!] File doesn't exist on disk: " << local_file << std::endl; return 1; } @@ -412,7 +415,12 @@ int AddFile(HANDLE archive, const fs::path &local_file, const std::string &archi } // Get file size for rule matching - const std::uintmax_t raw_file_size = fs::file_size(local_file); + const std::uintmax_t raw_file_size = fs::file_size(local_file, ec); + if (ec) { + std::cerr << "[!] Failed to read file size: (" << ec.value() << ") " << ec.message() << ": " + << local_file << std::endl; + return 1; + } if (raw_file_size > std::numeric_limits::max()) { std::cerr << "[!] Warning: file exceeds 4GB, size-based compression rules may not apply " "correctly: " From 7562fad763419bd9205b09487e6f9b5f51818290 Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:29:28 +1200 Subject: [PATCH 5/6] Added recursive file listing helper to remove duplicate directory walks --- src/commands.cpp | 43 ++++++++++++++++++++++++++++++------------- src/helpers.cpp | 27 ++++++++++++++++++--------- src/helpers.h | 6 +++++- src/mpq.cpp | 27 +++++++-------------------- src/mpq.h | 4 ++-- 5 files changed, 62 insertions(+), 45 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index a1c7dca..56fee6b 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -109,8 +110,23 @@ int HandleCreate(const std::string &target, const std::optional &pa overrides.raw_chunk_size = static_cast(raw_chunk_size); game_rules.OverrideCreateSettings(overrides); - // Determine the number of files we are going to add - uint32_t file_count = CalculateMpqMaxFileValue(target); + // List the files up front: the archive's max file count is fixed at creation + std::error_code ec; + std::vector files; + const bool is_directory = fs::is_directory(target, ec); + if (is_directory) { + files = ListFilesRecursive(target, ec); + if (ec) { + std::cerr << "[!] Failed to list directory: (" << ec.value() << ") " << ec.message() + << ": " << target << std::endl; + return 1; + } + } else if (!fs::is_regular_file(target, ec)) { + std::cerr << "[!] Not a file or directory: " << target << std::endl; + return 1; + } + const uint32_t file_count = + CalculateMpqMaxFileValue(is_directory ? static_cast(files.size()) : 1); // Create the MPQ archive and add files int result = 0; @@ -127,18 +143,12 @@ int HandleCreate(const std::string &target, const std::optional &pa if (file_compression_next >= 0) add_overrides.compression_next = static_cast(file_compression_next); - std::error_code ec; - if (fs::is_directory(target, ec)) { + if (is_directory) { const std::string prefix = path.value_or(""); - result |= AddFiles(archive, target, prefix, lcid, game_rules, add_overrides); - - } else if (fs::is_regular_file(target, ec)) { + result |= AddFiles(archive, files, target, prefix, lcid, game_rules, add_overrides); + } else { std::string archive_path = ResolveArchiveName(target, path); result |= AddFile(archive, target, archive_path, lcid, game_rules, add_overrides); - - } else { - std::cerr << "[!] Not a file or directory: " << target << std::endl; - result |= 1; } if (sign_archive) { @@ -201,9 +211,16 @@ int HandleAdd(const std::vector &files, const std::string &target, } if (fs::is_directory(f, ec)) { + std::vector directory_files = ListFilesRecursive(f, ec); + if (ec) { + std::cerr << "[!] Failed to list directory: (" << ec.value() << ") " << ec.message() + << ": " << f << std::endl; + result |= 1; + continue; + } std::string prefix = path.value_or(""); - result |= AddFiles(archive, f, prefix, lcid, game_rules, add_overrides, overwrite, - update, &files_skipped); + result |= AddFiles(archive, directory_files, f, prefix, lcid, game_rules, add_overrides, + overwrite, update, &files_skipped); } else if (fs::is_regular_file(f, ec)) { const bool treat_as_directory = has_directory || files.size() > 1; diff --git a/src/helpers.cpp b/src/helpers.cpp index b56dcfa..a5f979c 100644 --- a/src/helpers.cpp +++ b/src/helpers.cpp @@ -9,6 +9,8 @@ #include #include #include +#include +#include #ifdef _WIN32 #include @@ -89,18 +91,25 @@ std::string StormErrorString(uint32_t err) { } } -uint32_t CalculateMpqMaxFileValue(const std::string &path) { - uint32_t file_count = 0; - - // Determine the number of files in the target directory, recusively - if (!fs::is_regular_file(path)) { - for (const auto &entry : fs::recursive_directory_iterator(path)) { - if (fs::is_regular_file(entry.path())) { - ++file_count; - } +std::vector ListFilesRecursive(const fs::path &directory, std::error_code &ec) { + std::vector files; + fs::recursive_directory_iterator it(directory, ec); + while (!ec && it != fs::recursive_directory_iterator()) { + if (it->is_regular_file(ec)) { + files.push_back(it->path()); + } + if (!ec) { + it.increment(ec); } } + if (ec) { + return {}; + } + std::sort(files.begin(), files.end()); + return files; +} +uint32_t CalculateMpqMaxFileValue(uint32_t file_count) { // Always add 3 for "special" files file_count += 3; diff --git a/src/helpers.h b/src/helpers.h index ce9ce5d..617f7f1 100644 --- a/src/helpers.h +++ b/src/helpers.h @@ -5,6 +5,8 @@ #include #include #include +#include +#include namespace fs = std::filesystem; @@ -12,7 +14,9 @@ std::string FileTimeToLsTime(int64_t file_time); std::string NormalizeFilePath(const fs::path &path); std::string WindowsifyFilePath(const fs::path &path); std::string StormErrorString(uint32_t err); -uint32_t CalculateMpqMaxFileValue(const std::string &path); + +std::vector ListFilesRecursive(const fs::path &directory, std::error_code &ec); +uint32_t CalculateMpqMaxFileValue(uint32_t file_count); uint32_t NextPowerOfTwo(uint32_t n); void PrintAsBinary(const char *buffer, uint32_t size); diff --git a/src/mpq.cpp b/src/mpq.cpp index f23ed8c..053cfa5 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -280,32 +280,19 @@ static bool ArchivedFileMatches(HANDLE archive, HANDLE file, const fs::path &loc return false; } -int AddFiles(HANDLE archive, const std::string &input_path, const std::string &path_prefix, - LCID locale, const GameRules &game_rules, +int AddFiles(HANDLE archive, const std::vector &files, const fs::path &base_path, + const std::string &path_prefix, LCID locale, const GameRules &game_rules, const CompressionSettingsOverrides &overrides, bool overwrite, bool update, int *skipped) { - fs::path target_path = fs::path(input_path); - - std::vector entries; - for (const auto &entry : fs::recursive_directory_iterator(input_path)) { - if (fs::is_regular_file(entry.path())) { - entries.push_back(entry); - } - } - std::sort(entries.begin(), entries.end(), - [](const fs::directory_entry &a, const fs::directory_entry &b) { - return a.path() < b.path(); - }); - int files_added = 0; int files_skipped = 0; int files_failed = 0; - for (const auto &entry : entries) { + for (const auto &file : files) { // Determine relative path lexically rather than with fs::relative, which // resolves paths through the OS and throws on volumes that cannot report // real paths (RAM disks, some network shares). - fs::path input_file_path = entry.path().lexically_relative(target_path); + fs::path input_file_path = file.lexically_relative(base_path); std::string archive_file_path; if (path_prefix.empty()) { @@ -322,8 +309,8 @@ int AddFiles(HANDLE archive, const std::string &input_path, const std::string &p } int file_skipped = 0; - if (AddFile(archive, entry.path(), archive_file_path, locale, game_rules, overrides, - overwrite, update, &file_skipped) != 0) { + if (AddFile(archive, file, archive_file_path, locale, game_rules, overrides, overwrite, + update, &file_skipped) != 0) { files_failed++; } else if (file_skipped > 0) { files_skipped++; @@ -333,7 +320,7 @@ int AddFiles(HANDLE archive, const std::string &input_path, const std::string &p } if (update) { - std::cout << "[*] For " << input_path << ": " << files_added << " files added, " + std::cout << "[*] For " << base_path.u8string() << ": " << files_added << " files added, " << files_skipped << " files skipped, " << files_failed << " files failed." << std::endl; } diff --git a/src/mpq.h b/src/mpq.h index f1637a4..362628d 100644 --- a/src/mpq.h +++ b/src/mpq.h @@ -21,8 +21,8 @@ int ExtractFile(HANDLE archive, const std::string &output, const std::string &fi bool keep_folder_structure, LCID preferred_locale); HANDLE CreateMpqArchive(const std::string &output_archive_name, uint32_t file_count, const GameRules &game_rules); -int AddFiles(HANDLE archive, const std::string &input_path, const std::string &path_prefix, - LCID locale, const GameRules &game_rules, +int AddFiles(HANDLE archive, const std::vector &files, const fs::path &base_path, + const std::string &path_prefix, LCID locale, const GameRules &game_rules, const CompressionSettingsOverrides &overrides = CompressionSettingsOverrides(), bool overwrite = false, bool update = false, int *skipped = nullptr); int AddFile(HANDLE archive, const fs::path &local_file, const std::string &archive_file_path, From 00ab41d2a58ef00cbaa0a31b6c9d8706b534a573 Mon Sep 17 00:00:00 2001 From: thomaslaurenson Date: Tue, 25 Aug 2026 22:35:41 +1200 Subject: [PATCH 6/6] Skipped dangling symlinks when listing files and handled output path resolution errors --- src/commands.cpp | 9 +++++++-- src/helpers.cpp | 13 +++++++++---- test/test_create.py | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/commands.cpp b/src/commands.cpp index 56fee6b..e19415a 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -61,9 +61,15 @@ int HandleCreate(const std::string &target, const std::optional &pa int64_t stream_flags, int64_t sector_size, int64_t raw_chunk_size, int64_t file_flags1, int64_t file_flags2, int64_t file_flags3, int64_t attr_flags, int64_t file_flags, int64_t file_compression, int64_t file_compression_next) { + std::error_code ec; fs::path output_file_path; if (output.has_value()) { - output_file_path = fs::absolute(output.value()); + output_file_path = fs::absolute(output.value(), ec); + if (ec) { + std::cerr << "[!] Failed to resolve output path: (" << ec.value() << ") " + << ec.message() << ": " << output.value() << std::endl; + return 1; + } } else { output_file_path = fs::path(target); // If the path ends with a separator (e.g. "dir/"), strip the @@ -111,7 +117,6 @@ int HandleCreate(const std::string &target, const std::optional &pa game_rules.OverrideCreateSettings(overrides); // List the files up front: the archive's max file count is fixed at creation - std::error_code ec; std::vector files; const bool is_directory = fs::is_directory(target, ec); if (is_directory) { diff --git a/src/helpers.cpp b/src/helpers.cpp index a5f979c..d9ffda7 100644 --- a/src/helpers.cpp +++ b/src/helpers.cpp @@ -95,12 +95,17 @@ std::vector ListFilesRecursive(const fs::path &directory, std::error_c std::vector files; fs::recursive_directory_iterator it(directory, ec); while (!ec && it != fs::recursive_directory_iterator()) { - if (it->is_regular_file(ec)) { + // A dangling symlink reports not_found rather than a hard error, and is + // skipped like any other non-regular entry + std::error_code status_ec; + const fs::file_status status = it->status(status_ec); + if (fs::is_regular_file(status)) { files.push_back(it->path()); + } else if (status_ec && status.type() != fs::file_type::not_found) { + ec = status_ec; + break; } - if (!ec) { - it.increment(ec); - } + it.increment(ec); } if (ec) { return {}; diff --git a/test/test_create.py b/test/test_create.py index d3adfbf..abd14ee 100644 --- a/test/test_create.py +++ b/test/test_create.py @@ -1,7 +1,10 @@ +import platform import subprocess import shutil from pathlib import Path +import pytest + def test_create_mpq_target_does_not_exist(binary_path, generate_test_files): """ @@ -768,6 +771,35 @@ def test_create_mpq_folder_structure_with_dot_relative_path(binary_path, tmp_pat verify_archive_file_content(binary_path, output_file, {"enUS sub\\nested.txt"}) +@pytest.mark.skipif(platform.system() == "Windows", reason="Symlink creation needs privileges on Windows") +def test_create_mpq_skips_dangling_symlink(binary_path, tmp_path): + """ + Test MPQ archive creation from a directory containing a dangling symlink. + + This test checks: + - The archive is created successfully. + - The dangling symlink is skipped and the regular files are added. + """ + source_dir = tmp_path / "src" + source_dir.mkdir() + (source_dir / "real.txt").write_text("real file") + (source_dir / "dangling.txt").symlink_to(tmp_path / "missing.txt") + + output_file = tmp_path / "output.mpq" + + result = subprocess.run( + [str(binary_path), "create", str(source_dir), "-o", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert output_file.exists(), "MPQ file was not created" + + verify_archive_file_content(binary_path, output_file, {"enUS real.txt"}) + + def verify_archive_file_content(binary_path, test_file, expected_output): result = subprocess.run( [str(binary_path), "list", str(test_file), "-d", "-p", "locale"],