Build: an opt-in AddressSanitizer, macOS only for now

.gitignore

@@ -1,6 +1,7 @@
 .svn
 .scc
 /build
+/build-asan
 /Source/cache_font
 /AttribEditor/TreeListCtrl/output
 **/.Debug

CMakeLists.txt

@@ -328,6 +328,42 @@ if(NOT WIN32)
     )
 endif()
 
+# ---------------------------------------------------------------------------
+# AddressSanitizer
+# ---------------------------------------------------------------------------
+# -DVISTA_ASAN=ON builds the engine — and only the engine — instrumented.
+#
+# The position in this file is the whole trick: compile and link options set on a
+# directory reach the subdirectories added *after* them, and every third-party
+# dependency is fetched above. So SDL, FreeType, miniaudio and (on a cold build)
+# the vendored DXC keep compiling clean, while everything in the
+# add_subdirectory() list below — XLibs.Net included, since XZip and XBuffer are
+# exactly where a buffer bug would hide — is instrumented. ffmpeg is out of reach
+# either way: it is an ExternalProject with a configure script of its own.
+#
+# Instrumenting only part of a program is what ASan is built for. The clean
+# libraries simply report nothing of their own, and the final link of Game is
+# what pulls the runtime in.
+#
+# macOS only for now. Windows would need MSVC's /fsanitize=address plus its
+# clang_rt DLL staged next to the executable, and Linux its own libasan on the
+# link line; neither has been tried, and a silently ignored flag is worse than a
+# refused one.
+option(VISTA_ASAN "Build the engine with AddressSanitizer (macOS only for now)" OFF)
+if(VISTA_ASAN)
+    if(NOT APPLE)
+        message(FATAL_ERROR
+            "VISTA_ASAN is macOS-only for now. See the comment above this check in "
+            "CMakeLists.txt for what the other two platforms would need.")
+    endif()
+    # -fno-omit-frame-pointer because the build type we actually run is
+    # RelWithDebInfo: -O2 drops the frame pointer, and with it the readable half
+    # of every ASan report.
+    add_compile_options(-fsanitize=address -fno-omit-frame-pointer)
+    add_link_options(-fsanitize=address)
+    message(STATUS "AddressSanitizer: ON (engine targets only; dependencies stay uninstrumented)")
+endif()
+
 add_subdirectory(XLibs.Net)
 add_subdirectory(Util)
 add_subdirectory(AI)

Documents/Build-PORTING.md

@@ -51,6 +51,55 @@ Two include directories serve everyone now: `Platform/d3d9_compat` (D3D9 declara
 render headers still name its types, and on Windows this *deliberately shadows* the SDK's
 `d3d9.h`, since a retired backend only has to parse) and `Network/dplay8.h`.
 
+## AddressSanitizer
+
+```
+cmake -B build-asan -DCMAKE_BUILD_TYPE=RelWithDebInfo -DVISTA_ASAN=ON
+```
+
+**macOS only for now**, and the configure fails loudly anywhere else 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; neither has been tried.
+
+The option is declared **below every `FetchContent_MakeAvailable`** in the root
+`CMakeLists.txt` and that placement is the whole mechanism: `add_compile_options` reaches the
+subdirectories added *after* it, so SDL, FreeType, miniaudio and DXC stay clean while every
+engine module — `XLibs.Net` included, since `XZip` and `XBuffer` are where a buffer bug would
+hide — is instrumented. ffmpeg is out of reach either way; it is an ExternalProject with its
+own configure. Partial instrumentation is fine: the clean libraries report nothing of their
+own, and Game's link pulls the runtime in.
+
+Two things to know before reading a report:
+
+- **`RelWithDebInfo` is `-O2`, which drops the frame pointer**, so the option adds
+  `-fno-omit-frame-pointer` — without it half of every stack trace is unreadable.
+- **Leak detection does not exist here.** LeakSanitizer is unsupported on Darwin
+  (`detect_leaks=1` aborts with "not supported on this platform"); ASan on macOS finds
+  overflows and use-after-free, not leaks.
+
+The game reads its data from the working directory:
+
+```
+cd GameData && ASAN_OPTIONS=intercept_strstr=0 ../build-asan/Game/Game
+```
+
+**That option is not optional in a `Debug` build**, and it is the first thing this port trips
+over. Without it the game appears to hang during `loadAllLibraries()` — the last line printed
+is `SDLMinimapRenderer: …` and it sits there at 100% CPU. It is not hung and it is not a
+deadlock: `XPrmIArchive::getToken` (`XPrmArchive.cpp:832`) runs `strstr(i, "\"")` once per
+quoted literal while parsing the UI attributes, with `i` pointing into the whole remaining file
+buffer. Native `strstr` stops at the first quote a few bytes on; ASan's interceptor measures
+the *entire* haystack with `internal_strlen` first, to poison-check the range it might read. So
+every literal pays a scan of everything after it and an O(n) parse turns into O(n²) —
+`sample`(1) puts 2786 of 2795 samples in `internal_strlen`. `intercept_strstr=0` turns off that
+one interceptor and leaves the rest of ASan intact; the only checking lost is on `strstr`'s own
+reads. `RelWithDebInfo` has the same shape with a small enough constant to get through.
+
+Reports go to **stderr**, which is where this engine's own logging goes too, so a report lands
+in the log in the place it happened. ASan writes them with `write(2)` rather than stdio, so
+nothing buffers them away; `ASAN_OPTIONS=log_path=/tmp/asan` diverts them to `/tmp/asan.<pid>`
+if the interleaving gets in the way. Under `lldb` the process stops on the report either way.
+
 ## Traps, by platform
 
 ### Linux — case sensitivity
