Assorted correctness fixes - #572
Open
AdamCoulterOz wants to merge 14 commits into
Open
Conversation
AdamCoulterOz
force-pushed
the
fix/assorted-correctness
branch
2 times, most recently
from
August 4, 2026 04:09
02fa8b3 to
460a750
Compare
Author
|
One thing worth pointing out in this branch, since it is not what the PR is about: b7de496 braces 25 bodies in They came from #564. Taking 🤖 Addressed by Claude Code |
AdamCoulterOz
force-pushed
the
fix/assorted-correctness
branch
from
August 4, 2026 10:36
460a750 to
fac5755
Compare
The outer decoding loop tests the output count only between tokens, but a match token unconditionally emits up to F (30) bytes. Entering that inner loop with a single byte of output left therefore wrote up to 29 bytes past the caller's buffer, and every caller sizes its buffer from the length recorded in the file being read. A truncated or hand-edited saved game whose last token is a long match is enough to corrupt the heap. Bound the inner copy by the remaining output space as well, so the decoder never produces more than the requested number of bytes. The new tests decompress a compressed run of identical bytes into every possible prefix length and check that the bytes beyond the requested size are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Not every backend reports a dismissed dialog. Starting the result at zero made those look like the first button was pressed; start it where the backends that do report leave it instead. The product dialog this was found through is gone - the launcher chooses now - but the message box is still used elsewhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sys_get_time_ns() returned the system clock, i.e. nanoseconds since 1970, but every one of its callers subtracts two readings to get a duration. CalcTics() starts from a zero timestamp, so its very first difference is the whole epoch offset and multiplying that by the tick base overflows a signed 64-bit integer on every launch, leaving the first frame with a meaningless delay. The other callers - the movie player, the input and text-presenter delays and the cursor blink - are merely at the mercy of NTP and manual clock changes. Add get_elapsed_time_ns(), backed by SDL_GetTicksNS(), and measure with it. get_current_time_ns() stays where a wall-clock reading is what is actually wanted, which today is the screenshot file name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SDL reports both the wheel amount and the relative mouse motion as floats. Truncating them to int discards anything below one unit, and on the devices that report fractions - a trackpad, a high-resolution wheel, a Wayland continuous axis - that is every event: the wheel bindings never fire at all and slow aiming does not move the view. Take the wheel amount from the fields SDL already accumulates into whole ticks, and carry the fraction of the motion over to the next event so that nothing is lost no matter how slowly the mouse is moved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The single slot backing the texture lock allocator is an array of bytes sitting right after a bool, so the whole structure is one-byte aligned and the slot itself starts at an odd address. Placement-constructing a polymorphic object there is undefined: a replacement operator new has to hand back storage suitably aligned for what is about to be built in it. The software renderer takes a lock once or twice per frame, so this happens constantly - harmless on the usual desktop targets, a trap for an alignment sanitiser and a bus error on anything stricter. Declare the slot with the alignment of the object it stores. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each search path publishes a pointer into its own pathname string. The entries live in a vector that is grown one entry at a time, so adding a search path move-constructs the earlier ones into fresh storage and frees the old block. A short pathname lives inside the string object itself, so its published pointer then refers to the freed block, and the asset probe and the product-selection dialog both read it. Hold the entries by pointer so that growing the list leaves them where they are. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The open flag was never raised, so is_initialized() answered no even for a fully working virtual file system, and terminate() dropped the flag and the logger while leaving the search paths in place. Since initialize() starts by terminating, re-initializing kept every archive file open and appended a second copy of every search path, which also reverses the order files are resolved in. Raise the flag once the search paths are added and release them when terminating. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The result of SDL_GL_CreateContext went unchecked, so a failed creation produced a fully formed context object wrapping a null handle and threw away the reason for the failure. Probing for an ES 2.0 context on a desktop without a GLES driver does exactly that, and the attribute queries that follow do not fail either, so the renderer only gives up later with a misleading complaint about a null version string. Report the failure where it happens, like every other call in this layer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The software backend creates its window and never applies the window mode, so it always comes up windowed no matter what the archived setting says. The hardware backend applies it as soon as its renderer exists, and the only other way in is the video menu, so a fullscreen player who starts with - or falls back to - the software renderer is stuck in a window sized to the desktop until they toggle the mode by hand. Apply the window mode once the renderer exists, as the hardware backend does; the layout is recomputed from the resulting window size. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The buffer the description is read into is one byte longer than the longest description, but only the first thirty-one bytes were zeroed. A description that uses every character - the input field allows exactly that many - fills those bytes completely and leaves the terminator byte whatever the stack happened to hold, so copying the description out ran past the buffer and past the end of the name it was copied into. Zero the whole buffer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The wide string helper leaves its storage pointer alone when the length probe of the UTF-8 input fails, and the member has no default initialiser, so the object is left holding whatever was on the stack. Every caller tests the pointer against null to detect a failure - which is what the other failure path sets it to - so an indeterminate pointer sails through that test and is handed to the registry functions, and the destructor then hands the same value to operator delete. Initialise the pointer and clear it on that path too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A wall is always a 64 by 64 page and every reader treats it as one - the extractor, the hardware texture manager and the ray caster all walk the full four kilobytes without asking how big the chunk claims to be. The file is loaded into an allocation of exactly its own size, so a wall chunk that declares a smaller size and sits at the end of the file is read past the end of that allocation. Reject such a file while validating the chunk table, where the rest of the layout is already checked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Taking the diagnostic out of a validation block took its braces with it, since the block held only the assert and the return. The earlier bracing pass then had nothing to find: these read as one-line bodies that had always been one-line bodies, though the file itself disagreed with them four lines further down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wide-string storage now starts null on its own, so clearing it on the failed path repeats what the declaration did. The VFS search loop's body sits under a line this branch changed, and was the only one of its kind left bare there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AdamCoulterOz
force-pushed
the
fix/assorted-correctness
branch
from
August 4, 2026 15:52
fac5755 to
10ddba0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From a correctness review of wip; twelve small independent fixes.
🤖 Generated with Claude Code