UriDecode() formed SRC_END - 2 unconditionally, a pointer before the
start of the buffer (UB) for a 0- or 1-byte input. IsLocalAbsolutePath()
indexed path[0]/path[1] on a std::string_view with no bounds check,
UB for an empty path (path[0]) or a 1-byte path on Windows (path[1]).
Neither was known to crash in practice, but both are real UB flagged
by hardened/UBSan builds and easy to trigger (e.g. an empty query
string, or listing the VFS root).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
ReadFrame() accepted a 64-bit client-supplied payload length with only
a top-bit check, and ReadPending() immediately resized a buffer by it
before any data had arrived - a single frame claiming a huge length
(reachable via the WebSocket debugger endpoint) could trigger a
multi-exabyte allocation attempt. Now rejected up front (both the
single frame and the fragmented-message total) against a 64MB cap.
Also replaced &payload[0]/&vector[0] with .data() in the send/receive
paths - operator[] on a possibly-empty vector (e.g. an empty PING) is
UB even when the result is never dereferenced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
ReadFile()/ReadSingleFileFromZip() allocated/resized directly off a
zip entry's declared uncompressed size with no sanity check. A
crafted size near UINT64_MAX would wrap ReadFile()'s "size + 1" to 0,
allocating almost nothing while zip_fread() still writes the full
declared size into it - a length-field-driven heap overflow from a
malicious zip/texture pack. Both now reject entries above a generous
4GB cap. Also fixes GetFileInfo() reading zstat.name[strlen(name)-1]
unchecked, which underflows to SIZE_MAX for a zero-length entry name.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
ParseHttpHeader() used strchr(buffer, ' ') unconditionally as endptr,
even though the parser explicitly supports HTTP/0.9-style requests
with no trailing space/version (type = SIMPLE). A request line like
"GET /" with no space made strchr return null, and nullptr - buffer
truncated to a garbage length driving new[]/memcpy. Falls back to the
end of the line when no space is found, and clamps param_length to
avoid a similar issue when '?' appears after the (missing) space.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
GrabMemSpace() ignored a NULL return from CreateFileMapping and
returned true anyway, unlike every other platform's arena backend
(Darwin, Android, Posix all check and return false on failure). Under
memory pressure this let startup proceed to CreateView()/MapViewOfFileEx
with an invalid handle instead of failing cleanly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
Update()/Entries()/ClickEntry()/Show()/SetProgressBar()/RemoveProgressBar()
locked mutex_ around entries_, but CancelById/ShowAchievementUnlocked/
ShowAchievementProgress/ShowChallengeIndicator/ShowLeaderboardTracker/
ClearAchievementStuff/SetClickCallback/SetFlags didn't. Core/WebServer.cpp
runs its own thread and calls SetClickCallback() while the main thread's
Update() concurrently erases/iterates the same vector every frame.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
DoVector already rejects an attacker/corruption-controlled size that
would resize far beyond what's actually left in the savestate buffer.
DoList/DoDeque/DoMap/DoMultimap/DoSet never got the same treatment -
a corrupted count field (e.g. 0xFFFFFFFF) drove an immediate huge
resize (list/deque) or an unbounded loop of allocations (map/set)
before any per-element bounds checking kicked in. All five now check
the declared count against PointerWrap::Remaining() first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
MODE_READ built the string via x = (char*)*p.ptr, which strlen()s for
a NUL terminator. CheckRead(stringLen) only guarantees stringLen bytes
are available in the buffer, not that one of them is a NUL - a
corrupted savestate missing the terminator caused strlen to scan past
the checked region. Now uses a length-bounded assign(), matching how
the wstring/u16string siblings already do this correctly via memcpy.
Also tightens the stringLen validity check to require >= 1 (matching
what a real serialized string always has), so stringLen - 1 can't
go negative for a corrupted stringLen of 0.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
When vsnprintf's return value (the would-have-been length) was >=
sizeof(buffer), the code logged a truncation warning but then still
memcpy'd that full, untruncated length out of the 4096-byte stack
buffer, reading past its end. retval is now clamped to what vsnprintf
actually wrote before use.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
initPostfixExpression() filled a fixed char subStr[256] with no bound
check while tokenizing numeric literals and identifiers. A 256+ char
token in a debugger expression (breakpoint condition, watch,
memory.search) - settable over the network via the WebSocket debugger
- smashed the stack. Now bails out with an error once the token
reaches the buffer size.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
VR_DestroyRenderer() freed projections without nulling it, and
VR_GetView() indexed it with no initialized check - a latent
use-after-free if VR_GetView were ever called between a destroy and
the next VR_InitRenderer. Not currently reachable (VR_DestroyRenderer
is only called from inside VR_InitRenderer, synchronously followed by
reallocation), but cheap to close off.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
INVR_Vibrate()'s channel selection was `i & chan` (bitwise AND) inside
a loop over i in [0,2), instead of just using chan directly as the
index. For chan=0 (left controller, per the only call site iterating
j in [0,2)), i & 0 is always 0, so the "if (channel)" check was never
true and vibration_channel_duration/intensity were never set - the
left controller silently never vibrated. chan=1 (right) happened to
work by coincidence (i=1 gives 1 & 1 == 1). chan is now used directly
as the array index, with no loop needed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
FromConfigString() indexed parts[0]/parts[1] from SplitString() with
no check that at least 2 parts were produced - a malformed/truncated
line in a hand-edited controls.ini (no '-') was an OOB vector access.
Separately, DEVICE_ID_ANY (-1) didn't round-trip correctly:
ToConfigString() formats it as e.g. "-1-5", but splitting that on '-'
produces "", "1", "5" instead of "-1", "5" - deviceId decoded to 0 and
keyCode to 1 instead of 5. Rather than special-casing the negative
sign to make it round-trip, just reject DEVICE_ID_ANY mappings
outright - it's not something we want to support, and the whole ANY
concept is likely going away. Preserves the existing (tested) behavior
of tolerating a MultiInputMapping string and parsing just its first
mapping, via atoi()'s stop-at-first-non-digit behavior.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
The only guard against writing past the fixed 65536-entry verts_
array was _dbg_assert_msg_, which is compiled out entirely outside
_DEBUG - a large enough single batch (e.g. Circle()/CircleSegment()
with a big segment count, or a large UI list without an intervening
Flush()) silently corrupted the heap in release builds. Added a real
bounds check that drops the vertex instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
TextureLoadTask::Run() only freed the VFS-read file buffer on the
success path - a corrupt/truncated image in a texture pack or
replacement texture directory (valid magic, bad payload) leaked the
whole file's bytes on every load attempt.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
The memmove length was computed from size_ after ExtendByOne() had
already bumped it, so it moved (oldSize - pos + 1) elements instead of
(oldSize - pos) - reading one uninitialized element past the old data
and writing one element past the new logical size. Currently masked
by ExtendByOne()'s growth policy always leaving capacity slack, but
a real overflow waiting for that assumption to not hold. Now captures
the old size before extending.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
next_crlf_offset() called peek() one byte past the currently buffered
data whenever a '\r' was the very last byte received (a normal TCP
fragmentation boundary) - peek() has no way to signal "not enough
data yet" and just asserts. Now checks there's actually a next byte
before peeking.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
next() would read a truncated lead byte's "second byte" unconditionally,
even when that byte was actually the string's null terminator - leaving
index_ one past the terminator, so a subsequent end()/next() call read
one byte out of bounds. Now checks for the terminator before consuming
it, returning INVALID without advancing past it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
RIFFReader::ReadData() trusted its count argument completely and
memcpy'd straight from the internal buffer with no bounds check.
Hardened it to clamp against the buffer and zero-fill any shortfall,
as defense in depth.
The actual reachable bug was in BackgroundAudio.cpp: it read a WAV
'smpl' chunk into a vector sized by GetCurrentChunkSize(), then
unconditionally indexed smplData[28] (and, for the loop array,
smplData[36]) with no check that the chunk was actually that large -
a short/corrupt chunk in a game's background-music WAV caused a heap
OOB read. Also fixes &smplData[0] being UB when the chunk is empty.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
These dereferenced get()'s result unconditionally, unlike the
two/three-arg "OrDefault" overloads which check. Hit on externally
sourced JSON: UI/Store.cpp reads the remote homebrew-store listing,
UI/DriverManagerScreen.cpp reads user-supplied GPU driver package
metadata - a field simply missing from either crashed the app instead
of failing gracefully.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
hTriggerEvent/criticalSection/logPending_ were only initialized when
useThread_ was true, but Log() falls back to SendToThread() (which
uses all three) whenever the console isn't open yet, regardless of
useThread_. With useThread_ false, that meant an uninitialized
CRITICAL_SECTION and a null logPending_ write. Dead path today (only
reachable via a commented-out line), but now initializes unconditionally.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
SetFileLogPath() and Shutdown() closed/reassigned fp_ without holding
logFileLock_, while LogLine() only locked around the actual fprintf,
after already reading fp_ unlocked. A concurrent SetFileLogPath() or
Shutdown() could close the FILE* mid-write from another thread. Now
all three paths hold logFileLock_ across the check-and-use of fp_.
Also fixes fp_ being left dangling (non-null but closed) if
SetFileLogPath() runs while File output is disabled.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
The notify_one chain (each waiter wakes exactly one more) was correct
but fragile and non-obvious. notify_all is simpler and just as cheap
here since Arrive() is not a hot path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
SimpleParallelTask/LoopRangeTask never overrode Cancellable(), so a
ThreadManager::Teardown() racing with an in-flight parallel loop would
drop them without ever calling counter_->Count(), leaving anyone
blocked in WaitableCounter::Wait() stuck forever. Cancel() now counts
down the same as a normal completion.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
- ~ThreadManager() never stopped worker threads before freeing the
global context. If Teardown() wasn't reached before process exit
(e.g. an early return between Init() and a caller's shutdown path),
the still-running threads kept touching freed mutexes/queues/condvars,
causing intermittent crashes on exit. The destructor now tears down
if still initialized.
- Teardown() silently leaked any non-cancellable task still sitting in
a queue (global or per-thread) at shutdown time - such tasks were
never run nor released, since nothing drains those queues once the
worker threads have been marked cancelled/joined. Now every queued
task is properly cancelled-or-warned and released.
- Added an assert against a divide-by-zero in EnqueueTask's round-robin
fallback, which would trigger if Init() were ever called with zero
compute threads.
- Removed Task::id()/Kind() and ThreadManager::TryCancelTask(), which
were unused dead code (TryCancelTask was a no-op with no callers).
- Fixed a stale comment on EnqueueTaskOnThread referencing a parameter
that doesn't exist, and an auto-typed ternary that misleadingly read
like a deque array copy (it's a pointer, not a copy).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4QAoxV2KY7ek4PcZw3WvY
Our Qt backend has long been left behind and doesn't even support Vulkan
currently. There would be a lot of work to make it viable, and I don't
think anyone is really interested.
ImGui on SDL will soon fulfill the need for a more classic user interface
with a menu bar on Linux, and on Mac we already have a native UI.
