diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d0341c5..2512c22 100644 --- a/.github/workflows/test.yml +++ b/.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: diff --git a/CMakeLists.txt b/CMakeLists.txt index 14c08a5..0bb71f4 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eb79a2e..4fa94e0 100644 --- a/CONTRIBUTING.md +++ b/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) | diff --git a/Makefile b/Makefile index 9a675d2..e1dab63 100644 --- a/Makefile +++ b/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 diff --git a/docs/contributing.md b/docs/contributing.md index eb79a2e..4fa94e0 100644 --- a/docs/contributing.md +++ b/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) | diff --git a/src/mpq/list.cpp b/src/mpq/list.cpp index dae873e..d88de19 100644 --- a/src/mpq/list.cpp +++ b/src/mpq/list.cpp @@ -82,13 +82,17 @@ void ListFiles(HANDLE archive, const std::optional &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 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 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 &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++) { diff --git a/src/util/format.cpp b/src/util/format.cpp index ae484bb..00c368b 100644 --- a/src/util/format.cpp +++ b/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); } diff --git a/src/util/format.h b/src/util/format.h index bba74c3..794793e 100644 --- a/src/util/format.h +++ b/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 diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a1fb8bb..ca54321 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/fixtures/raw_archive.h b/test/fixtures/raw_archive.h index aea1acf..af71c86 100644 --- a/test/fixtures/raw_archive.h +++ b/test/fixtures/raw_archive.h @@ -1,6 +1,7 @@ #ifndef RAW_ARCHIVE_H #define RAW_ARCHIVE_H +#include #include #include #include @@ -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> &entries) { + // Room for every entry and the (listfile) + const auto max_file_count = std::max(16, static_cast(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) { diff --git a/test/functional/test_list.cpp b/test/functional/test_list.cpp index b8fabfb..3f52e9d 100644 --- a/test/functional/test_list.cpp +++ b/test/functional/test_list.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -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 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"; diff --git a/test/unit/mpq/test_list.cpp b/test/unit/mpq/test_list.cpp new file mode 100644 index 0000000..87dca88 --- /dev/null +++ b/test/unit/mpq/test_list.cpp @@ -0,0 +1,54 @@ +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#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> entries = {{"a.txt", 0}}; + std::set 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(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 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(lines.begin(), lines.end()) == expected); + REQUIRE(err.str().empty()); + } +} diff --git a/test/unit/util/test_format.cpp b/test/unit/util/test_format.cpp index 72fe368..7d68f34 100644 --- a/test/unit/util/test_format.cpp +++ b/test/unit/util/test_format.cpp @@ -55,6 +55,21 @@ TEST_CASE("FileTimeToLsTime", "[format]") { const int64_t file_time = LocalFileTime(2025, 7, 29, 14, 31, 5) + intervals_per_second - 1; REQUIRE(FileTimeToLsTime(file_time) == "2025-07-29 14:31:05"); } + + SECTION("prints nothing for a time past the year 9999") { + // Linux converts it, but the five-digit year overflows the buffer; Windows + // refuses to convert it + REQUIRE(FileTimeToLsTime(INT64_MAX).empty()); + } + + SECTION("prints a time before 1970 where the platform can convert it") { +#ifdef _WIN32 + // localtime_s rejects a time before 1970 + REQUIRE(FileTimeToLsTime(1).empty()); +#else + REQUIRE(FileTimeToLsTime(LocalFileTime(1969, 7, 20, 20, 17, 40)) == "1969-07-20 20:17:40"); +#endif + } } TEST_CASE("StormErrorString", "[format]") {