Add AddressSanitizer support for clang-cl builds

.gitignore

@@ -9,3 +9,6 @@
 /CMakeUserPresets.json
 /CMakeSettings.json
 /lib/
+
+# AddressSanitizer reports (see SANITIZERS.md)
+/gamedir/asan.report.*

CMakeLists.txt

@@ -33,11 +33,8 @@ else()
   message(STATUS "Configuring WITHOUT link-time optimization ${IpoError}")
 endif()
 
-option(ADDRESS_SANITIZER OFF)
-if(ADDRESS_SANITIZER)
-  message(STATUS "AddressSanitizer ENABLED for non-Release builds")
-  add_compile_options($<IF:$<OR:$<CONFIG:Debug>,$<CONFIG:RelWithDebInfo>>,-fsanitize=address,>)
-endif()
+# declares the ADDRESS_SANITIZER option and the _asan genexpr used below
+include(cmake/AddressSanitizer.cmake)
 
 # keep frame pointers in both MSVC and clang-cl
 add_compile_options(/Oy-)
@@ -50,8 +47,9 @@ endif()
 set(usingMsBuild $<STREQUAL:${CMAKE_VS_PLATFORM_NAME},Win32>)
 
 # RakNetLibStatic.lib carries /DEFAULTLIB:LIBCMT and /DEFAULTLIB:libcpmt, so the
-# static runtime is forced on us until RakNet too is built from source
-set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
+# static runtime is forced on us until RakNet too is built from source. Debug
+# keeps its /MTd only when asan is off: asan needs the release CRT (see below).
+set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<AND:$<CONFIG:Debug>,$<NOT:${_asan}>>:Debug>")
 # make Debug builds /Z7 instead of /Zi so they can be sccache'd
 set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT
   "$<$<CONFIG:Debug,RelWithDebInfo>:Embedded>")
@@ -85,6 +83,9 @@ add_subdirectory("ext/zlib")
 add_subdirectory("ext/VFS")
 target_link_libraries(bfVFS PRIVATE 7z)
 
+# turn on asan for everything defined below; 3rd party libs above keep defaults
+ja2_asan_instrument_first_party()
+
 # from here on down our own code only: vendored code above keeps default flags
 include(cmake/Warnings.cmake)
 
@@ -104,7 +105,10 @@ add_subdirectory(wine)
 # takes it without complaint but emits an import descriptor with an empty thunk
 # table, so every Bink call reaches a stale address and BinkInitialize faults the
 # process during the intro. mingw/GNU ar cannot do this at all, see mingw.cmake
-if(NOT binkw32_lib)
+#
+# asan builds skip this entirely: they compile no-op Bink stubs into the exe
+# (ja2_asan_link_binkw32_stub) so no binkw32.dll is imported at all.
+if(NOT ADDRESS_SANITIZER AND NOT binkw32_lib)
   # Only the name shape differs by toolchain: lib.exe prepends the x86 leading
   # underscore to every name in the .def, llvm-lib and llvm-dlltool take the name as
   # written. The checked-in file carries the underscore for the llvm tools, so strip
@@ -132,7 +136,6 @@ if(NOT binkw32_lib)
 endif()
 
 set(Ja2_Libraries
-"${binkw32_lib}"
 "${CMAKE_SOURCE_DIR}/libexpatMT.lib"
 "winmm.lib"
 "ws2_32.lib"
@@ -143,6 +146,10 @@ Multiplayer
 smacker
 wine
 )
