Build: an opt-in AddressSanitizer, macOS only for now - #30
Merged
Conversation
-DVISTA_ASAN=ON builds the engine instrumented. The option is declared in the root CMakeLists.txt below every FetchContent_MakeAvailable and above the add_subdirectory() list, and that placement is the whole mechanism: compile and link options set on a directory reach only the subdirectories added after them, so SDL, FreeType, miniaudio and the vendored DXC keep compiling clean while every engine module is instrumented. XLibs.Net is in, deliberately -- XZip and XBuffer are exactly where a buffer bug would hide. ffmpeg is out of reach either way; it is an ExternalProject with a configure script of its own. Instrumenting part of a program is what ASan is built for: the clean libraries report nothing of their own, and the final link of Game is what pulls the runtime in. Verified -- all 373 engine translation units carry -fsanitize=address and all 266 dependency ones do not, libUtil.a has 345 undefined __asan_* symbols, and the linked executable loads libclang_rt.asan_osx_dynamic.dylib. -fno-omit-frame-pointer rides along because the configuration we actually run is RelWithDebInfo, i.e. -O2, which drops the frame pointer and with it the readable half of every report. macOS only, and a configure elsewhere fails outright rather than ignoring the flag: Windows would need MSVC's /fsanitize=address plus its clang_rt DLL staged beside the executable, Linux its own libasan on the link line, and neither has been tried. A silently ignored sanitizer flag is worse than a refused one. No CI job. The macOS runner can build the game but never run it, so an ASan build there would roughly double the job for no runtime coverage. Run it with ASAN_OPTIONS=intercept_strstr=0, which the register explains at length: without it a Debug build looks hung during loadAllLibraries(), because XPrmIArchive::getToken calls strstr() once per quoted literal into the whole remaining file buffer and ASan's interceptor measures the entire haystack before each search -- an O(n) parse becomes O(n^2) and 2786 of 2795 samples land in internal_strlen. With the flag the same build reaches "Universe created" and exits on its own in 55s; without it, three runs never got there at all. It earns its keep immediately: the first run reported a global-buffer-overflow in XGUID::serialize, a "%08lX" reading 64 bits of varargs for a 32-bit field. That fix is a commit of its own, and there are certainly more where it came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AddressSanitizer's first report on this tree was a global-buffer-overflow in
XGUID::serialize, five bytes past a static char[80], and both directions of the
function were wrong under LP64.
Writing: "{0x%08lX, 0x%04hX, 0x%04hX, {0x%02wX, ...}}" against a 32-bit Data1.
The `l` takes 64 bits off the varargs, so it printed a 16-digit number, ran off
the end of the buffer, and -- because Apple's arm64 variadic convention packs
arguments at their natural size -- consumed the slots holding Data2 and Data3,
shifting every argument after it. ("%02wX" is not a conversion at all, only an
MSVC-ism; ASan's printf interceptor flags it as unknown.)
Reading: sscanf("%lx", &Data1) wrote eight bytes into a four-byte field, over
Data2 and Data3 sitting next to it in the struct.
So every GUID this build wrote was garbage: the campaign progress in a profile's
passedMissions, and the worldGUID/missionGUID in every mission header.
It formats with std::format now, which takes each width from the argument's type
rather than from a conversion that has to be kept in sync by hand -- the same
class of bug cannot come back, and the fixed-size buffer (also a non-reentrant
static) goes away with it. The text is unchanged: the canonical 78-character
form the 32-bit build wrote, verified to round-trip identically.
sscanf has no std::format counterpart in C++20, so the read path keeps it but
reads every field into unsigned locals of one type and narrows afterwards,
never straight into struct fields of assorted widths. One behaviour change
worth naming: a string that does not parse now leaves the GUID alone instead of
half-assigning it.
Not a hot path -- the callers are the mission header and profile save, both file
I/O -- so std::format costs nothing that matters here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CD::CDDuality binds both of its constructor arguments to const Convex& members, and both call sites passed the first one as a temporary: CD::CDDuality penetrate(CD::Transform(X12, box_), geom2->box()); if(!penetrate.computeBoxBoxPenetration(cp1, cp2)) // reads a dead slot The CD::Transform dies at the semicolon; the call on the next line reads through penetrate.p_ into stack that is no longer alive. AddressSanitizer reports it as stack-use-after-scope, and the frame layout identifies the object exactly: the 64-byte ref.tmp is a CD::Transform (a MatXf held by value, a Convex&, and the vtable pointer). This is the MSVC habit that -Wno-error=address-of-temporary exists for in this tree, applied where the reasoning behind that exemption does not hold. Taking the address of a temporary is fine while the temporary is used within its own full-expression, which is the case at the ~63 sites the build notes cover. Here the object that captured the reference outlives the expression, so it is plain undefined behaviour that survived only because nothing had reused the stack yet. Naming the local is the whole fix -- MatXf is held by value and box_ is a member, so a named CD::Transform outlives the CDDuality that references it, and nothing else in either expression dangles (box() returns const CD::Box&). GeomBox::bodyCollision runs from UnitBase::testCollision, i.e. every moving unit every quant, and is what ASan aborted on in C2_M01. The MultiBodyDispatcher.cpp site is the same defect but is NOT verified: that file is an orphan excluded from the build (Physics/CMakeLists.txt), unbuildable as it stands -- the fix is there for whoever revives it. Not runtime-verified either: a non-interactive run never enters bodyCollision (an lldb breakpoint on it is not hit before the process exits), so the mission that aborted before now runs clean only because the path is not reached without a player driving units. The defect and the fix are unambiguous by construction; confirmation is the next interactive session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
No description provided.