From 6072966eb67e8eb8c38ec46c03c6fc7e8f47a09b Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 00:52:04 -0400 Subject: [PATCH 1/6] hi-res: mobile-safe budget, per-texture cap, and in-place .zip packs Rework the HiResPack engine so a pack is survivable on memory-tight platforms and can be read straight out of a .zip: - Decoded-RGBA8 LRU budget is platform-scaled and runtime-settable (gHiResTextures.CacheBudgetMB): 512 MB desktop / 128 MB Android default, applied in Init() once CVars load. LruCache gains SetBudget/EvictToBudget. - Per-texture upscale cap (kMaxPackTexels, mobile only) rejects a single oversize PNG so it can't blow the budget or the uncompressed GPU upload -- the LRU never evicts its just-inserted tail, so a lone over-budget entry would otherwise exceed the cap outright. - mods/ indexes .zip packs in place via libzip + stbi_load_from_memory (no extraction); loose PNGs and zip members share the same hash grammar, only the decode source differs. This is the distributed pack format. - Wrap the mods/ scan so a filesystem error (locked subdir, symlink loop, file removed mid-walk) disables the pack instead of crashing boot. - Fix the stale "~256 MB" budget figure in the Lookup() docstring. Co-Authored-By: Claude Opus 4.8 (1M context) --- port/hires/HiResPack.cpp | 245 +++++++++++++++++++++++++++++++-------- port/hires/HiResPack.h | 53 ++++++++- 2 files changed, 249 insertions(+), 49 deletions(-) diff --git a/port/hires/HiResPack.cpp b/port/hires/HiResPack.cpp index 6dfc53bf..cc2aecd5 100644 --- a/port/hires/HiResPack.cpp +++ b/port/hires/HiResPack.cpp @@ -10,6 +10,12 @@ // here to pick up the function declarations. #include +// libzip — already on the include path via libultraship.h → classes.h → +// O2rArchive.h (the engine's .o2r loader is libzip-based). Used here so a +// pack can be dropped into mods/ as a .zip and read in place, with no +// extraction step (the distributed pack format on every platform). +#include + #include #include #include @@ -22,6 +28,7 @@ #include #include #include +#include namespace ssb64::hires { @@ -45,8 +52,24 @@ struct HashKeyHasher { } }; -// Index of all parsed pack PNGs. Phase 3 reads it from Lookup(). -std::unordered_map gIndex; +// Where a parsed pack PNG lives. A pack entry is either a loose file on disk +// (member empty) or a member inside a .zip dropped in mods/ (member = the +// entry name inside the archive at `container`). Same HashKey grammar either +// way — only the decode source differs (stbi_load vs stbi_load_from_memory). +struct PackEntry { + std::string container; // loose: the .png path; zip: the .zip path + std::string member; // empty for loose; entry name within the zip + bool inZip() const noexcept { return !member.empty(); } +}; + +// Index of all parsed pack PNGs (loose + zip members). Lookup() reads it. +std::unordered_map gIndex; + +// Open zip handles, keyed by .zip path, kept alive for the process so we don't +// re-parse a pack's central directory on every first-decode. Single-threaded +// (the hook runs on the game/render thread), so one shared handle per zip is +// safe to reuse sequentially. +std::unordered_map gOpenZips; std::string gModsRoot; @@ -156,8 +179,25 @@ class LruCache { mList.emplace_back(k, std::move(tex)); mIndex[k] = std::prev(mList.end()); mBytes += addBytes; - // Evict from the LRU end (front) until back under budget. Never - // evict the tail — that's what we just inserted. + EvictToBudget(); + } + + // Re-point the budget after construction. The cache is built at static-init + // time (before CVars load), so HiResPack::Init reads the + // gHiResTextures.CacheBudgetMB CVar and calls this once config is up. + void SetBudget(size_t budgetBytes) { + mBudget = budgetBytes; + EvictToBudget(); + } + + size_t Bytes() const noexcept { return mBytes; } + size_t Budget() const noexcept { return mBudget; } + +private: + // Evict from the LRU end (front) until back under budget. Never evict the + // tail (most-recently inserted) — Lookup returns a pointer into it that + // must stay valid through the immediately-following UploadTexture call. + void EvictToBudget() { while (mBytes > mBudget && mList.size() > 1) { auto& front = mList.front(); mBytes -= front.second.rgba.size(); @@ -166,17 +206,15 @@ class LruCache { } } - size_t Bytes() const noexcept { return mBytes; } - -private: std::list mList; std::unordered_map::iterator, HashKeyHasher> mIndex; size_t mBytes = 0; size_t mBudget; }; -constexpr size_t kDefaultLruBudget = 512u * 1024u * 1024u; // 512 MB -LruCache gLru{ kDefaultLruBudget }; +// Initial budget from the platform default in HiResPack.h; HiResPack::Init +// re-points it from the gHiResTextures.CacheBudgetMB CVar once config loads. +LruCache gLru{ (size_t)kDefaultLruBudgetMB * 1024u * 1024u }; bool IsHexDigit(char c) noexcept { return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f'); @@ -257,6 +295,82 @@ std::optional ParseFilename(std::string_view filename) { return key; } +// Basename (after the last '/' or '\\') of a path or zip entry name. +std::string_view Basename(std::string_view p) noexcept { + size_t slash = p.find_last_of("/\\"); + return slash == std::string_view::npos ? p : p.substr(slash + 1); +} + +// True if `name` ends in "." (case-insensitive). `ext` is 3 lowercase chars. +bool HasExt(std::string_view name, const char* ext) noexcept { + if (name.size() < 4) return false; + std::string_view e = name.substr(name.size() - 4); + return e[0] == '.' && (e[1] | 0x20) == ext[0] && (e[2] | 0x20) == ext[1] && (e[3] | 0x20) == ext[2]; +} + +// Insert a parsed entry, applying the "last scan wins" collision rule (matches +// the loose-file ordering) and updating stats. Shared by both scanners. +void IndexEntry(const HashKey& key, PackEntry entry, PackStats& stats) { + auto it = gIndex.find(key); + if (it == gIndex.end()) { + gIndex.emplace(key, std::move(entry)); + stats.indexedTextures++; + } else { + it->second = std::move(entry); // last-scanned wins + stats.collisions++; + } +} + +// Open a pack .zip read-only and cache the handle for the process lifetime so +// we don't re-parse its central directory on every first-decode. A null handle +// is cached too, so a broken zip isn't retried. +zip_t* OpenZipCached(const std::string& path) { + if (auto it = gOpenZips.find(path); it != gOpenZips.end()) return it->second; + int err = 0; + zip_t* z = zip_open(path.c_str(), ZIP_RDONLY, &err); + if (z == nullptr) { + zip_error_t ze; + zip_error_init_with_code(&ze, err); + port_log("HiResPack: cannot open zip %s (%s)\n", path.c_str(), zip_error_strerror(&ze)); + zip_error_fini(&ze); + } + gOpenZips[path] = z; + return z; +} + +// Enumerate a .zip's entries and index every member whose basename matches the +// pack grammar. The central-directory read is one shot — far cheaper than the +// recursive directory walk a loose pack needs (and it sidesteps the per-file +// scoped-storage traversal cost on Android). +void ScanZip(const std::string& zipPath, PackStats& stats) { + zip_t* z = OpenZipCached(zipPath); + if (z == nullptr) return; + zip_int64_t n = zip_get_num_entries(z, 0); + for (zip_int64_t i = 0; i < n; i++) { + const char* name = zip_get_name(z, i, 0); + if (name == nullptr) continue; + auto key = ParseFilename(Basename(name)); + if (!key) { stats.skippedFilenames++; continue; } + IndexEntry(*key, PackEntry{zipPath, std::string(name)}, stats); + } +} + +// Read a zip member fully into `out`. Returns false on any libzip error. +bool ReadZipMember(const std::string& zipPath, const std::string& member, + std::vector& out) { + zip_t* z = OpenZipCached(zipPath); + if (z == nullptr) return false; + zip_stat_t st; + zip_stat_init(&st); + if (zip_stat(z, member.c_str(), 0, &st) != 0 || !(st.valid & ZIP_STAT_SIZE)) return false; + zip_file_t* zf = zip_fopen(z, member.c_str(), 0); + if (zf == nullptr) return false; + out.resize((size_t)st.size); + zip_int64_t rd = zip_fread(zf, out.data(), st.size); + zip_fclose(zf); + return rd >= 0 && (zip_uint64_t)rd == st.size; +} + } // namespace HiResPack& HiResPack::Get() { @@ -271,6 +385,11 @@ const char* HiResPack::ModsRoot() const noexcept { bool HiResPack::Init() { mStats = {}; gIndex.clear(); + // Close any zip handles from a prior Init before re-scanning. + for (auto& [path, z] : gOpenZips) { + if (z != nullptr) zip_close(z); + } + gOpenZips.clear(); // Resolve /mods alongside BattleShip.o2r and ssb64_save.bin. // Same convention as port_save.cpp. @@ -282,51 +401,60 @@ bool HiResPack::Init() { return false; } + // Apply the decoded-RGBA8 LRU budget now that config (CVars) is loaded. + // Platform default (HiResPack.h) unless the user overrode it; floored so a + // too-small value can't make the cache thrash by re-decoding every miss. + int budgetMB = CVarGetInteger("gHiResTextures.CacheBudgetMB", kDefaultLruBudgetMB); + if (budgetMB < kMinLruBudgetMB) budgetMB = kMinLruBudgetMB; + gLru.SetBudget((size_t)budgetMB * 1024u * 1024u); + port_log("HiResPack: decoded-RGBA8 LRU budget = %d MB%s\n", budgetMB, + kMaxPackTexels ? " (mobile per-texture upscale cap active)" : ""); + if (!Directory::Exists(gModsRoot)) { port_log("HiResPack: %s does not exist; create it and drop a pack inside to enable\n", gModsRoot.c_str()); return false; } - auto files = Directory::ListFiles(gModsRoot); // recursive - std::sort(files.begin(), files.end()); // deterministic collision-winner - - for (const std::string& path : files) { - mStats.scannedFiles++; - - // Extract the basename for parsing. - size_t slash = path.find_last_of("/\\"); - std::string_view name = (slash == std::string::npos) - ? std::string_view(path) - : std::string_view(path).substr(slash + 1); - - // Cheap pre-filter: must end in .png. - if (name.size() < 5) continue; - std::string_view ext = name.substr(name.size() - 4); - if (ext.size() != 4 || ext[0] != '.' - || (ext[1] | 0x20) != 'p' || (ext[2] | 0x20) != 'n' || (ext[3] | 0x20) != 'g') { - continue; - } - - auto key = ParseFilename(name); - if (!key) { - mStats.skippedFilenames++; - continue; - } - - auto [it, inserted] = gIndex.try_emplace(*key, path); - if (!inserted) { - mStats.collisions++; - it->second = path; // last-scanned (alphabetically last) wins - } else { - mStats.indexedTextures++; + // ListFiles' recursive iterator throws on a bad mods/ (locked subdir, + // symlink loop, a file removed mid-walk) — catch it so a junk folder just + // disables the pack instead of taking down boot. + try { + auto files = Directory::ListFiles(gModsRoot); // recursive + std::sort(files.begin(), files.end()); // deterministic collision-winner + + for (const std::string& path : files) { + mStats.scannedFiles++; + std::string_view name = Basename(path); + + // A .zip pack is read in place: enumerate + index its members. This + // is the distributed pack format — desktop users drop the zip into + // mods/, and the Android importer copies the downloaded zip here. + if (HasExt(name, "zip")) { + ScanZip(path, mStats); + continue; + } + + // Otherwise index a loose .png (member empty → decoded via stbi_load). + if (!HasExt(name, "png")) continue; + auto key = ParseFilename(name); + if (!key) { + mStats.skippedFilenames++; + continue; + } + IndexEntry(*key, PackEntry{path, std::string()}, mStats); } + } catch (const std::exception& e) { + port_log("HiResPack: cannot scan %s (%s); hi-res pack disabled this run\n", + gModsRoot.c_str(), e.what()); + gIndex.clear(); + return false; } - port_log("HiResPack: scanned %s — %zu files, %zu indexed, %zu unparsed PNGs, %zu hash collisions\n", + port_log("HiResPack: scanned %s — %zu files, %zu indexed, %zu unparsed, %zu hash collisions, %zu zip(s)\n", gModsRoot.c_str(), mStats.scannedFiles, mStats.indexedTextures, - mStats.skippedFilenames, mStats.collisions); + mStats.skippedFilenames, mStats.collisions, gOpenZips.size()); return true; } @@ -378,18 +506,43 @@ const DecodedTexture* HiResPack::Lookup(uint8_t fmt, uint8_t siz, return nullptr; } + const PackEntry& entry = it->second; int w = 0, h = 0, ch = 0; - uint8_t* raw = stbi_load(it->second.c_str(), &w, &h, &ch, 4); + uint8_t* raw = nullptr; + if (entry.inZip()) { + // Decode straight from the zip member — no extraction to disk. + std::vector bytes; + if (ReadZipMember(entry.container, entry.member, bytes)) { + raw = stbi_load_from_memory(bytes.data(), (int)bytes.size(), &w, &h, &ch, 4); + } + } else { + raw = stbi_load(entry.container.c_str(), &w, &h, &ch, 4); + } if (raw == nullptr || w <= 0 || h <= 0 || w > 65535 || h > 65535) { if (raw) stbi_image_free(raw); - port_log("HiResPack: stbi_load failed for %s (%s)\n", - it->second.c_str(), stbi_failure_reason() ? stbi_failure_reason() : "?"); + port_log("HiResPack: decode failed for %s%s%s (%s)\n", + entry.container.c_str(), entry.inZip() ? " :: " : "", + entry.inZip() ? entry.member.c_str() : "", + stbi_failure_reason() ? stbi_failure_reason() : "?"); // Drop the entry so we don't keep retrying every cache miss. gIndex.erase(it); mLookupStats.decodeFails++; return nullptr; } + // Mobile per-texture upscale cap (kMaxPackTexels = 0 → uncapped on desktop). + // A single oversize PNG would blow the LRU budget (the just-inserted tail is + // never evicted) and balloon the uncompressed GPU upload, so reject it and + // fall back to the native texture. Drop the index entry so we don't re-decode + // the same monster on every cache miss. + if (kMaxPackTexels != 0 && (size_t)w * (size_t)h > kMaxPackTexels) { + stbi_image_free(raw); + port_log("HiResPack: %s decodes to %dx%d (> mobile %u-texel cap) — using native texture\n", + entry.container.c_str(), w, h, (unsigned)kMaxPackTexels); + gIndex.erase(it); + return nullptr; + } + DecodedTexture tex; tex.w = (uint16_t)w; tex.h = (uint16_t)h; diff --git a/port/hires/HiResPack.h b/port/hires/HiResPack.h index c39a3dc2..9cea7427 100644 --- a/port/hires/HiResPack.h +++ b/port/hires/HiResPack.h @@ -7,6 +7,13 @@ * * ###[_].png * + * A pack may be either a folder of loose PNGs OR a .zip dropped straight into + * mods/ — the distributed pack format. Zips are read in place (libzip + + * stbi_load_from_memory at decode time), so there is no extraction step on + * any platform: desktop users drop the zip into mods/, and the Android + * importer copies the downloaded zip there. Loose PNGs and zip members are + * indexed identically; only the decode source differs. + * * The hash key is a CRC32-IEEE over the *decoded* RGBA8 image — the same * tightly-packed pixel buffer the GPU receives at upload time. This makes * the key independent of N64 source byte layout (Sprite/Bitmap/raw/CI all @@ -18,8 +25,8 @@ * (rgba8CRC, fmt, siz). PNG decoding is deferred to first lookup; Init() * only builds the index. * - * Pack folders work additively — drop multiple subfolders under mods/ and - * the union of their PNGs is indexed. If two PNGs hash-collide the later + * Packs work additively — drop multiple subfolders and/or zips under mods/ + * and the union of their PNGs is indexed. If two PNGs hash-collide the later * scan wins (alphabetical order). */ @@ -29,6 +36,44 @@ namespace ssb64::hires { +// ── Platform-scaled tuning (shared by HiResPack.cpp / HiResHook.cpp / +// PortMenu.cpp so the runtime default and the menu widget default agree) ── + +// Master-enable CVar (gHiResTextures.Enabled) default. Hi-res substitution +// is on by default on desktop but OPT-IN on Android: a pack's decoded-RGBA8 +// working set (see kDefaultLruBudgetMB) plus its uncompressed GPU uploads can +// push a phone past the OS low-memory-killer threshold, so a mobile user must +// enable it deliberately rather than have a dropped-in pack allocate silently. +#if defined(__ANDROID__) +inline constexpr int kHiResEnabledDefault = 0; +#else +inline constexpr int kHiResEnabledDefault = 1; +#endif + +// Decoded-RGBA8 LRU budget default, in MB (overridable at runtime via the +// gHiResTextures.CacheBudgetMB CVar, read in HiResPack::Init). Desktop can +// afford a large cache; Android runs under a far tighter per-app footprint +// (the LMK reaps the foreground app well below desktop RAM limits), so it +// defaults much lower. Floored at kMinLruBudgetMB so a too-small value can't +// make the cache thrash (re-decoding on every miss stalls the render thread). +#if defined(__ANDROID__) +inline constexpr int kDefaultLruBudgetMB = 128; +#else +inline constexpr int kDefaultLruBudgetMB = 512; +#endif +inline constexpr int kMinLruBudgetMB = 16; + +// Per-texture upscale ceiling, in decoded pixels. A pack PNG decoding to more +// than this many texels is rejected (the native texture renders instead) so a +// single pathological upscale can't blow the budget / GPU upload in one shot — +// the LRU never evicts its just-inserted tail, so a lone over-budget entry +// would otherwise exceed the cap outright. 0 = uncapped (desktop). +#if defined(__ANDROID__) +inline constexpr uint32_t kMaxPackTexels = 2048u * 2048u; // 16 MB RGBA8 +#else +inline constexpr uint32_t kMaxPackTexels = 0u; +#endif + struct DecodedTexture { std::vector rgba; // tightly packed RGBA8, w * h * 4 bytes uint16_t w = 0; @@ -78,7 +123,9 @@ class HiResPack { /* Hash the decoded RGBA8 image (CRC32-IEEE over `width*height*4` bytes) * and return a substitute decoded RGBA8 buffer if the pack contains a * matching PNG. Decode of the pack PNG is lazy + memoized; a small LRU - * caps total decoded RAM at ~256 MB. The returned pointer is owned by + * caps total decoded RAM at kDefaultLruBudgetMB (512 MB desktop / 128 MB + * Android, overridable via the gHiResTextures.CacheBudgetMB CVar). The + * returned pointer is owned by * the LRU and stays valid through the calling frame's UploadTexture * (eviction only fires on subsequent Lookup() calls that miss the * cache, never during this call). Returns nullptr on miss / decode From 528de332f6a8b216b5e73b9bc28757b092896577 Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 00:52:04 -0400 Subject: [PATCH 2/6] hi-res: opt-in master enable on Android + menu wiring - gHiResTextures.Enabled defaults via kHiResEnabledDefault: on for desktop, OFF/opt-in on Android so a dropped-in pack can't silently allocate a large decoded working set on a phone. Hook and menu share the constant so the runtime default and the menu widget agree. - PortMenu: platform-default checkbox, an Android memory-cost note, and the dev dump-tooling (source/miss dumps) gated off touch UI. Co-Authored-By: Claude Opus 4.8 (1M context) --- port/gui/PortMenu.cpp | 15 ++++++++++++++- port/hires/HiResHook.cpp | 10 ++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/port/gui/PortMenu.cpp b/port/gui/PortMenu.cpp index 7ba2d7ec..ccdab740 100644 --- a/port/gui/PortMenu.cpp +++ b/port/gui/PortMenu.cpp @@ -1335,7 +1335,15 @@ void PortMenu::AddMenuAssets() { .Options(CheckboxOptions().Tooltip("When on, every cache-miss texture upload hashes the decoded RGBA8 image " "and substitutes a matching PNG from the mods/ index at the pack's higher " "resolution.") - .DefaultValue(true)); + .DefaultValue(ssb64::hires::kHiResEnabledDefault != 0)); +#if defined(__ANDROID__) + AddWidget(path, + "Mobile note: decoded textures are uncompressed in RAM and on the GPU. " + "The pack is capped to a 128 MB cache (gHiResTextures.CacheBudgetMB) and " + "per-texture upscale limit, but large packs can still pressure memory on " + "low-RAM devices. Off by default — enable only if your device has headroom.", + WIDGET_TEXT); +#endif AddWidget(path, "Open Mods Folder", WIDGET_BUTTON) .RaceDisable(false) .Callback([](WidgetInfo&) { @@ -1350,6 +1358,10 @@ void PortMenu::AddMenuAssets() { ssb64::hires::HiResPack::Get().Stats().indexedTextures), WIDGET_TEXT); + // Pack-authoring dump tooling is desktop-only — it writes hundreds of + // files into / and is driven from a separate offline conversion + // pipeline, neither of which makes sense on a touch device. +#if !defined(__ANDROID__) AddWidget(path, "Pack Authoring", WIDGET_SEPARATOR_TEXT); AddWidget(path, "Dump Source Textures writes one .bin per unique texture into " @@ -1398,6 +1410,7 @@ void PortMenu::AddMenuAssets() { SDL_OpenURL(std::string("file:///" + fs::absolute(missPath).string()).c_str()); }) .Options(ButtonOptions().Tooltip("Opens the hires_miss_dump/ folder where native-key dumps land.")); +#endif // !__ANDROID__ (pack-authoring dump tooling) #endif // PORT_HIRES_ENABLED } diff --git a/port/hires/HiResHook.cpp b/port/hires/HiResHook.cpp index afaa6c3f..0aba725c 100644 --- a/port/hires/HiResHook.cpp +++ b/port/hires/HiResHook.cpp @@ -129,10 +129,12 @@ bool HiResHook(uint8_t fmt, uint8_t siz, const uint8_t* rgba8, uint16_t width, uint16_t height, const uint8_t** outBuf, uint16_t* outW, uint16_t* outH) { // Master enable lives in a CVar so the menu toggle takes effect - // immediately (no relaunch). Defaults to on — Init() already logged - // whether the mods/ folder exists, so a flat "no PNGs to substitute" - // setup is silently a no-op. - if (CVarGetInteger("gHiResTextures.Enabled", 1) == 0) { + // immediately (no relaunch). Default is platform-scaled (kHiResEnabledDefault + // in HiResPack.h): on for desktop, OFF/opt-in for Android so a dropped-in + // pack can't silently allocate a large decoded working set on a phone. + // Init() already logged whether the mods/ folder exists, so a flat "no PNGs + // to substitute" setup is silently a no-op. + if (CVarGetInteger("gHiResTextures.Enabled", ssb64::hires::kHiResEnabledDefault) == 0) { return false; } From 8776428b3e9f2871722b0c18b8c5325b2fc058a8 Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 00:52:04 -0400 Subject: [PATCH 3/6] hi-res: build the pack on Android (US) Stop excluding port/hires/ on Android and define PORT_HIRES_ENABLED for Android US too, so the opt-in, mobile-budgeted pack compiles into libmain.so. JP still drops the pack entirely (US-only hash inputs). Co-Authored-By: Claude Opus 4.8 (1M context) --- CMakeLists.txt | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d676414f..270633d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -472,13 +472,13 @@ if (CMAKE_SYSTEM_NAME STREQUAL "Android") # deps don't bloat the SDL-bound game library. list(FILTER SSB64_SRC_PORT EXCLUDE REGEX "port/android_torch_bridge\\.cpp$") # Drop features we explicitly disable on Android (no curl-driven - # self-updater, no discord-rpc link, no libretro shader downloader, - # no hires-pack runtime). The corresponding menu entries are gated - # with #if !defined(__ANDROID__) in port/gui/PortMenu.cpp so nothing - # references the removed symbols. + # self-updater, no discord-rpc link, no libretro shader downloader). + # The corresponding menu entries are gated with #if !defined(__ANDROID__) + # in port/gui/PortMenu.cpp so nothing references the removed symbols. + # (port/hires/ is NOT dropped here — it compiles on Android but runs + # opt-in with a reduced LRU budget; see the PORT_HIRES_ENABLED block.) list(FILTER SSB64_SRC_PORT EXCLUDE REGEX "port/enhancements/(Updater|DiscordRichPresence|ShaderDownloader)\\.cpp$") - list(FILTER SSB64_SRC_PORT EXCLUDE REGEX "port/hires/") list(FILTER SSB64_SRC_PORT EXCLUDE REGEX "port/port_window_icon\\.cpp$") endif() # Treat the generated icon header as a source so CMake schedules it ahead @@ -621,17 +621,19 @@ if(SSB64_STAGE_CYCLE_DEMO) list(APPEND SSB64_COMPILE_DEFS PORT_STAGE_CYCLE_DEMO=1) endif() -# Hi-res texture pack support is US-only on desktop and dropped entirely -# on Android (mobile memory budget can't carry the 512 MB LRU cache, -# and the dump-tooling menu items add no value on touch UI). +# Hi-res texture pack support is US-only (desktop AND Android). On Android +# it compiles but defaults to OFF and runs with a smaller decoded-RGBA8 LRU +# budget (see port/hires/HiResPack.h: kHiResEnabledDefault / kDefaultLruBudgetMB) +# plus a per-texture upscale cap, so a pack can't blow the mobile memory +# budget and trip the low-memory killer; the dev dump-tooling menu items are +# still gated off touch UI in PortMenu.cpp. # Pack PNGs are addressed by CRC32-IEEE of the decoded RGBA8 buffer that # came out of the US ROM fast-path; JP rendering hits different hash inputs # (different glyph art for VS-records, JP-only menus, plus palette/sprite # nuances on shared assets), so a US-built pack would silently miss on JP -# and no JP-only pack has been authored. Drop port/hires/ from the JP and -# Android builds entirely and gate the port.cpp / PortMenu.cpp integration -# on PORT_HIRES_ENABLED. -if(SSB64_VERSION STREQUAL "us" AND NOT CMAKE_SYSTEM_NAME STREQUAL "Android") +# and no JP-only pack has been authored. Drop port/hires/ from the JP build +# entirely and gate the port.cpp / PortMenu.cpp integration on PORT_HIRES_ENABLED. +if(SSB64_VERSION STREQUAL "us") list(APPEND SSB64_COMPILE_DEFS PORT_HIRES_ENABLED=1) else() list(FILTER SSB64_SRC_PORT EXCLUDE REGEX "port/hires/") From 597553b4b6e04bdf9d034774a5fe39e44c348b46 Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 01:14:56 -0400 Subject: [PATCH 4/6] android: import hi-res texture pack (.zip) via SAF into app-owned mods/ Under scoped storage the native HiResPack scanner can only read files the app itself owns; a pack adb-pushed or dropped by a file manager into Android/data// is owned by another uid and the recursive scan fails with "Permission denied". So the app copies the user-picked zip into its own mods/ (app-owned -> readable), reusing BootActivity's SAF flow. - PackImporter: streams the picked content:// URI straight to mods/.zip (no cacheDir double-copy -- packs are ~200 MB) via a .part temp renamed only after it validates as a real ZIP. Every failure path deletes the partial; a full disk fails fast; the SAF display name is sanitized to a safe basename (no path traversal); a 1 GB cap rejects absurd input. Carries the v1.3 Android-pass discipline: clean up large staged files, validate untrusted input, fail fast on write error. - BootActivity: an "Import texture pack" launcher shortcut routes here with ssb64.import_pack=true -> SAF picker -> background import -> Toast -> normal route. Import runs BEFORE the SDL game starts, so HiResPack's scan sees the new pack single-threaded at boot -- no rescan race against the renderer. - Dev shortcut ssb64.dev_pack= mirrors ssb64.dev_rom for scripted install without the picker UI. Verified on the API 34 arm64 emulator: import writes an app-owned mods/devpack.zip, the native scan indexes 4512 textures from it (no Permission denied), lookups hit ~87%, and the decoded-RGBA8 LRU stays pinned at the 128 MB Android budget under the full 219 MB pack with no low-memory kill. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../com/jrickey/battleship/BootActivity.java | 107 ++++++++++ .../com/jrickey/battleship/PackImporter.java | 200 ++++++++++++++++++ android/app/src/main/res/values/strings.xml | 2 + android/app/src/main/res/xml/shortcuts.xml | 18 ++ 4 files changed, 327 insertions(+) create mode 100644 android/app/src/main/java/com/jrickey/battleship/PackImporter.java diff --git a/android/app/src/main/java/com/jrickey/battleship/BootActivity.java b/android/app/src/main/java/com/jrickey/battleship/BootActivity.java index f4996241..56f7e5df 100644 --- a/android/app/src/main/java/com/jrickey/battleship/BootActivity.java +++ b/android/app/src/main/java/com/jrickey/battleship/BootActivity.java @@ -11,6 +11,7 @@ import android.widget.Button; import android.widget.LinearLayout; import android.widget.TextView; +import android.widget.Toast; import androidx.activity.ComponentActivity; import androidx.activity.result.ActivityResultLauncher; @@ -48,6 +49,13 @@ public class BootActivity extends ComponentActivity { new ActivityResultContracts.OpenDocument(), this::onRomPicked); + /** Result handler for the hi-res-pack SAF picker (import-pack flow). + * Registered eagerly so it survives Activity recreation mid-pick. */ + private final ActivityResultLauncher mPickPack + = registerForActivityResult( + new ActivityResultContracts.OpenDocument(), + this::onPackPicked); + @Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); @@ -80,6 +88,34 @@ protected void onCreate(Bundle savedInstanceState) { */ private static final String EXTRA_REPICK = "ssb64.repick"; + /** + * If true on the launching Intent, BootActivity shows a SAF picker for a + * hi-res texture pack (.zip) after assets are ready, copies it into the + * app-owned mods/ folder ({@link PackImporter}), then continues to the + * normal route. Surfaced via the "Import texture pack" launcher shortcut + * (res/xml/shortcuts.xml); also drivable for dev/testing: + * + * adb shell am start -n com.jrickey.battleship.debug/.BootActivity \ + * --ez ssb64.import_pack true + * + * Import runs BEFORE the SDL game starts, so HiResPack's scan sees the new + * pack on a single thread at boot — no rescan race against the renderer. + */ + private static final String EXTRA_IMPORT_PACK = "ssb64.import_pack"; + + private boolean isImportPackRequest() { + return getIntent() != null + && getIntent().getBooleanExtra(EXTRA_IMPORT_PACK, false); + } + + /** + * Dev/test: import a pack from an absolute path the app can read, skipping + * the SAF picker. Mirrors {@link #EXTRA_DEV_ROM}. + * adb shell am start -n com.jrickey.battleship.debug/.BootActivity \ + * --es ssb64.dev_pack /sdcard/Download/pack.zip + */ + private static final String EXTRA_DEV_PACK = "ssb64.dev_pack"; + private void buildUi() { mStatus = new TextView(this); mStatus.setGravity(Gravity.CENTER); @@ -129,6 +165,16 @@ private void startAssetExtraction() { showError("Asset extraction failed:\n\n" + err); return; } + String devPack = getIntent() != null + ? getIntent().getStringExtra(EXTRA_DEV_PACK) : null; + if (devPack != null && !devPack.isEmpty()) { + importDevPack(new File(devPack)); + return; + } + if (isImportPackRequest()) { + showPackPicker(); + return; + } routeAfterAssets(); }); }, "ssb64-boot-extract").start(); @@ -248,6 +294,67 @@ private void onRomPicked(Uri romUri) { }, "ssb64-rom-stage").start(); } + /* ===================================================================== */ + /* Optional: hi-res texture pack import (ssb64.import_pack) */ + /* ===================================================================== */ + + private void showPackPicker() { + mStatus.setText( + "Import a hi-res texture pack.\n\n" + + "Pick the pack's .zip file. It's copied into the app's mods folder " + + "and applied the next time the game loads textures." + ); + mPickButton.setText("Choose texture pack (.zip)"); + mPickButton.setOnClickListener(v -> launchPackPicker()); + mPickButton.setVisibility(View.VISIBLE); + } + + private void launchPackPicker() { + // Bias toward zips but fall back to "*/*" (some providers report a + // pack as octet-stream); PackImporter validates by actually opening it. + mPickPack.launch(new String[] { "application/zip", "*/*" }); + } + + private void importDevPack(File packFile) { + mPickButton.setVisibility(View.GONE); + mStatus.setText("Importing texture pack (dev)…"); + new Thread(() -> { + File pack = PackImporter.importPackFromFile(getApplicationContext(), packFile); + runOnUi(() -> { + Toast.makeText(BootActivity.this, + pack != null + ? "Texture pack imported (dev)." + : "Dev pack import failed — see logcat tag ssb64.pack.", + Toast.LENGTH_LONG).show(); + routeAfterAssets(); + }); + }, "ssb64-pack-import-dev").start(); + } + + private void onPackPicked(Uri packUri) { + if (packUri == null) { + // Cancelled — go on to the game/ROM flow without a new pack. + routeAfterAssets(); + return; + } + mPickButton.setVisibility(View.GONE); + mStatus.setText("Importing texture pack…"); + + new Thread(() -> { + File pack = PackImporter.importPack(getApplicationContext(), packUri); + runOnUi(() -> { + // Toast survives the hand-off to the game Activity, so the + // result is visible even though we route on immediately. + Toast.makeText(BootActivity.this, + pack != null + ? "Texture pack imported." + : "Couldn't import that file — is it a valid .zip pack?", + Toast.LENGTH_LONG).show(); + routeAfterAssets(); + }); + }, "ssb64-pack-import").start(); + } + /* ===================================================================== */ /* Stage 3: hand off to the SDL game Activity */ /* ===================================================================== */ diff --git a/android/app/src/main/java/com/jrickey/battleship/PackImporter.java b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java new file mode 100644 index 00000000..568ad8f5 --- /dev/null +++ b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java @@ -0,0 +1,200 @@ +package com.jrickey.battleship; + +import android.content.Context; +import android.database.Cursor; +import android.net.Uri; +import android.provider.OpenableColumns; +import android.util.Log; + +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.Locale; +import java.util.zip.ZipFile; + +/** + * Imports a hi-res texture pack (.zip) from a SAF content URI into the app's + * own mods/ directory under externalFilesDir. + * + * Why copy at all: under scoped storage the native HiResPack scanner can only + * read files the app itself owns. A pack adb-pushed or dropped by a file + * manager into Android/data/<pkg>/ is owned by another uid and the scan + * fails with "Permission denied". Copying the picked stream here makes the + * file app-owned, so {@code HiResPack::Init} reads it (in place, no extraction) + * on the next game launch. + * + * Security/robustness (mirrors the v1.3 Android pass on RomImporter/BootActivity): + * - The pack is hundreds of MB, so the stream goes STRAIGHT to its final + * mods/ path — never staged into cacheDir and copied again. Bytes flow in + * 256 KB chunks, so peak heap is bounded regardless of pack size. + * - Written to a "<name>.zip.part" temp and renamed only after it + * validates, so an interrupted/torn copy never leaves a half-written .zip + * the scanner would trip on. Every failure path deletes the partial — no + * orphaned hundred-MB files. + * - A full disk (write IOException) fails fast instead of publishing a + * truncated archive. + * - The SAF display name is sanitized to a bare basename (no path + * separators / traversal) and forced to end in .zip. + * - The copied file is untrusted: it must open as a real ZIP with at least + * one entry before it's published. Extension/MIME are not trusted. + * - A size cap rejects an absurd download (DoS / accidental wrong file). + */ +public final class PackImporter { + private static final String TAG = "ssb64.pack"; + + /** Hard ceiling on an imported pack. Real packs are ~200 MB; this only + * guards against an accidental wrong file or a malicious huge archive. */ + private static final long MAX_PACK_BYTES = 1024L * 1024L * 1024L; // 1 GB + + /** + * Copy the picked zip into <externalFilesDir>/mods/. Heavy I/O — call + * from a background thread. + * + * @return the published File on success, or null on any failure (already + * logged under tag "ssb64.pack"; the partial is cleaned up). + */ + public static File importPack(Context ctx, Uri uri) { + if (uri == null) { + return null; + } + try (InputStream in = ctx.getContentResolver().openInputStream(uri)) { + if (in == null) { + Log.e(TAG, "openInputStream returned null for " + uri); + return null; + } + return copyToMods(ctx, in, queryDisplayName(ctx, uri)); + } catch (IOException e) { + Log.e(TAG, "open content uri failed", e); + return null; + } + } + + /** + * Dev/test path: import a pack from an absolute file the app can already + * read (e.g. an adb-pushed /sdcard/Download/*.zip). Mirrors RomImporter's + * dev_rom shortcut; not part of the user-facing SAF flow. + */ + public static File importPackFromFile(Context ctx, File src) { + if (src == null || !src.canRead()) { + Log.e(TAG, "dev pack not readable: " + src); + return null; + } + try (InputStream in = new FileInputStream(src)) { + return copyToMods(ctx, in, src.getName()); + } catch (IOException e) { + Log.e(TAG, "open dev pack failed", e); + return null; + } + } + + /** Stream {@code in} into mods/<safe>.zip via a validated temp+rename. */ + private static File copyToMods(Context ctx, InputStream in, String displayName) { + File extDir = ctx.getExternalFilesDir(null); + if (extDir == null) { + Log.e(TAG, "externalFilesDir is null — external storage not mounted?"); + return null; + } + File modsDir = new File(extDir, "mods"); + if (!modsDir.isDirectory() && !modsDir.mkdirs()) { + Log.e(TAG, "could not create mods dir " + modsDir); + return null; + } + + File dst = new File(modsDir, safeZipName(displayName)); + File tmp = new File(modsDir, dst.getName() + ".part"); + + long total = 0; + try (OutputStream out = new FileOutputStream(tmp)) { + byte[] buf = new byte[256 * 1024]; + int n; + while ((n = in.read(buf)) > 0) { + total += n; + if (total > MAX_PACK_BYTES) { + // Treated as a copy failure so the single catch cleans up. + throw new IOException("pack exceeds " + MAX_PACK_BYTES + "-byte cap"); + } + out.write(buf, 0, n); + } + } catch (IOException e) { + Log.e(TAG, "copy failed (full disk / read error / too large?)", e); + tmp.delete(); + return null; + } + + // Untrusted input: confirm it's a parseable ZIP with at least one entry + // before publishing it where the native scanner will open it. + if (!isValidZip(tmp)) { + Log.e(TAG, "rejected " + total + " bytes: not a valid .zip archive"); + tmp.delete(); + return null; + } + + // Publish: replace any existing same-named pack, then atomically rename + // the validated temp into place. + if (dst.exists() && !dst.delete()) { + Log.w(TAG, "could not replace existing " + dst); + } + if (!tmp.renameTo(dst)) { + Log.e(TAG, "rename " + tmp.getName() + " -> " + dst.getName() + " failed"); + tmp.delete(); + return null; + } + Log.i(TAG, "imported " + total + " bytes -> " + dst); + return dst; + } + + /** Best-effort SAF display name (OpenableColumns.DISPLAY_NAME); null on miss. */ + private static String queryDisplayName(Context ctx, Uri uri) { + try (Cursor c = ctx.getContentResolver().query( + uri, new String[] { OpenableColumns.DISPLAY_NAME }, + null, null, null)) { + if (c != null && c.moveToFirst()) { + int idx = c.getColumnIndex(OpenableColumns.DISPLAY_NAME); + if (idx >= 0) { + return c.getString(idx); + } + } + } catch (Exception e) { + Log.w(TAG, "display-name query failed; using fallback name", e); + } + return null; + } + + /** + * Turn a SAF display name into a safe basename ending in .zip. Strips any + * path components (a content provider could hand back "../x" or "a/b") and + * restricts to a conservative charset so nothing escapes mods/. + */ + static String safeZipName(String displayName) { + String base = (displayName == null) ? "" : displayName; + int cut = Math.max(base.lastIndexOf('/'), base.lastIndexOf('\\')); + if (cut >= 0) { + base = base.substring(cut + 1); + } + base = base.replaceAll("[^A-Za-z0-9 ._-]", "_").trim(); + if (base.isEmpty() || base.equals(".") || base.equals("..")) { + base = "pack"; + } + if (!base.toLowerCase(Locale.ROOT).endsWith(".zip")) { + base = base + ".zip"; + } + if (base.length() > 100) { + base = base.substring(0, 96) + ".zip"; + } + return base; + } + + /** True if {@code f} opens as a ZIP with at least one entry. */ + private static boolean isValidZip(File f) { + try (ZipFile zf = new ZipFile(f)) { + return zf.size() > 0; + } catch (IOException e) { + return false; + } + } + + private PackImporter() { /* static */ } +} diff --git a/android/app/src/main/res/values/strings.xml b/android/app/src/main/res/values/strings.xml index fe0e93e6..32c65feb 100644 --- a/android/app/src/main/res/values/strings.xml +++ b/android/app/src/main/res/values/strings.xml @@ -5,4 +5,6 @@ Re-extract ROM Provide a different SSB64 ROM + Import texture pack + Import a hi-res texture pack (.zip) diff --git a/android/app/src/main/res/xml/shortcuts.xml b/android/app/src/main/res/xml/shortcuts.xml index 6bf4e392..b4f2cba6 100644 --- a/android/app/src/main/res/xml/shortcuts.xml +++ b/android/app/src/main/res/xml/shortcuts.xml @@ -28,4 +28,22 @@ + + + + + + + + + From 81c92ed1d466daf9bb1757ca1e87d76d7b565529 Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 01:47:19 -0400 Subject: [PATCH 5/6] hi-res: address Codex adversarial review (zip-read OOM guard + cleanups) Findings from an adversarial pass over the branch: - [high] ReadZipMember trusted zip_stat's declared member size and did out.resize(st.size) with no bound or bad_alloc handling -- a hostile or corrupt downloaded pack with a huge declared entry crashed the game via uncaught std::bad_alloc. Now reject size 0 / > 256 MB (real PNGs are ~25 MB; stays under INT_MAX so the later (int) cast for stbi can't overflow) and catch bad_alloc. - [low] Init() cleared gIndex/gOpenZips but not gLru -- a future re-scan with a changed pack could serve a stale decoded texture on an LRU hit. Add LruCache::Clear() and call it in Init(). - [low] PackImporter published with delete-then-rename (a window leaving mods/ with neither old nor new pack on rename failure) and could orphan the ~200 MB .part if ZipFile validation threw an Error. Now publish via atomic Files.move(REPLACE_EXISTING) inside a published-flag try/finally that deletes the temp on any non-success exit. - [low] The ssb64.dev_pack BootActivity extra is now gated behind BuildConfig.DEBUG so a release build's exported launcher activity can't be driven by another app into an arbitrary-path import (confused deputy). Re-verified on the emulator: import still writes an app-owned mods/devpack.zip, the scan indexes 4512 textures with zero spurious size rejections, and lookups hit ~86%. The review also confirmed the init-race is absent and the LRU tail-eviction invariant, budget floor, and entry-lifetime are sound. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../com/jrickey/battleship/BootActivity.java | 5 ++- .../com/jrickey/battleship/PackImporter.java | 44 +++++++++++-------- port/hires/HiResPack.cpp | 37 +++++++++++++++- 3 files changed, 65 insertions(+), 21 deletions(-) diff --git a/android/app/src/main/java/com/jrickey/battleship/BootActivity.java b/android/app/src/main/java/com/jrickey/battleship/BootActivity.java index 56f7e5df..3ffe95a7 100644 --- a/android/app/src/main/java/com/jrickey/battleship/BootActivity.java +++ b/android/app/src/main/java/com/jrickey/battleship/BootActivity.java @@ -165,7 +165,10 @@ private void startAssetExtraction() { showError("Asset extraction failed:\n\n" + err); return; } - String devPack = getIntent() != null + // Debug-only: BootActivity is the exported launcher entry, so a + // release build must not let another app drive an arbitrary + // absolute-path import through this extra (confused deputy). + String devPack = (BuildConfig.DEBUG && getIntent() != null) ? getIntent().getStringExtra(EXTRA_DEV_PACK) : null; if (devPack != null && !devPack.isEmpty()) { importDevPack(new File(devPack)); diff --git a/android/app/src/main/java/com/jrickey/battleship/PackImporter.java b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java index 568ad8f5..7bf427a0 100644 --- a/android/app/src/main/java/com/jrickey/battleship/PackImporter.java +++ b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java @@ -12,6 +12,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; import java.util.Locale; import java.util.zip.ZipFile; @@ -124,26 +126,32 @@ private static File copyToMods(Context ctx, InputStream in, String displayName) return null; } - // Untrusted input: confirm it's a parseable ZIP with at least one entry - // before publishing it where the native scanner will open it. - if (!isValidZip(tmp)) { - Log.e(TAG, "rejected " + total + " bytes: not a valid .zip archive"); - tmp.delete(); - return null; - } - - // Publish: replace any existing same-named pack, then atomically rename - // the validated temp into place. - if (dst.exists() && !dst.delete()) { - Log.w(TAG, "could not replace existing " + dst); - } - if (!tmp.renameTo(dst)) { - Log.e(TAG, "rename " + tmp.getName() + " -> " + dst.getName() + " failed"); - tmp.delete(); + // Validate (untrusted input — confirm it parses as a ZIP with >=1 entry) + // and publish. The `published` flag + finally guarantees the .part temp + // is removed on every exit short of a successful publish — including a + // Throwable (e.g. OutOfMemoryError raised inside ZipFile on a hostile + // archive), which would otherwise leave a ~200 MB orphan. + boolean published = false; + try { + if (!isValidZip(tmp)) { + Log.e(TAG, "rejected " + total + " bytes: not a valid .zip archive"); + return null; + } + // Files.move replaces any existing same-named pack in one step — no + // delete-then-rename window that could leave mods/ with neither the + // old nor the new pack. + Files.move(tmp.toPath(), dst.toPath(), StandardCopyOption.REPLACE_EXISTING); + published = true; + Log.i(TAG, "imported " + total + " bytes -> " + dst); + return dst; + } catch (IOException e) { + Log.e(TAG, "publish failed", e); return null; + } finally { + if (!published) { + tmp.delete(); + } } - Log.i(TAG, "imported " + total + " bytes -> " + dst); - return dst; } /** Best-effort SAF display name (OpenableColumns.DISPLAY_NAME); null on miss. */ diff --git a/port/hires/HiResPack.cpp b/port/hires/HiResPack.cpp index cc2aecd5..9ae9a6b3 100644 --- a/port/hires/HiResPack.cpp +++ b/port/hires/HiResPack.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -190,6 +191,15 @@ class LruCache { EvictToBudget(); } + // Drop all decoded entries (called from Init so a re-scan with a changed + // pack can't return a stale decoded texture on an LRU hit that predates + // the new index). + void Clear() { + mList.clear(); + mIndex.clear(); + mBytes = 0; + } + size_t Bytes() const noexcept { return mBytes; } size_t Budget() const noexcept { return mBudget; } @@ -355,7 +365,16 @@ void ScanZip(const std::string& zipPath, PackStats& stats) { } } -// Read a zip member fully into `out`. Returns false on any libzip error. +// Upper bound on a single decoded zip member (the compressed PNG bytes we +// read into memory). A pack is untrusted input — without a cap a crafted or +// corrupt entry whose stat declares a huge uncompressed size would drive +// out.resize() into an uncaught std::bad_alloc (crash). Real pack PNGs top out +// around ~25 MB; 256 MB is generous headroom and stays well under INT_MAX so +// the later (int)bytes.size() cast for stbi_load_from_memory can't overflow. +constexpr uint64_t kMaxPackMemberBytes = 256ull * 1024ull * 1024ull; + +// Read a zip member fully into `out`. Returns false on any libzip error, an +// out-of-range member size, or an allocation failure. bool ReadZipMember(const std::string& zipPath, const std::string& member, std::vector& out) { zip_t* z = OpenZipCached(zipPath); @@ -363,9 +382,22 @@ bool ReadZipMember(const std::string& zipPath, const std::string& member, zip_stat_t st; zip_stat_init(&st); if (zip_stat(z, member.c_str(), 0, &st) != 0 || !(st.valid & ZIP_STAT_SIZE)) return false; + // Reject empty / absurd members before allocating from an untrusted stat. + if (st.size == 0 || st.size > kMaxPackMemberBytes) { + port_log("HiResPack: zip member %s size %llu out of range — skipping\n", + member.c_str(), (unsigned long long)st.size); + return false; + } zip_file_t* zf = zip_fopen(z, member.c_str(), 0); if (zf == nullptr) return false; - out.resize((size_t)st.size); + try { + out.resize((size_t)st.size); + } catch (const std::bad_alloc&) { + port_log("HiResPack: out of memory reading zip member %s (%llu bytes)\n", + member.c_str(), (unsigned long long)st.size); + zip_fclose(zf); + return false; + } zip_int64_t rd = zip_fread(zf, out.data(), st.size); zip_fclose(zf); return rd >= 0 && (zip_uint64_t)rd == st.size; @@ -385,6 +417,7 @@ const char* HiResPack::ModsRoot() const noexcept { bool HiResPack::Init() { mStats = {}; gIndex.clear(); + gLru.Clear(); // drop decoded textures so a re-scan can't serve stale hits // Close any zip handles from a prior Init before re-scanning. for (auto& [path, z] : gOpenZips) { if (z != nullptr) zip_close(z); From 20c6e6aac0b06d0d36779051338de5a7cdda85a7 Mon Sep 17 00:00:00 2001 From: JRickey Date: Wed, 17 Jun 2026 02:13:56 -0400 Subject: [PATCH 6/6] hi-res: second adversarial-review pass (Activity-recreation + edge guards) A deeper review of the branch surfaced three items beyond the first pass: - [medium] BootActivity runs the multi-second pack import on a background thread but had no configChanges, so a config change in that window (multi-window resize / dark mode / font scale; rotation is already locked landscape) recreates the Activity and the worker posts its result to the dead instance, leaving the app stuck on the picker. Give BootActivity a configChanges list so it absorbs those without recreation. Also covers the identical pre-existing ROM-picker case. - [low] safeZipName: a SAF display name of " .zip" trims to ".zip" and would land as a hidden file; reset that to "pack.zip". - [low] ScanZip: log when zip_get_num_entries returns -1 (corrupt central directory) instead of silently treating the pack as empty. The same review confirmed the earlier fixes hold and that the remaining flagged items are non-bugs (log-before-erase, tail-eviction invariant, zero-budget floor, the redundant-but-safe post-Insert Get, boot-only init with no concurrent Lookup). Co-Authored-By: Claude Opus 4.8 (1M context) --- android/app/src/main/AndroidManifest.xml | 9 ++++++++- .../main/java/com/jrickey/battleship/PackImporter.java | 5 ++++- port/hires/HiResPack.cpp | 5 +++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml index bf919a4b..c7d0e748 100644 --- a/android/app/src/main/AndroidManifest.xml +++ b/android/app/src/main/AndroidManifest.xml @@ -52,7 +52,14 @@ android:exported="true" android:resizeableActivity="true" android:maxAspectRatio="2.4" - android:screenOrientation="landscape"> + android:screenOrientation="landscape" + android:configChanges="layoutDirection|locale|orientation|density|fontScale|uiMode|screenLayout|screenSize|smallestScreenSize|keyboard|keyboardHidden|navigation"> + diff --git a/android/app/src/main/java/com/jrickey/battleship/PackImporter.java b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java index 7bf427a0..2a18eb4a 100644 --- a/android/app/src/main/java/com/jrickey/battleship/PackImporter.java +++ b/android/app/src/main/java/com/jrickey/battleship/PackImporter.java @@ -183,7 +183,10 @@ static String safeZipName(String displayName) { base = base.substring(cut + 1); } base = base.replaceAll("[^A-Za-z0-9 ._-]", "_").trim(); - if (base.isEmpty() || base.equals(".") || base.equals("..")) { + // Also reject names that are nothing but the extension (e.g. a SAF name + // of " .zip" trims to ".zip", which would land as a hidden file). + if (base.isEmpty() || base.equals(".") || base.equals("..") + || base.equalsIgnoreCase(".zip")) { base = "pack"; } if (!base.toLowerCase(Locale.ROOT).endsWith(".zip")) { diff --git a/port/hires/HiResPack.cpp b/port/hires/HiResPack.cpp index 9ae9a6b3..fc88b9c5 100644 --- a/port/hires/HiResPack.cpp +++ b/port/hires/HiResPack.cpp @@ -356,6 +356,11 @@ void ScanZip(const std::string& zipPath, PackStats& stats) { zip_t* z = OpenZipCached(zipPath); if (z == nullptr) return; zip_int64_t n = zip_get_num_entries(z, 0); + if (n < 0) { // corrupt central directory; signed n makes the loop skip anyway + port_log("HiResPack: zip_get_num_entries failed for %s — treating as empty\n", + zipPath.c_str()); + return; + } for (zip_int64_t i = 0; i < n; i++) { const char* name = zip_get_name(z, i, 0); if (name == nullptr) continue;