+# empty under asan (stubs compiled into the exe instead, see above)
+if(binkw32_lib)
+  list(APPEND Ja2_Libraries "${binkw32_lib}")
+endif()
 
 # static libraries whose translation units rely on Application preprocessor definitions.
 set(Ja2_Libs
@@ -204,6 +211,8 @@ foreach(app IN LISTS ApplicationTargets)
   target_link_libraries(${exe} PRIVATE ${Ja2_Libraries} legacy_stdio_definitions.lib)
   target_compile_definitions(${exe} PRIVATE ${compilationFlags} ${debugFlags})
 
+  ja2_asan_link_binkw32_stub(${exe})
+
   # language library for the application, e.g. JA2MAPEDITOR_i18n — one per app, all 8
   # languages compiled in, selected at runtime by BindLanguageStrings
   set(language_library ${exe}_i18n)

CMakePresets.json

@@ -51,11 +51,30 @@
       "cacheVariables": {
         "CMAKE_BUILD_TYPE": "Release"
       }
+    },
+    {
+      "name": "clang-cl-asan",
+      "inherits": "base",
+      "displayName": "clang-cl RelWithDebInfo (AddressSanitizer)",
+      "description": "AddressSanitizer build. Needs the C++ Clang tools for Windows component; see SANITIZERS.md.",
+      "vendor": {
+        "microsoft.com/VisualStudioSettings/CMake/1.0": {
+          "hostOS": [ "Windows" ],
+          "intelliSenseMode": "windows-clang-x86"
+        }
+      },
+      "cacheVariables": {
+        "CMAKE_BUILD_TYPE": "RelWithDebInfo",
+        "CMAKE_C_COMPILER": "clang-cl",
+        "CMAKE_CXX_COMPILER": "clang-cl",
+        "ADDRESS_SANITIZER": "ON"
+      }
     }
   ],
   "buildPresets": [
     { "name": "msvc-relwithdebinfo", "configurePreset": "msvc-relwithdebinfo" },
     { "name": "msvc-debug", "configurePreset": "msvc-debug" },
-    { "name": "msvc-release", "configurePreset": "msvc-release" }
+    { "name": "msvc-release", "configurePreset": "msvc-release" },
+    { "name": "clang-cl-asan", "configurePreset": "clang-cl-asan" }
   ]
 }

SANITIZERS.md

@@ -0,0 +1,53 @@
+# AddressSanitizer
+
+AddressSanitizer (asan) finds memory errors at runtime: out-of-bounds accesses,
+use-after-free, and similar. It reports the exact faulting access.
+
+## Requirements
+
+asan requires `C++ Clang tools for Windows` (MSVC's AddressSanitizer does not
+support -fsanitize-ignorelist).
+
+It is NOT installed by default . You can install it via the Visual Studio
+Installer.
+
+## Build and run
+
+Either use the included `clang-cl-asan` preset, or inherit from it in your own
+`CMakeUserPresets.json`
+
+## Debug from Visual Studio
+
+asan needs `clang_rt.asan_dynamic-i386.dll` at run time. MSVC ships this DLL
+with the x86 tools, but Visual Studio does not add that folder to the debugger
+PATH. To start the exe with F5, add the tools folder to the
+debugger PATH:
+
+1. Access the CMake Targets View in Solution Explorer
+2. Right-click the exe target (for example `JA2.exe`) and select `Add Debug Configuration`. 
+   Visual Studio creates `launch.vs.json` in the `.vs` folder and adds a configuration with
+   the correct `projectTarget`.
+3. Add an `env` block to that configuration:
+
+```json
+      "env": {
+        "PATH": "C:\\Program Files\\Microsoft Visual Studio\\18\\Community\\VC\\Tools\\MSVC\\14.51.36231\\bin\\Hostx64\\x86;${env.PATH}"
+      }
+```
+
+Set the first path to the `bin\Hostx64\x86` folder of your MSVC tools. To find
+it, search your Visual Studio installation for `clang_rt.asan_dynamic-i386.dll`.
+Your edition (`Community`), MSVC version (`14.51.36231`), and host (`Hostx64` or
+`Hostx86`) can differ.
+
+## Reading the report
+
+Every app is a GUI app with no console, so asan writes the report to a file by
+default:
+
+```
+gamedir/asan.report.<pid>
+```
+
+When asan finds an error, the run stops and the report names the faulting access.
+Set the `ASAN_OPTIONS` environment variable to change asan behaviour.

cmake/AddressSanitizer.cmake