@@ -150,6 +199,22 @@ Not portability defects — actual bugs, on every platform:
   The original 32-bit build was consistent. Fixed in the replay format (`UniverseX`),
   `ParameterSet` and `NParticleKey`; **the wire fields say `int32_t`/`uint32_t` now**, and the
   pattern is worth looking for wherever reader and writer sit in different files.
+- **`%08lX` for a 32-bit field.** The first thing ASan reported, on the first run: `XGUID`
+  printed its GUID with `"%08lX, %04hX, %04hX, {%02wX, …}"`, and under LP64 the `l` takes 64
+  bits off the varargs for a 32-bit `Data1` — a 16-digit number, five bytes off the end of the
+  80-byte buffer, and every argument after it shifted by one. `sscanf` read it back with
+  `"%lx"` *into* `Data1`, writing eight bytes into four, over `Data2` and `Data3`. So every
+  GUID this build wrote — the campaign progress in `passedMissions`, the mission headers — was
+  garbage. It formats with `std::format` now, which takes each width from the argument's type;
+  the text is the same canonical 78-character form the 32-bit build wrote.
+- **A temporary bound to a reference member, in the collision path.**
+  `GeomBox::bodyCollision` built `CD::CDDuality penetrate(CD::Transform(X12, box_), …)`, and
+  `CDDuality` keeps both arguments as `const Convex&`. The transform was a temporary, dead at
+  the semicolon, and the next line read through the reference into the freed stack slot — on
+  every moving unit, every quant. This is the `-Wno-error=address-of-temporary` habit
+  (see above) applied where it does *not* hold: that exemption is only sound while the
+  temporary is used inside its own full-expression. Naming the local fixes it. ASan's
+  stack-use-after-scope is what surfaced it; nothing else would have.
 - **The vendored zlib compiled against the system's `zlib.h`.** `XLibs.Net/XZip/zlib` was on
   nobody's include path, so minizip's `#include "zlib.h"` quietly resolved to
   `/usr/include/zlib.h` — a different zlib than the `.c` files beside it.

Physics/Geom.cpp

