PointerWrap and the Do() overloads around it are how every savestate is
written and read, and had no direct coverage. Everything read back came off
disk, so the corrupt-input paths matter as much as the round trips.
Three bugs, all in the bounds checking added in 58d4759ceb:
1. sizeof(T) is not a lower bound on how many bytes an element serializes to.
It only holds for the types DoHelper_ writes out raw. A std::string is 32-40
bytes in memory and serializes to as few as five; a T* serializes to whatever
T::DoState() writes. So DoVector/DoList/DoSet/DoMap could reject a perfectly
valid savestate whenever count * sizeof(element) exceeded the bytes left in
the buffer. That is not hypothetical: pspFileSystem is serialized dead last
in SaveStart::DoState, and MetaFileSystem::DoState does Do(p, currentDir) on
a std::map<int, std::string>, so the check runs with only a few hundred bytes
remaining and claims 44 bytes per entry against roughly 22 actual. Added
SerializeMinElemSize<T>(), mirroring DoHelper_'s own condition, and used it
in all five containers. The bound is only loosened, so nothing that loaded
before can stop loading.
2. Do(p, std::map<K, T *> &) deletes every value before reading the new ones,
and DoMap then returned on a bad count without clearing - leaving the map
full of freed pointers to be used or deleted again. Six live maps go through
this (sceMpeg, sceMp3, sceAac, sceFont, sceHeap, sceKernelThread's pending
calls), so a corrupt savestate meant a use-after-free. Clear before the guard
can bail out, in DoMap, DoMultimap and DoSet.
3. The wstring and u16string overloads validated stringLen < 0 but not 0, and
didn't require a whole number of characters. read() computes
stringLen / sizeof(char) - 1, so a length of 0 resized to SIZE_MAX and
memcpy'd with a wrapped-around size. PSPOskDialog::DoState serializes both
(inputChars at v2, a legacy wstring below that), so this was reachable: the
test aborts the process without the fix.
The test covers round trips of PODs, strings (empty, embedded NUL), vector,
map, set, list and map-of-pointers, section titles and version gating in both
directions, marker mismatches, measure-vs-write checkpoint disagreement, the
error latch dropping to MODE_NOOP, every truncation of a valid buffer, and
hand-corrupted counts and lengths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
DenseHashMap and PrehashMap are the open-addressed, linear-probing maps behind
the texture cache, the shader managers and the software renderer's
sampler/drawpixel caches, and had no coverage.
Writing the tests turned up a latent hang. Removal leaves tombstones, which
occupy probe slots exactly like live entries, but the load factor check only
looked at count_. So a workload that inserts and removes distinct keys keeps
count_ low forever while REMOVED fills the table, and no Grow is ever triggered.
Once there is no FREE bucket left, a lookup for a missing key has nothing to
terminate on - and the probe loops don't break out after their "Hit full"
assert, which is compiled out in release builds. The test reproduced it as a
hard hang in about a second.
Two fixes: count tombstones towards the load factor (rebuilding in place when
the load is mostly tombstones, growing otherwise), and make the probe loops
return instead of spinning if they ever do wrap all the way around.
Not reachable today - nothing in GPU/ calls Remove() on these maps, and
Maintain(), which exists to rebuild when tombstones pile up, is never called
anywhere. But Remove() is public API and the first caller to use it in a loop
would have hit an unexplained freeze.
Tests cover insert/get/miss/remove/size, tombstones not cutting a probe chain,
Iterate visiting exactly the live entries, Clear, growth past the initial
capacity, Rebuild compacting, a 20000-operation differential test against
std::unordered_map, and the tombstone churn above. PrehashMap gets the same
treatment.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
SymbolMap had no coverage. It stores symbols relative to a module so they
survive that module being unloaded and reloaded elsewhere, and only symbols
belonging to a loaded module count as active - that indirection is where the
surprises are, so the tests concentrate on module lifetime and the shared label
table.
Writing them turned up a real bug. UpdateActiveSymbols() bailed out early when
activeModuleEnds was empty, as a "tiny optimization" for startup and shutdown,
having already cleared the active maps. But symbols with module index 0 are
absolute by design - they belong to no module, which is how you label a heap or
stack address - and the loops it skipped are exactly what keeps those alive.
So an absolute symbol disappeared as soon as the last module was unloaded, and
didn't exist at all before the first one was loaded. Dropping activeModuleEnds
from the early-out condition fixes it; the symbol-count half still gives the
intended fast path when there's nothing to do.
Tests cover function and data lookup by containing address, SetFunctionSize,
RemoveFunction/RemoveData, symbols surviving an unload/reload at a different
address, absolute (module 0) symbols, GetSymbolInfo/GetDescription, and Clear.
Two of them pin down behaviour that catches people out rather than asserting
it's right: AddLabel deliberately won't overwrite an existing label, and because
functions and data share one label table, renaming or removing via one affects
the other.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
BlockAllocator backs sceKernelAllocPartitionMemory and the various partition
allocators, and had no coverage at all. It's pure address bookkeeping - no real
memory - so it can be checked hard: after any sequence of operations the blocks
must still tile the range exactly, with no gaps, overlaps or strays, and
GetTotalFreeBytes/GetLargestFreeBlockSize must agree with what's really in the
list. ValidateAllocator() rebuilds the list through the public accessors and
asserts all of that, and it runs after every step of the randomised sections.
Covers bottom-up and top-down allocation, grain rounding, rejection of zero and
oversized requests, splitting and re-merging on free, double free and free of an
address that was never allocated, AllocAt including unaligned positions and
collisions, AllocAligned with alignments coarser than the allocator's grain,
filling the range completely and draining it, a range whose size isn't a
multiple of the grain, and two randomised churn loops - one plain alloc/free,
one mixing in aligned allocations and AllocAt to reach block layouts the simple
loop never produces. Fixed seeds, so a failure reproduces.
No bugs found - the allocator holds up. Verified the tests aren't vacuous by
injecting two plausible bugs: dropping the forward merge in MergeFreeBlocks, and
an off-by-one in the bottom-up fit check. Both are caught, the second pinpointed
to a specific churn iteration.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
UnitTest.exe runs on CI and from tooling, where an assert or an abort() puts
up a message box that nothing will ever click, and the run just hangs until it
is killed. Headless already solved this; move its SetupCRT() into Common
(ExceptionHandlerSetup, which is where the rest of the process-level fault
setup lives) and call it from the unit tests too.
No behaviour change for headless. The OS-level SetErrorMode() call is now
guarded for UWP, which doesn't have it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
step-over, step-out and run-until plant a one-shot breakpoint at the address
they want execution to return to. Keeping it in breakPoints_ alongside the
user's own meant the two kept colliding:
- Adding a log-only user breakpoint at the same address hijacked the temporary
one. AddBreakPoint() didn't match across temp-ness so both existed, and then
ChangeBreakPoint() looked up "the first enabled breakpoint at this address" -
a log-only breakpoint isn't enabled, so the temporary one won and had its
action overwritten to log-only. It lost PAUSE and the step never came back.
- RemoveBreakPoint() erased up to two entries per address to catch an
overlapping temporary one, so deleting either deleted both - including the
interpreter's cleanup path in CheckExecBreakpoints() taking the user's
breakpoint with it.
- ExecBreakPoint() handled one breakpoint per address, so with both at the same
address only one of them did anything: the step completed but the user's log
line never printed.
- Nothing dropped it when something *else* stopped us first, so an interrupted
step left a breakpoint armed at an address nobody was waiting for anymore,
which later fired as a phantom stop.
It's a single TempBreakPoint member now, invisible to the breakpoint lists and
untouched by user edits. One is enough: step over/out and cross-thread step into
all require the CPU to already be stepping and resume it immediately, so only
one can be in flight, and run-until now replaces rather than stacking (two
pending run-untils had no coherent meaning, and the loser stayed armed).
Behavior follows what other debuggers do. Both breakpoints at an address are
evaluated independently and their actions combine, so a log-only breakpoint
logs without stopping and still lets the step finish. Core_Break() drops the
temporary breakpoint on any stop, whatever the reason - the same way gdb deletes
its step-resume breakpoint and lldb discards the thread plan.
Two things to be careful of, both covered by the new TempBreakpoints test:
HasBreakPoints() has to account for it, or the interpreter's checked run loop
and the JIT skip breakpoint checking entirely and a step with no user
breakpoints set never returns; and IsAddressBreakPoint() (user-facing, for the
lists and disassembly markers) is now separate from NeedsBreakCheckAt() (what
the JIT frontends and interpreter ask), since only the latter should see it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
ChangeBreakPointAddress() moves the breakpoint keeping its action, condition and
log format, invalidates both ends, refuses to land on an existing breakpoint,
and resets the hit count since it belonged to the old address. The edit form now
works on a copy of the address and commits on deactivation rather than per
keystroke, so typing one address doesn't churn through every prefix of it.
The breakpoint edit form assigned straight to bp.addr and then invalidated the
icache at "bp.addr - 4, 8" - which by then is the *new* address - need both.
Also clear the selection after Delete in both edit forms - the reference into
the vector is dangling from that point on. Harmless today, but only because
nothing happens to touch it below.
Covered by a new Breakpoints unit test (verified to fail without the duplicate
check and the hit reset).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
memory.readString could kill the connection: it copied raw emulated memory
straight into a JSON string, so any address not holding valid UTF-8 produced an
invalid WebSocket text frame.
hle.data.remove wiped the name of a function sharing the address. Labels are
shared between data and function symbols, so removing the data label left the
function showing up in hle.func.list with an empty name.
hle.data.add silently did nothing outside a loaded module. GetModuleIndex()
returns -1 for e.g. a heap or stack address, and symbols under that index never
reach the active maps - so the add reported success while the symbol was
invisible to list, and rename/remove then failed with "No data symbol found".
Falls back to module index 0 ("no module, absolute address"), which is the right
answer for a label the user put somewhere after a memory.search.
hle.thread.list reported the thread's stack base address in a field called
initialStackSize. Renamed to initialStack, matching the SceKernelThreadInfo
field it comes from.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
main() only ever read argv[1], silently ignoring any further test
names despite both the usage text and the file's own top-of-file
comment claiming "one or more of the below" was supported. Rewrote
argument handling to collect every named test (or expand "all"),
validate all names up front (bailing with the usage listing if any is
unrecognized, rather than silently running a partial set), and run
them with the same pass/fail summary "all" already used - single-test
invocations now also get the "**** Running test X ****" banner "all"
always had, for consistency.
Verified: `UnitTest.exe CLZ MathUtil Path` now runs all three (was:
silently only CLZ); a bad name reports "Unknown test: X" plus usage
and exits 1; `all` and single-name behavior otherwise unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Headless.cpp, NativeApp.cpp, and SDLMain.cpp each still hand-parsed a few
argv flags directly (mount/log/state/ignore/loglevel in headless and the
app, xres/yres/dpi/scale in SDL), duplicating and in some cases conflicting
with the shared CommandLineOptions parser. Consolidate all of it into
CmdLine.cpp/.h so there's a single source of truth, and drop the now-dead
remain_argc/remain_argv filtering in SDLMain.cpp since NativeInit no longer
reads argv itself.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PfFvWzpHxErWgRhKqqSewN
Add unittest/TestTextureReplacer which creates a fictive texture pack
(textures.ini with readable invented hashes plus real PNG files), loads
it via the replacer, and verifies lookups, filtering, hashranges, mip
levels, and missing/ignored entries.
To make the replacer runnable outside the emulator:
- The constructor now accepts a null DrawContext (formats just default
to unsupported).
- FindReplacement/FindFiltering use the replaceEnabled_ member instead
of the global config.
- Added TextureReplacer::LoadPackForTesting() to load an ini from a
path directly.
The LZRC decompressor's only bounds check for output (and input) was a
debug-only _dbg_assert_msg_, which is a no-op in release builds. The
NPDRM demo block device also passed a hardcoded 1 MiB output length
while the real destination buffer (blockBuf_) could be as small as 2048
bytes, allowing a crafted NPDRM image to trigger an unbounded heap
overflow during game load.
Changes:
- rc_putbyte/rc_getbyte now enforce real bounds and set an error flag
instead of relying on debug asserts; decompression aborts with -1 on
overflow or truncated input.
- normalize() reads via rc_getbyte so it stays in bounds.
- Plain-text path clamps the copy size to both the output buffer and the
remaining input (and no longer interprets the size as signed).
- NPDRMDemoBlockDevice::ReadBlock passes blockSize_ (the real buffer
size) instead of 0x00100000 to lzrc_decompress.
- Add unittest/TestLzrc (synthetic input, no test data files): checks the
plain-text clamp, truncated input, and output overflow all fail safely.
- AGENTS.md: note to reuse existing format handlers/decompressors before
writing new ones.
A crafted zip with a parent-directory ("..") entry name could escape the
destination directory during extraction, writing arbitrary files on the
host (e.g. into startup/autostart folders). ExtractZipContents built the
output path by concatenating the raw zip entry name onto the destination
with no traversal check.
Changes:
- Add HasParentDirComponent() utility in Core/Util/PathUtil and use it in
GameManager::ExtractZipContents to reject entries with a ".." component.
Guard both the directory-creation and file-writing passes.
- Expose ExtractZipContents as public for testing.
- Add unittest/TestZipSlip which crafts a zip with a "../evil.txt" entry
and verifies it is not written outside the destination directory.