@@ -0,0 +1,50 @@
+# AddressSanitizer wiring, kept in one place. Include this early: it declares
+# the option and the _asan generator expression that the CRT selection in the
+# root CMakeLists depends on. The two functions are ordered hook points the
+# root calls at the right moments:
+#   ja2_asan_instrument_first_party() — after the vendored add_subdirectory()s
+#     (they must keep default flags), before our own targets are defined.
+#   ja2_asan_link_binkw32_stub(<exe>) — per executable, links the Bink stubs in.
+
+option(ADDRESS_SANITIZER "Enable AddressSanitizer for Debug and RelWithDebInfo" OFF)
+
+# asan only on the debuggable configs, shared by the flag and the CRT choice
+set(_asan "$<AND:$<BOOL:${ADDRESS_SANITIZER}>,$<OR:$<CONFIG:Debug>,$<CONFIG:RelWithDebInfo>>>")
+
+if(ADDRESS_SANITIZER)
+  message(STATUS "AddressSanitizer ENABLED for Debug and RelWithDebInfo (first-party code only)")
+  message(STATUS "  Bink stubbed into the exe; no binkw32.dll import, so the asan exe runs from gamedir untouched")
+  # keep MSVC-STL container annotations uniform across un-instrumented TUs
+  add_compile_definitions("$<${_asan}:_DISABLE_STL_ANNOTATION>")
+  # retail binkw32.dll cannot load in an asan process (image base 0x30000000 is
+  # the 32-bit shadow). __RADINEXE__ makes bink.h declare the Bink functions as
+  # in-exe calls instead of dllimports (sgp/RAD.H), so ja2_asan_link_binkw32_stub
+  # can satisfy them from sgp/binkw32_stub.c and the exe imports no binkw32.dll.
+  # Configure-time, not per-config: it must stay in lockstep with dropping the
+  # binkw32 import library from the link (see root CMakeLists).
+  add_compile_definitions(__RADINEXE__)
+endif()
+
+function(ja2_asan_instrument_first_party)
+  # instrument first-party code; /bigobj for the TUs asan inflates past the COFF
+  # section cap (LanguageStrings.cpp). The ignorelist opts individual functions
+  # with 32-bit inline __asm out of instrumentation (see cmake/asan-ignorelist.txt).
+  # Empty when the option is off.
+  add_compile_options(
+    "$<${_asan}:-fsanitize=address;-fsanitize-ignorelist=${CMAKE_SOURCE_DIR}/cmake/asan-ignorelist.txt;/bigobj>")
+
+  # clang-cl/lld-link do not infer the asan runtime; /WHOLEARCHIVE is invalid for llvm-lib
+  if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    add_link_options(
+      "$<$<AND:${_asan},$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>>:clang_rt.asan_dynamic-i386.lib;/WHOLEARCHIVE:clang_rt.asan_static_runtime_thunk-i386.lib>")
+  endif()
+endfunction()
+
+function(ja2_asan_link_binkw32_stub exe)
+  # Compile the no-op Bink exports into the exe. With __RADINEXE__ above the game
+  # calls them directly, so this replaces the binkw32.dll the retail import
+  # library would have pulled in.
+  if(ADDRESS_SANITIZER)
+    target_sources(${exe} PRIVATE "${CMAKE_SOURCE_DIR}/sgp/binkw32_stub.c")
+  endif()
+endfunction()

cmake/asan-ignorelist.txt