@@ -396,7 +396,11 @@ bool GeomBox::bodyCollision(RigidBodyCollision* body1, RigidBodyCollision* body2
 		X12.invert();
 		X12.trans().add(X12.xformVect(center1));
 		X12.rot().postmult(body1->orientation());
-		CD::CDDuality penetrate(CD::Transform(X12, box_), safe_cast<const GeomBox*>(geom2)->box());
+		// CDDuality holds both arguments by reference (CDDual.h), so the transform has
+		// to be a named local: as a temporary it died at the end of this declaration and
+		// the computeBoxBoxPenetration() call below read a dead stack slot.
+		CD::Transform box1InBox2Space(X12, box_);
+		CD::CDDuality penetrate(box1InBox2Space, safe_cast<const GeomBox*>(geom2)->box());
 		//if(!penetrate.computePenetrationDistance(cp1, cp2))
 		if(!penetrate.computeBoxBoxPenetration(cp1, cp2))
 			return false;

Physics/MultiBodyDispatcher.cpp

@@ -77,7 +77,11 @@ bool MultiBodyDispatcher::test(RigidBody& b1, RigidBody& b2, MatXf& Xr1r2, Conta
 				}
 				else{
 					start_timer_auto(CDBox, STATISTICS_GROUP_PHYSICS);
-					CD::CDDuality penetrate(CD::Transform(Xr1r2, b1.box), b2.box);
+					// Named, not a temporary: CDDuality keeps a reference to it and uses
+					// it below, past the end of this declaration. Same defect as the one
+					// ASan caught in GeomBox::bodyCollision.
+					CD::Transform box1InBox2Space(Xr1r2, b1.box);
+					CD::CDDuality penetrate(box1InBox2Space, b2.box);
 					if(!penetrate.computePenetrationDistance(cp1, cp2))
 						return false;
 					Xr1r2.invXformPoint(cp1);

Util/FileUtils/XGUID.cpp

@@ -2,6 +2,7 @@
 #ifdef _WIN32
 #  include <objbase.h>
 #endif
+#include <format>
 #include "FileUtils/XGUID.h"
 #include "Serialization/Serialization.h"
 
@@ -21,29 +22,39 @@ void XGUID::generate()
 
 bool XGUID::serialize(Archive& ar, const char* name, const char* nameAlt)
 {
-	static char buf[80];
-
-	if(ar.isOutput()){
-		int size = sprintf(buf,
-			"{0x%08lX, 0x%04hX, 0x%04hX, {0x%02wX, 0x%02wX, 0x%02wX, 0x%02wX, 0x%02wX, 0x%02wX, 0x%02wX, 0x%02wX}}",
+	string data;
+
+	// std::format takes each field's width from its type instead of from a conversion
+	// the caller has to keep in sync by hand. The printf original did not survive the
+	// move to 64 bits: "%08lX" for a 32-bit Data1 took 64 bits off the varargs, which
+	// printed a 16-digit number, overran the 80-byte static buffer it wrote into, and
+	// shifted every argument after it by one. (Its "%02wX" was not a conversion at all,
+	// only an MSVC-ism.) The text is unchanged — the canonical 78-character form the
+	// 32-bit build wrote, which is what the profiles and mission headers hold.
+	if(ar.isOutput())
+		data = std::format(
+			"{{0x{:08X}, 0x{:04X}, 0x{:04X}, {{0x{:02X}, 0x{:02X}, 0x{:02X}, 0x{:02X}, 0x{:02X}, 0x{:02X}, 0x{:02X}, 0x{:02X}}}}}",
 			Data1, Data2, Data3, Data4[0],
 			Data4[1], Data4[2], Data4[3], Data4[4], Data4[5], Data4[6], Data4[7]);
-		xassert(size < sizeof(buf));
-	}
-	else
-		*buf = 0;
-
-	string data(buf);
+
 	bool res = ar.serialize(data, name, nameAlt);
 
 	if(res && ar.isInput()){
-		int rb[8];
-		sscanf(data.c_str(),
-			"{%lx, %hx, %hx, {%hx, %hx, %hx, %hx, %hx, %hx, %hx, %hx}}",
-			&Data1, &Data2, &Data3,
-			&rb[0], &rb[1], &rb[2], &rb[3], &rb[4], &rb[5], &rb[6], &rb[7]);
-		for(int idx = 0; idx < 8; ++idx)
-			Data4[idx] = (rb[idx] & 0xFF);
+		// sscanf offers no such type checking, so read every field into the same type
+		// and narrow afterwards: "%lx" straight into Data1 wrote eight bytes into four
+		// of them, over Data2 and Data3. A string that does not parse now leaves the
+		// GUID alone rather than half-assigning it.
+		unsigned rd[11] = {0};
+		if(sscanf(data.c_str(),
+				"{%x, %x, %x, {%x, %x, %x, %x, %x, %x, %x, %x}}",
+				&rd[0], &rd[1], &rd[2], &rd[3], &rd[4], &rd[5],
+				&rd[6], &rd[7], &rd[8], &rd[9], &rd[10]) == 11){
+			Data1 = rd[0];
+			Data2 = rd[1] & 0xFFFF;
+			Data3 = rd[2] & 0xFFFF;
+			for(int idx = 0; idx < 8; ++idx)
+				Data4[idx] = rd[idx + 3] & 0xFF;
+		}
 	}
 
 	return res;