Fix/archive overreads

.github/workflows/test.yml

@@ -58,6 +58,20 @@ jobs:
       - run: make test
       - run: make test_functional
 
+  # A dedicated job, not a step in the one above: sanitizers change code
+  # generation and roughly double runtime, so folding them into the main job
+  # slows every run and leaves a failure ambiguous between the instrumented and
+  # ordinary builds. test_asan configures build/asan itself.
+  test_asan:
+    runs-on: ubuntu-24.04
+    steps:
+      - name: Check out repository code
+        uses: actions/checkout@v6
+        with:
+          submodules: true
+
+      - run: make test_asan
+
   test_windows:
     name: test_windows (${{ matrix.build_type }})
     strategy:

CMakeLists.txt

@@ -18,6 +18,9 @@ set(MPQCLI_INTEGRATION_DATA "${PROJECT_SOURCE_DIR}/test/data/integration" CACHE
 # the same bar: clang-tidy replays these flags and .clang-tidy treats every
 # warning as an error.
 option(MPQCLI_WERROR "Treat compiler warnings as errors" OFF)
+# Off by default: ASan costs roughly 2x runtime and 3x memory. make test_asan
+# turns it on in build/asan, and test.yml runs that as a job of its own.
+option(MPQCLI_ASAN "Build with Address + UB sanitizers" OFF)
 
 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
@@ -44,6 +47,24 @@ if(MSVC)
     add_compile_definitions(__STORMLIB_NO_STATIC_LINK__)
 endif()
 
+# Applied before any target is declared, so every module and test is instrumented
+if(MPQCLI_ASAN)
+    if(MSVC)
+        # MSVC has AddressSanitizer but no UndefinedBehaviorSanitizer, and links
+        # its runtime automatically, so there is no matching add_link_options.
+        add_compile_options(/fsanitize=address /Oy-)
+    else()
+        # -fno-sanitize-recover makes a UBSan finding fail the run; by default it
+        # prints the report and carries on, and the process exits 0
+        add_compile_options(-fsanitize=address,undefined -fno-sanitize-recover=undefined
+                            -fno-omit-frame-pointer -g)
+        add_link_options(-fsanitize=address,undefined)
+        # GCC has no macro that says UBSan is on, and __has_feature arrived
+        # only in GCC 14, so a test that must step around UBSan reads this
+        add_compile_definitions(MPQCLI_SANITIZE_UNDEFINED)
+    endif()
+endif()
+
 if(MPQCLI_BUILD_STATIC)
     set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static")
     set(CMAKE_FIND_LIBRARY_SUFFIXES ".a")
@@ -84,6 +105,14 @@ mark_system(storm)
 if(NOT MSVC)
     set_target_properties(storm PROPERTIES C_STANDARD 11)
 endif()
+# Two UBSan findings in StormLib, each check turned off on storm alone:
+# - alignment: it writes the (attributes) file times through a ULONGLONG
+#   pointer at a 4-byte offset, after the CRC32 array (SFileAttributes.cpp:300)
+# - shift-base: its bundled bzip2 1.0.5 sets bit 31 with 1 << 31 on an int
+#   (the SET_BH macro, bzip2/blocksort.c:205)
+if(MPQCLI_ASAN AND NOT MSVC)
+    target_compile_options(storm PRIVATE -fno-sanitize=alignment,shift-base)
+endif()
 
 # Only app/ parses arguments, so the core never sees CLI11
 if(MPQCLI_BUILD_APP)

CONTRIBUTING.md

@@ -35,6 +35,7 @@ Run `make help` to list all available targets. Common ones:
 | `make test` | Run the unit tests |
 | `make test_functional` | Run the functional tests against the built binary |
 | `make test_all` | Run every test layer in build/dev |
+| `make test_asan` | Run every test layer under Address + UB sanitizers in build/asan |
 | `make fetch_integration_data` | Download the real archives a few functional tests read (needs filenget) |
 | `make check_all` | Run every static check (clang-format, clang-tidy, completion scripts) |
 | `make check_format` | Check formatting only (dry run) |

Makefile

@@ -24,6 +24,9 @@ CLANG_TIDY       ?= $(shell command -v clang-tidy-$(CLANG_VERSION) || echo clang
 # emits confident diagnostics from a broken AST, so it gets its own tree.
 LINT_DIR         ?= build/lint
 
+# Where test_asan has AddressSanitizer write its reports, one file per process
+ASAN_LOG_DIR     ?= build/asan/logs
+
 # Empty on a machine with no GCC, and configure_lint then passes no
 # --gcc-install-dir at all. The && is what keeps it empty: dirname of an empty
 # string is ".", which clang would take as the install directory.
@@ -56,7 +59,6 @@ help: ## Show this help message
 		/^[a-zA-Z_-]+:.*## / {printf "  %-22s %s\n", $$1, $$2}' $(MAKEFILE_LIST)
 
 ##@ BUILD
-
 .PHONY: configure
 configure: ## Configure the cmake build
 	cmake -B build/dev \
@@ -70,7 +72,6 @@ build: configure ## Build the project
 	cmake --build build/dev --parallel $(JOBS)
 
 ##@ TEST
-
 .PHONY: test
 test: build ## Run Catch2 unit tests
 	ctest --test-dir build/dev --output-on-failure --parallel $(JOBS) -L unit
@@ -87,15 +88,32 @@ test_functional: build ## Run Catch2 functional tests against the built binary
 test_all: build ## Run every test layer built into the current configure
 	ctest --test-dir build/dev --output-on-failure --parallel $(JOBS)
 
-# The checksums pin the bytes the tests' expected values were taken from, so a
-# changed or truncated download fails here rather than as a puzzling test failure
+.PHONY: configure_asan
+configure_asan: ## Configure build/asan with Address + UB sanitizers
+	cmake -B build/asan \
+		-DCMAKE_BUILD_TYPE=Debug \
+		-DMPQCLI_ASAN=ON \
+		-DMPQCLI_BUILD_TESTING=ON \
+		-DMPQCLI_BUILD_APP=ON \
+		$(CMAKE_ARGS)
+
+.PHONY: test_asan
+test_asan: configure_asan ## Build and run every test layer under sanitizers
+	cmake --build build/asan --parallel $(JOBS)
+	rm -rf $(ASAN_LOG_DIR) && mkdir -p $(ASAN_LOG_DIR)
+	ASAN_OPTIONS=exitcode=99:log_path=$(abspath $(ASAN_LOG_DIR))/asan \
+	UBSAN_OPTIONS=exitcode=99:print_stacktrace=1 \
+		ctest --test-dir build/asan --output-on-failure --parallel $(JOBS); \
+		status=$$?; \
+		if [ -n "$$(ls -A $(ASAN_LOG_DIR))" ]; then cat $(ASAN_LOG_DIR)/*; exit 1; fi; \
+		exit $$status
+
 .PHONY: fetch_integration_data
 fetch_integration_data: ## Download the Blizzard patches some functional tests read (requires: filenget)
 	$(FILENGET) "$(INTEGRATION_DATA)" '$(INTEGRATION_LINK)'
 	cd "$(INTEGRATION_DATA)" && sha256sum --check "$(CURDIR)/test/integration_data.sha256"
 
 ##@ LINT
-
 .PHONY: format
 format: ## Format all source files with clang-format
 	find $(CPP_DIRS) \( -name "*.cpp" -o -name "*.h" \) \
@@ -135,7 +153,6 @@ check_embed: ## Syntax-check the embedded completion scripts with every shell pr
 	  else echo "[*] pwsh not installed, skipped"; fi
 
 ##@ DOCS
-
 .PHONY: docs_mermaid
 docs_mermaid: ## Fetch mermaid.min.js and mermaid-init.js into the repo root (gitignored)
 	mdbook-mermaid install .
@@ -153,7 +170,6 @@ docs_clean: ## Remove the generated docs site and mermaid assets
 	rm -rf book mermaid.min.js mermaid-init.js
 
 ##@ DOCKER
-
 .PHONY: docker_build
 docker_build: ## Build the Docker image, tagged with the project version
 	docker build -t mpqcli:$(VERSION) .
@@ -163,7 +179,6 @@ docker_run: ## Run the Docker image
 	@docker run -it mpqcli:$(VERSION) version
 
 ##@ GENERATE
-
 # The docs site builds from the committed copy, so this is run deliberately and the
 # result committed rather than being made a prerequisite of docs_build
 .PHONY: gen_docs_changelog
@@ -188,7 +203,6 @@ gen_downloads: ## Update the package downloads badge in README.md from GitHub
 	echo "[*] Updated package downloads badge: $$BADGE ($$DOWNLOADS)"
 
 ##@ GET
-
 .PHONY: get_version
 get_version: ## Print the project version from CMakeLists.txt (fails if absent)
 	@awk '$(VERSION_AWK)' CMakeLists.txt
@@ -215,7 +229,6 @@ get_changelog: ## Print release notes for TAG to stdout (TAG=v1.0.0)
 	printf '%s\n' "$$notes"
 
 ##@ CI
-
 .PHONY: check_all
 check_all: check_format check_lint check_embed ## Run every static check
 

docs/contributing.md

@@ -35,6 +35,7 @@ Run `make help` to list all available targets. Common ones:
 | `make test` | Run the unit tests |
 | `make test_functional` | Run the functional tests against the built binary |
 | `make test_all` | Run every test layer in build/dev |
+| `make test_asan` | Run every test layer under Address + UB sanitizers in build/asan |
 | `make fetch_integration_data` | Download the real archives a few functional tests read (needs filenget) |
 | `make check_all` | Run every static check (clang-format, clang-tidy, completion scripts) |
 | `make check_format` | Check formatting only (dry run) |

src/mpq/list.cpp

@@ -82,13 +82,17 @@ void ListFiles(HANDLE archive, const std::optional<std::filesystem::path> &listf
             }
             seen_file_names.insert(find_data.cFileName);
 
-            // Multiple files can be stored with identical filenames under different locales
-            DWORD max_locales = 32; // Updated in place by SFileEnumLocales
-            std::vector<LCID> file_locale_vec(max_locales);
-            LCID *file_locales = file_locale_vec.data();
-
-            DWORD result =
-                SFileEnumLocales(archive, find_data.cFileName, file_locales, &max_locales, 0);
+            // Multiple files can be stored with identical filenames under different locales.
+            // SFileEnumLocales writes the real count back into max_locales and returns
+            // ERROR_INSUFFICIENT_BUFFER when the buffer is too small, so it is grown to fit.
+            DWORD max_locales = 32;
+            std::vector<LCID> file_locales;
+            DWORD result = ERROR_INSUFFICIENT_BUFFER;
+            while (result == ERROR_INSUFFICIENT_BUFFER) {
+                file_locales.resize(max_locales);
+                result = SFileEnumLocales(archive, find_data.cFileName, file_locales.data(),
+                                          &max_locales, 0);
+            }
 
             if (result == ERROR_INVALID_PARAMETER) {
                 // StormLib rejects a name it cannot resolve (IsPseudoFileName) before
@@ -100,11 +104,6 @@ void ListFiles(HANDLE archive, const std::optional<std::filesystem::path> &listf
             } else if (result == ERROR_INVALID_HANDLE || result == ERROR_NOT_SUPPORTED) {
                 err << "[!] Internal error for file: " << find_data.cFileName << std::endl;
                 continue;
-
-            } else if (result == ERROR_INSUFFICIENT_BUFFER) {
-                err << "[!] There are more than " << max_locales
-                    << " locales for the file: " << find_data.cFileName << ". Will only list the "
-                    << max_locales << " first files." << std::endl;
             }
 
             for (DWORD i = 0; i < max_locales; i++) {

src/util/format.cpp

@@ -57,15 +57,24 @@ std::string FileTimeToLsTime(int64_t file_time) {
         return "";
     }
     constexpr int64_t epoch_diff = 11644473600LL;
-    int64_t unix_time = (file_time / 10000000) - epoch_diff;
+    const std::time_t unix_time = (file_time / 10000000) - epoch_diff;
+    // The time comes from the archive, so a conversion can fail: localtime_s
+    // rejects a time before 1970, and a year past 9999 overflows buf, which
+    // strftime then leaves unterminated
     char buf[20];
     struct tm tm_buf;
 #ifdef _WIN32
-    localtime_s(&tm_buf, &unix_time);
+    if (localtime_s(&tm_buf, &unix_time) != 0) {
+        return "";
+    }
 #else
-    localtime_r(&unix_time, &tm_buf);
+    if (localtime_r(&unix_time, &tm_buf) == nullptr) {
+        return "";
+    }
 #endif
-    strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", &tm_buf);
+    if (strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", &tm_buf) == 0) {
+        return "";
+    }
     return std::string(buf);
 }
 

src/util/format.h

@@ -7,7 +7,11 @@
 
 namespace mpqcli {
 
+/// Formats a FILETIME as the local time, "YYYY-MM-DD HH:MM:SS"
+///
+/// Zero, and a time the platform cannot format, give an empty string.
 std::string FileTimeToLsTime(int64_t file_time);
+
 /// Describes a StormLib error code in words that are the same on every platform
 ///
 /// The code's value differs by platform, so it is left out. A code with no words of its

test/CMakeLists.txt

@@ -24,6 +24,7 @@ add_executable(mpqcli_unit_tests
     unit/gamerules/test_rules.cpp
     unit/mpq/test_extract.cpp
     unit/mpq/test_flags.cpp
+    unit/mpq/test_list.cpp
     unit/mpq/test_query.cpp
     unit/test_errors.cpp
     unit/util/test_capacity.cpp

test/fixtures/raw_archive.h

@@ -1,6 +1,7 @@
 #ifndef RAW_ARCHIVE_H
 #define RAW_ARCHIVE_H
 
+#include <algorithm>
 #include <filesystem>
 #include <stdexcept>
 #include <string>
@@ -19,9 +20,11 @@ namespace mpqcli::testing {
 /// std::runtime_error when StormLib refuses, since that is a broken fixture.
 inline void WriteRawArchive(const std::filesystem::path &path,
                             const std::vector<std::pair<std::string, LCID>> &entries) {
+    // Room for every entry and the (listfile)
+    const auto max_file_count = std::max<DWORD>(16, static_cast<DWORD>(entries.size()) + 1);
     HANDLE archive = nullptr;
-    if (!SFileCreateArchive(path.string().c_str(), MPQ_CREATE_LISTFILE | MPQ_CREATE_ARCHIVE_V1, 16,
-                            &archive)) {
+    if (!SFileCreateArchive(path.string().c_str(), MPQ_CREATE_LISTFILE | MPQ_CREATE_ARCHIVE_V1,
+                            max_file_count, &archive)) {
         throw std::runtime_error("WriteRawArchive: cannot create " + path.string());
     }
     for (const auto &[name, locale] : entries) {

test/functional/test_list.cpp

@@ -5,6 +5,7 @@
 #include <catch2/matchers/catch_matchers.hpp>
 #include <catch2/matchers/catch_matchers_string.hpp>
 #include <cstddef>
+#include <cstdio>
 #include <filesystem>
 #include <set>
 #include <string>
@@ -381,6 +382,32 @@ TEST_CASE("list: shows a file stored under several locales once per locale", "[l
     }
 }
 
+TEST_CASE("list: lists every locale of a name with more than 32 of them", "[list]") {
+    TempDir dir;
+    const auto file = dir.Path() / "a.txt";
+    const auto archive = dir.Path() / "locales.mpq";
+    WriteFile(file, "a");
+    REQUIRE(Run(mpqcli_binary, {"create", "--format-version", "1", archive.string(), file.string()})
+                .returncode == 0);
+
+    // The default locale and 33 others, none of them in the locale table, so each
+    // prints as its own four hexadecimal digits
+    std::set<std::string> expected = {"enUS  a.txt"};
+    for (int locale = 1; locale <= 33; locale++) {
+        char hex[5];
+        std::snprintf(hex, sizeof(hex), "%04X", locale);
+        REQUIRE(Run(mpqcli_binary, {"add", "--locale", hex, archive.string(), file.string()})
+                    .returncode == 0);
+        expected.insert(std::string(hex) + "  a.txt");
+    }
+
+    auto result = Run(mpqcli_binary, {"list", "-q", "locale", archive.string()});
+
+    REQUIRE(result.returncode == 0);
+    REQUIRE(result.stderr_output.empty());
+    RequireLines(result.stdout_output, expected);
+}
+
 TEST_CASE("list: prints nested entries with the archive's backslash separator", "[list]") {
     TempDir dir;
     const auto source_dir = dir.Path() / "src";

test/unit/mpq/test_list.cpp

@@ -0,0 +1,54 @@
+#include <catch2/catch_test_macros.hpp>
+#include <cstdio>
+#include <optional>
+#include <set>
+#include <sstream>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include <StormLib.h>
+
+#include "mpq/archive.h"
+#include "mpq/list.h"
+#include "raw_archive.h"
+#include "temp_dir.h"
+
+using mpqcli::Archive;
+using mpqcli::ListFiles;
+using mpqcli::testing::TempDir;
+using mpqcli::testing::WriteRawArchive;
+
+TEST_CASE("ListFiles", "[list]") {
+    TempDir dir;
+    std::ostringstream out;
+    std::ostringstream err;
+
+    SECTION("lists every locale of a name with more than 32 of them") {
+        // The default locale and 33 others, none of them in the locale table, so each
+        // prints as its own four hexadecimal digits
+        std::vector<std::pair<std::string, LCID>> entries = {{"a.txt", 0}};
+        std::set<std::string> expected = {"enUS  a.txt"};
+        for (LCID locale = 1; locale <= 33; locale++) {
+            entries.emplace_back("a.txt", locale);
+            char hex[5];
+            std::snprintf(hex, sizeof(hex), "%04X", static_cast<unsigned>(locale));
+            expected.insert(std::string(hex) + "  a.txt");
+        }
+        const auto path = dir.Path() / "locales.mpq";
+        WriteRawArchive(path, entries);
+        const auto archive = Archive::Open(path, MPQ_OPEN_READ_ONLY);
+
+        ListFiles(archive.Handle(), std::nullopt, false, true, {"locale"}, out, err);
+
+        std::vector<std::string> lines;
+        std::istringstream in(out.str());
+        for (std::string line; std::getline(in, line);) {
+            lines.push_back(line);
+        }
+        CAPTURE(lines);
+        REQUIRE(lines.size() == 34);
+        REQUIRE(std::set<std::string>(lines.begin(), lines.end()) == expected);
+        REQUIRE(err.str().empty());
+    }
+}