From 8168e26d930d230a043914dd39ac8ec4e59e3e54 Mon Sep 17 00:00:00 2001 From: DanS Date: Tue, 13 Oct 2026 22:03:00 -0500 Subject: [PATCH] perf(updater): read the downloaded archive in one bounded allocation Before verifying a downloaded archive, both updaters slurped it into a string with an istreambuf_iterator, which grows the buffer repeatedly and reallocates as it reads a multi-megabyte file. It also placed no upper bound on the size it would read into memory. - A shared readFileExact helper sizes the file first and reads it in a single allocation, failing closed if the file is missing, empty, larger than a caller-supplied cap, or comes up short. A truncated read can never reach the SHA-256 / signature verifier. - Both updaters use it with the existing max-archive cap. Tests cover a good read, a read exactly at the cap, over the cap, an empty file, and a missing file. Co-Authored-By: Claude Opus 4.8 --- src/util/daemon_updater.cpp | 10 ++-------- src/util/http_download.cpp | 15 +++++++++++++++ src/util/http_download.h | 6 ++++++ src/util/xmrig_updater.cpp | 10 ++-------- tests/test_phase4.cpp | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/util/daemon_updater.cpp b/src/util/daemon_updater.cpp index cb060b2..f314d6a 100644 --- a/src/util/daemon_updater.cpp +++ b/src/util/daemon_updater.cpp @@ -281,14 +281,8 @@ void DaemonUpdater::installResolved(const std::string& targetDir, const DaemonRe setProgress(State::Verifying, "Verifying download…"); std::string bytes; // kept in scope through extraction so we extract the VERIFIED buffer (I-01) { - std::ifstream f(zipPath, std::ios::binary); - if (!f) { - fs::remove(zipPath, ec); - setProgress(State::Failed, "Could not read the downloaded archive."); - return; - } - bytes.assign(std::istreambuf_iterator(f), std::istreambuf_iterator()); - if (f.bad()) { + if (!readFileExact(zipPath, bytes, + static_cast(kMaxArchiveBytes))) { fs::remove(zipPath, ec); setProgress(State::Failed, "Could not read the downloaded archive."); return; diff --git a/src/util/http_download.cpp b/src/util/http_download.cpp index dee255a..a96d4a2 100644 --- a/src/util/http_download.cpp +++ b/src/util/http_download.cpp @@ -8,8 +8,10 @@ #include +#include #include #include +#include #include #include @@ -155,5 +157,18 @@ bool httpDownloadToFile(const std::string& url, const std::string& destPath, return true; } +bool readFileExact(const std::string& path, std::string& out, std::uintmax_t maxBytes) +{ + out.clear(); + std::ifstream f(path, std::ios::binary | std::ios::ate); + if (!f) return false; + const std::streamoff size = f.tellg(); + if (size <= 0 || static_cast(size) > maxBytes) return false; + f.seekg(0, std::ios::beg); + out.resize(static_cast(size)); + if (!f.read(out.data(), size) || f.gcount() != size) { out.clear(); return false; } + return true; +} + } // namespace util } // namespace dragonx diff --git a/src/util/http_download.h b/src/util/http_download.h index a5ef032..b5c010d 100644 --- a/src/util/http_download.h +++ b/src/util/http_download.h @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -63,5 +64,10 @@ bool httpDownloadToFile(const std::string& url, const std::string& destPath, const char* logTag, long long maxBytes, const std::function& onProgress); +// Read the whole file at `path` into `out` in one allocation. Fails closed (returns false, `out` +// cleared) if the file is missing, empty, larger than `maxBytes`, or a short read occurs. Used by the +// updaters before SHA-256 / signature verification, so a truncated read can never reach the verifier. +bool readFileExact(const std::string& path, std::string& out, std::uintmax_t maxBytes); + } // namespace util } // namespace dragonx diff --git a/src/util/xmrig_updater.cpp b/src/util/xmrig_updater.cpp index 47b8bcc..3e0c53b 100644 --- a/src/util/xmrig_updater.cpp +++ b/src/util/xmrig_updater.cpp @@ -269,14 +269,8 @@ void XmrigUpdater::installResolved(const std::string& targetDir, const XmrigRele setProgress(State::Verifying, "Verifying download…"); std::string bytes; // kept in scope through extraction so we extract the VERIFIED buffer (I-01) { - std::ifstream f(zipPath, std::ios::binary); - if (!f) { - fs::remove(zipPath, ec); - setProgress(State::Failed, "Could not read the downloaded archive."); - return; - } - bytes.assign(std::istreambuf_iterator(f), std::istreambuf_iterator()); - if (f.bad()) { + if (!readFileExact(zipPath, bytes, + static_cast(kMaxArchiveBytes))) { fs::remove(zipPath, ec); setProgress(State::Failed, "Could not read the downloaded archive."); return; diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index e9bd1d2..2dfd766 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -18413,12 +18413,45 @@ void testMigrationWantsBalanceScan() EXPECT_FALSE(migrationWantsBalanceScan(false, true, 1, -1.0)); // syncing defers the automatic scan } +void testReadFileExact() +{ + namespace fs = std::filesystem; + const fs::path dir = fs::temp_directory_path() / "dragonx_readfileexact_test"; + std::error_code ec; + fs::create_directories(dir, ec); + const std::string good = (dir / "good.bin").string(); + const std::string empty = (dir / "empty.bin").string(); + std::string payload; + for (int i = 0; i < 1000; ++i) payload.push_back(static_cast(i % 251)); + payload.push_back('\0'); + { std::ofstream o(good, std::ios::binary); o.write(payload.data(), static_cast(payload.size())); } + { std::ofstream o(empty, std::ios::binary); } + + std::string out = "stale"; + EXPECT_TRUE(dragonx::util::readFileExact(good, out, 1 << 20)); + EXPECT_TRUE(out == payload); + EXPECT_TRUE(dragonx::util::readFileExact(good, out, payload.size())); // exactly at the cap + EXPECT_TRUE(out == payload); + + out = "stale"; + EXPECT_FALSE(dragonx::util::readFileExact(good, out, payload.size() - 1)); // over the cap + EXPECT_TRUE(out.empty()); + out = "stale"; + EXPECT_FALSE(dragonx::util::readFileExact(empty, out, 1 << 20)); + EXPECT_TRUE(out.empty()); + out = "stale"; + EXPECT_FALSE(dragonx::util::readFileExact((dir / "missing.bin").string(), out, 1 << 20)); + EXPECT_TRUE(out.empty()); + fs::remove_all(dir, ec); +} + int main() { testConnectionConfig(); testClassifyRpcFailure(); testClassifyUnlockFailure(); testHttpSinkAccept(); + testReadFileExact(); testSecureWipeAndRemove(); testFormatHttpError(); testRpcFailureKindOf();