From 0edeebaf4a6857691f38a308017fc3e7a84d38e8 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Mon, 4 May 2026 21:12:53 -0600 Subject: [PATCH 1/4] ArchiveScanner: fix WriteCacheData crash when pool cache outlives .sdp archives CArchiveScanner::WriteCacheData has a verification loop that erases- during-iteration on poolFilesInfo (a spring::unordered_map). Two design defects combine into a deterministic 0xc0000005 crash: A. Empty-map dereference. The loop wraps it == end() back to begin(), then breaks if it == st (the original randomized start). When the loop empties the map mid-iteration, begin() == end() but st points at a smaller bucket index, so the next ExistenceTest(end()) call dereferences past the allocated buckets array. B. Vacuous missing-verdict when allPoolRootDirs is empty. The loop builds allPoolRootDirs by filtering archiveInfos for .sdp entries. With zero .sdp archives in scan dirs (e.g. dev install with empty data/packages/ but a stale archivecache.lua from a prior prod run), ExistenceTest iterates an empty set and returns false unconditionally. Every entry gets erased, then defect A triggers. Fix: - Wrap the verification loop in if (!allPoolRootDirs.empty()) -- with no roots, the only safe action is to leave the cache alone, since the ExistenceTest cannot meaningfully verify anything. - Add if (poolFilesInfo.empty()) break inside the loop body as defense in depth against any other future code path that drains the map mid- iteration. Reproduces deterministically on WSL2 dev installs that point at the prod BAR launcher data dir: prod populated archivecache.lua with 10k+ pool entries, dev clears data/packages/, next launch erases the whole cache and crashes. Regression originally introduced in spring/spring PR #2078 (deadline target for pool files scanning), which replaced a naturally- terminating it != end() loop with a time-bounded wraparound loop. --- rts/System/FileSystem/ArchiveScanner.cpp | 86 ++++++++++++++---------- 1 file changed, 50 insertions(+), 36 deletions(-) diff --git a/rts/System/FileSystem/ArchiveScanner.cpp b/rts/System/FileSystem/ArchiveScanner.cpp index 7a1ab6597d5..e024c263edc 100644 --- a/rts/System/FileSystem/ArchiveScanner.cpp +++ b/rts/System/FileSystem/ArchiveScanner.cpp @@ -1282,44 +1282,58 @@ void CArchiveScanner::WriteCacheData(const std::string& filename) allPoolRootDirs.emplace(CPoolArchive::GetPoolRootDirectory(ai.path + ai.origName)); } - const uint32_t seed = std::chrono::system_clock::now().time_since_epoch().count(); - std::mt19937 generator(seed); - std::uniform_int_distribution distribution(0, poolFilesInfo.size() - 1); - auto startOffset = distribution(generator); - - auto st = poolFilesInfo.begin(); - std::advance(st, startOffset); - auto it = st; - - // we only want to check if the file still exists, we don't check for size / modDate - // this is checked in the checksum code anyway. - const auto ExistenceTest = [&allPoolRootDirs = std::as_const(allPoolRootDirs)](const auto& it) { - for (const auto& poolRootDir : allPoolRootDirs) { - const auto fileName = CPoolArchive::GetPoolFilePath(poolRootDir, it->first); - if (FileSystem::FileExists(fileName)) { - return true; + // No pool archives means we have no roots to verify file existence + // against. Without this guard, ExistenceTest below returns false for + // every entry, the loop erases the entire cache, and the next + // iteration dereferences begin()==end() on the now-empty map. + // Regression originally introduced in PR #2078 ("Add deadline target + // for pool files scanning"), which replaced a naturally-terminating + // `it != end()` loop with a time-bounded wraparound loop. + if (!allPoolRootDirs.empty()) { + const uint32_t seed = std::chrono::system_clock::now().time_since_epoch().count(); + std::mt19937 generator(seed); + std::uniform_int_distribution distribution(0, poolFilesInfo.size() - 1); + auto startOffset = distribution(generator); + + auto st = poolFilesInfo.begin(); + std::advance(st, startOffset); + auto it = st; + + // we only want to check if the file still exists, we don't check for size / modDate + // this is checked in the checksum code anyway. + const auto ExistenceTest = [&allPoolRootDirs = std::as_const(allPoolRootDirs)](const auto& it) { + for (const auto& poolRootDir : allPoolRootDirs) { + const auto fileName = CPoolArchive::GetPoolFilePath(poolRootDir, it->first); + if (FileSystem::FileExists(fileName)) { + return true; + } } + return false; + }; + + // can't spend too much time in this code, thus set the deadline and rely on + // random luck and sheer amount of invocations to eventually remove most if not all + // stale items from the pool cache + static constexpr int64_t MAX_POOL_VERIFICATION_TIME = 1 * 1000; + for (auto t0 = spring_now(), t1 = t0; (t1 - t0).toMilliSecsi() < MAX_POOL_VERIFICATION_TIME; t1 = spring_now()) { + // Map can drain to empty mid-loop (e.g. all entries genuinely + // missing). begin() then equals end(); ExistenceTest(end()) + // would dereference past the allocated buckets array. + if (poolFilesInfo.empty()) + break; + + // cleanup files that got deleted in the meantime + if (ExistenceTest(it)) + ++it; + else + it = poolFilesInfo.erase(it); + + if (it == poolFilesInfo.end()) + it = poolFilesInfo.begin(); //rewind to the very start + + if (it == st) + break; // everything got checked and we're back to the starting iterator } - return false; - }; - - // can't spend too much time in this code, thus set the deadline and rely on - // random luck and sheer amount of invocations to eventually remove most if not all - // stale items from the pool cache - static constexpr int64_t MAX_POOL_VERIFICATION_TIME = 1 * 1000; - for (auto t0 = spring_now(), t1 = t0; (t1 - t0).toMilliSecsi() < MAX_POOL_VERIFICATION_TIME; t1 = spring_now()) { - // cleanup files that got deleted in the meantime - - if (ExistenceTest(it)) - ++it; - else - it = poolFilesInfo.erase(it); - - if (it == poolFilesInfo.end()) - it = poolFilesInfo.begin(); //rewind to the very start - - if (it == st) - break; // everything got checked and we're back to the starting iterator } } From 81e3f7b8f23eb18e719c233a9344c8f05041a064 Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 5 May 2026 08:43:05 -0600 Subject: [PATCH 2/4] Update rts/System/FileSystem/ArchiveScanner.cpp Co-authored-by: sprunk --- rts/System/FileSystem/ArchiveScanner.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/rts/System/FileSystem/ArchiveScanner.cpp b/rts/System/FileSystem/ArchiveScanner.cpp index e024c263edc..b4ebdec1a03 100644 --- a/rts/System/FileSystem/ArchiveScanner.cpp +++ b/rts/System/FileSystem/ArchiveScanner.cpp @@ -1286,9 +1286,6 @@ void CArchiveScanner::WriteCacheData(const std::string& filename) // against. Without this guard, ExistenceTest below returns false for // every entry, the loop erases the entire cache, and the next // iteration dereferences begin()==end() on the now-empty map. - // Regression originally introduced in PR #2078 ("Add deadline target - // for pool files scanning"), which replaced a naturally-terminating - // `it != end()` loop with a time-bounded wraparound loop. if (!allPoolRootDirs.empty()) { const uint32_t seed = std::chrono::system_clock::now().time_since_epoch().count(); std::mt19937 generator(seed); From 8ea2aa4a78cca42b3b894ca98d54905bdb77d6ad Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 5 May 2026 08:43:49 -0600 Subject: [PATCH 3/4] Update rts/System/FileSystem/ArchiveScanner.cpp specific variable conveying what we're protecting against Co-authored-by: sprunk --- rts/System/FileSystem/ArchiveScanner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rts/System/FileSystem/ArchiveScanner.cpp b/rts/System/FileSystem/ArchiveScanner.cpp index b4ebdec1a03..e53dc262e66 100644 --- a/rts/System/FileSystem/ArchiveScanner.cpp +++ b/rts/System/FileSystem/ArchiveScanner.cpp @@ -1316,7 +1316,7 @@ void CArchiveScanner::WriteCacheData(const std::string& filename) // Map can drain to empty mid-loop (e.g. all entries genuinely // missing). begin() then equals end(); ExistenceTest(end()) // would dereference past the allocated buckets array. - if (poolFilesInfo.empty()) + if (it == poolFilesInfo.end()) break; // cleanup files that got deleted in the meantime From fef6037e5b388634eb35cae72d8c8e7b35029334 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Tue, 5 May 2026 09:48:17 -0600 Subject: [PATCH 4/4] ArchiveScanner: re-anchor st when erased in pool verification loop unordered_map::erase invalidates the erased iterator. If st is the entry being erased, the termination check silently stops working and the loop runs to its 1s deadline re-checking survivors. Re-anchor st to erase()'s return value when st is the one erased; wrap to begin() if that landed on end() --- rts/System/FileSystem/ArchiveScanner.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/rts/System/FileSystem/ArchiveScanner.cpp b/rts/System/FileSystem/ArchiveScanner.cpp index e53dc262e66..4b0846eb91e 100644 --- a/rts/System/FileSystem/ArchiveScanner.cpp +++ b/rts/System/FileSystem/ArchiveScanner.cpp @@ -1320,14 +1320,28 @@ void CArchiveScanner::WriteCacheData(const std::string& filename) break; // cleanup files that got deleted in the meantime - if (ExistenceTest(it)) + if (ExistenceTest(it)) { ++it; - else + } else { + // erase invalidates the erased iterator. If we're erasing + // the loop's anchor, re-anchor to the successor so the + // `it == st` termination check below still works; otherwise + // st dangles and we'd burn the full deadline re-checking + // survivors. + const bool erasingStart = (it == st); it = poolFilesInfo.erase(it); + if (erasingStart) + st = it; + } if (it == poolFilesInfo.end()) it = poolFilesInfo.begin(); //rewind to the very start + // st may have been re-anchored to end() above; wrap it too so + // the comparison is meaningful. + if (st == poolFilesInfo.end()) + st = poolFilesInfo.begin(); + if (it == st) break; // everything got checked and we're back to the starting iterator }