@@ -0,0 +1,104 @@
+# AddressSanitizer ignorelist. Functions with 32-bit inline __asm blocks:
+# asan's shadow-base register reservation starves a register the asm needs,
+# so instrumenting them fails to compile. Excluded from instrumentation only
+# (still linked and called); the rest of each translation unit stays checked.
+# Patterns end in @@ so each matches one clang-cl (MSVC) mangled name exactly,
+# never a longer-named sibling (e.g. Blt16BPPTo16BPP vs Blt16BPPTo16BPPTrans).
+# Regenerate: grep the *.cpp for __asm / _asm and list the enclosing functions.
+
+[address]
+# sgp/vobject_blitters.cpp
+fun:*BlitZRect@@*
+fun:*Blt16BPPBufferPixelateRectWithColor@@*
+fun:*Blt16BPPBufferShadowRect@@*
+fun:*Blt16BPPBufferShadowRectAlternateTable@@*
+fun:*Blt16BPPDataTo16BPPBufferTransZClip@@*
+fun:*Blt16BPPDataTo16BPPBufferTransparentClip@@*
+fun:*Blt16BPPTo16BPP@@*
+fun:*Blt16BPPTo16BPPMirror@@*
+fun:*Blt8BPPDataSubTo16BPPBuffer@@*
+fun:*Blt8BPPDataTo16BPPBuffer@@*
+fun:*Blt8BPPDataTo16BPPBufferFullTransparent@@*
+fun:*Blt8BPPDataTo16BPPBufferHalf@@*
+fun:*Blt8BPPDataTo16BPPBufferHalfRect@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensity@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensityClip@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensityZ@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensityZClip@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensityZNB@@*
+fun:*Blt8BPPDataTo16BPPBufferIntensityZNBClip@@*
+fun:*Blt8BPPDataTo16BPPBufferMask@@*
+fun:*Blt8BPPDataTo16BPPBufferMonoShadow@@*
+fun:*Blt8BPPDataTo16BPPBufferMonoShadowClip@@*
+fun:*Blt8BPPDataTo16BPPBufferOutline@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineClip@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineShadow@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineShadowClip@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineZ@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineZClip@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineZNB@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineZPixelateObscured@@*
+fun:*Blt8BPPDataTo16BPPBufferOutlineZPixelateObscuredClip@@*
+fun:*Blt8BPPDataTo16BPPBufferShadow@@*
+fun:*Blt8BPPDataTo16BPPBufferShadowClip@@*
+fun:*Blt8BPPDataTo16BPPBufferShadowZ@@*
+fun:*Blt8BPPDataTo16BPPBufferShadowZClip@@*
+fun:*Blt8BPPDataTo16BPPBufferShadowZNB@@*
+fun:*Blt8BPPDataTo16BPPBufferShadowZNBClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransMirror@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadow@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowBelowOrEqualZNBClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowClipAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZ@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZClipAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNB@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBClipAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBObscured@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBObscuredAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBObscuredClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBObscuredClipAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransShadowZNBObscuredTest@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZ@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZClipPixelate@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZClipPixelateObscured@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZClipTranslucent@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNB@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBClipColor@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBClipPixelate@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBClipTranslucent@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBColor@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBPixelate@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZNBTranslucent@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZPixelate@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZPixelateObscured@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZTranslucent@@*
+fun:*Blt8BPPDataTo16BPPBufferTransparent@@*
+fun:*Blt8BPPDataTo16BPPBufferTransparentClip@@*
+fun:*Blt8BPPTo8BPP@@*
+fun:*FillRect16BPP@@*
+fun:*blendWithAlpha@@*
+# sgp/shading.cpp
+fun:*FindIndecies@@*
+# TileEngine/renderworld.cpp
+fun:*Blt8BPPDataTo16BPPBufferTransInvZ@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZIncClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZIncClipZSameZBurnsThrough@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZIncObscureClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZTransShadowIncClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZTransShadowIncClipAlpha@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZTransShadowIncObscureClip@@*
+fun:*Blt8BPPDataTo16BPPBufferTransZTransShadowIncObscureClipAlpha@@*
+fun:*IsTileRedundent@@*
+fun:*Zero8BPPDataTo16BPPBufferTransparent@@*
+# TileEngine/Interactive Tiles.cpp
+fun:*CheckVideoObjectScreenCoordinateInData@@*
+# Ja2/profiler.cpp
+fun:*getCPUCount@PerfManager@@*