diff --git a/CMakeLists.txt b/CMakeLists.txt index f51c636eb..ef8866e3a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,8 @@ option(WITH_ORACLE "Build Cockatrice card database tool (Oracle)" ON) option(WITH_SERVER "Build Cockatrice server (Servatrice)" OFF) # Compile tests option(TEST "Build tests" OFF) +# Build with AddressSanitizer and UndefinedBehaviorSanitizer +option(ENABLE_SANITIZERS "Build with AddressSanitizer and UndefinedBehaviorSanitizer" OFF) # Use vcpkg regardless of OS option(USE_VCPKG "Use vcpkg regardless of OS" OFF) @@ -211,6 +213,10 @@ else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pipe") endif() +# Always included, because it defines cockatrice_disable_sanitizers_for() for the vendored +# trees. It only touches the global flags when ENABLE_SANITIZERS is on. +include(Sanitizers) + # GNU systems need to define the Mersenne exponent for the RNG to compile w/o warning if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") add_definitions("-DSFMT_MEXP=19937") diff --git a/cmake/Sanitizers.cmake b/cmake/Sanitizers.cmake new file mode 100644 index 000000000..b069e5773 --- /dev/null +++ b/cmake/Sanitizers.cmake @@ -0,0 +1,94 @@ +# AddressSanitizer + UndefinedBehaviorSanitizer opt-in, plus per-source opt-out +# Input: ENABLE_SANITIZERS +# Defines: cockatrice_disable_sanitizers_for +# +# Included once from the root CMakeLists.txt, immediately after the per-compiler flag +# block, so add_compile_options() reaches every library and executable defined below. + +include(CheckCXXSourceCompiles) + +# Opt a set of vendored sources out of instrumentation. +# +# cockatrice_disable_sanitizers_for( ...) +# +# Sanitizer findings inside vendored code are not actionable for us, and letting them +# through buries our own bugs in noise until everyone learns to skip the job. +# +# The sources must be named relative to the directory that compiles them. CMake resolves a +# source-file property against the directory scope the target was declared in, so calling +# this from the root with absolute paths silently does nothing. That is why the call sites +# live next to the add_library/add_executable that pull the vendored code in. +# +# SKIP_PRECOMPILE_HEADERS is not optional. The precompiled header is built with the +# sanitizer flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with +# -fno-sanitize=all does not, and GCC then refuses the PCH with +# "not used because __SANITIZE_ADDRESS__ not defined", which is a fatal -Werror=invalid-pch +# in the servatrice and oracle builds. +function(cockatrice_disable_sanitizers_for) + if(NOT ENABLE_SANITIZERS) + return() + endif() + set_source_files_properties(${ARGN} PROPERTIES COMPILE_OPTIONS "-fno-sanitize=all" SKIP_PRECOMPILE_HEADERS ON) +endfunction() + +if(ENABLE_SANITIZERS) + # Probe a flag the way it will actually be used: on the compile step AND the link step. + # check_cxx_compiler_flag() only probes the compile step, which makes every sanitizer + # flag look unsupported because linking an ASan object without -fsanitize=address leaves + # __asan_init undefined. That is a silent misconfiguration rather than an error, so the + # probe has to link. + function(cockatrice_probe_flag FLAG VARNAME) + set(CMAKE_REQUIRED_FLAGS "${FLAG}") + set(CMAKE_REQUIRED_LINK_OPTIONS "${FLAG}") + check_cxx_source_compiles("int main() { return 0; }" ${VARNAME}) + unset(CMAKE_REQUIRED_FLAGS) + unset(CMAKE_REQUIRED_LINK_OPTIONS) + endfunction() + + set(SANITIZER_FLAGS + -fsanitize=address,undefined + -fno-omit-frame-pointer + # Qt builds QObject subclasses into raw storage and writes the vptr afterwards, which + # the vptr check reports on essentially every QObject instantiation. The check has no + # value for us because nothing outside moc-generated code inherits QObject, so it is + # off outright rather than suppressed piecemeal. + -fno-sanitize=vptr + ) + + # MSVC has no ASan/UBSan equivalent, and clang-cl would need /fsanitize=address plus a + # matching runtime link, which is a separate piece of work. + if(MSVC) + message(FATAL_ERROR "ENABLE_SANITIZERS is not supported with MSVC") + endif() + + # Probe rather than assume, so an older toolchain gets the sanitizers it can support + # instead of failing the build on a flag it does not recognize. + set(SANITIZER_SUPPORTED_FLAGS) + foreach(FLAG ${SANITIZER_FLAGS}) + string(MAKE_C_IDENTIFIER "COCKATRICE_HAS${FLAG}" _FLAG_VAR) + cockatrice_probe_flag("${FLAG}" ${_FLAG_VAR}) + if(${_FLAG_VAR}) + list(APPEND SANITIZER_SUPPORTED_FLAGS ${FLAG}) + else() + message(STATUS "Sanitizers: ${FLAG} unsupported by ${CMAKE_CXX_COMPILER_ID}, skipping") + endif() + endforeach() + + # The whole point of the option is the sanitizers, so an unsupported pair is fatal even + # though the cosmetic flags above are allowed to degrade. + if(NOT "-fsanitize=address,undefined" IN_LIST SANITIZER_SUPPORTED_FLAGS) + message(FATAL_ERROR "ENABLE_SANITIZERS requires -fsanitize=address,undefined, which " + "${CMAKE_CXX_COMPILER_ID} does not accept" + ) + endif() + + add_compile_options(${SANITIZER_SUPPORTED_FLAGS}) + add_link_options(-fsanitize=address,undefined) + + # Recorded so the test tree can label its entries and a plain `make` at the same time + # cannot quietly disagree with what CI ran. + set(COCKATRICE_SANITIZERS + ON + CACHE INTERNAL "AddressSanitizer and UBSan are enabled" + ) +endif() diff --git a/libcockatrice_rng/CMakeLists.txt b/libcockatrice_rng/CMakeLists.txt index b988c9d2f..16beffcfe 100644 --- a/libcockatrice_rng/CMakeLists.txt +++ b/libcockatrice_rng/CMakeLists.txt @@ -11,6 +11,9 @@ add_library( libcockatrice/rng/sfmt/SFMT.c ) +# SFMT is vendored and its own headers trip UBSan's alignment and vptr checks. +cockatrice_disable_sanitizers_for(libcockatrice/rng/sfmt/SFMT.c) + target_include_directories(libcockatrice_rng PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_rng PUBLIC ${QT_CORE_MODULE}) diff --git a/oracle/CMakeLists.txt b/oracle/CMakeLists.txt index 0003b77ac..9b6863553 100644 --- a/oracle/CMakeLists.txt +++ b/oracle/CMakeLists.txt @@ -79,6 +79,8 @@ if(ZLIB_FOUND) include_directories(${ZLIB_INCLUDE_DIRS}) add_definitions("-DHAS_ZLIB") list(APPEND oracle_SOURCES src/zip/unzip.cpp src/zip/zipglobal.cpp) + # minizip and zlib are vendored. + cockatrice_disable_sanitizers_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) else() message(STATUS "Oracle: zlib not found; ZIP support disabled") endif() @@ -89,6 +91,8 @@ if(LIBLZMA_FOUND) include_directories(${LIBLZMA_INCLUDE_DIRS}) add_definitions("-DHAS_LZMA") list(APPEND oracle_SOURCES src/lzma/decompress.cpp) + # The LZMA SDK is vendored. + cockatrice_disable_sanitizers_for(src/lzma/decompress.cpp) else() message(STATUS "Oracle: LibLZMA not found; xz support disabled") endif() diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index 21f71a908..b176d8009 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -27,6 +27,11 @@ set(servatrice_SOURCES set(servatrice_RESOURCES servatrice.qrc) +# Qxt SMTP is vendored. +cockatrice_disable_sanitizers_for( + src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp +) + if(WIN32) set(servatrice_SOURCES ${servatrice_SOURCES} servatrice.rc) endif(WIN32) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6e7ad64f7..c48e6c957 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -64,7 +64,7 @@ include_directories(${GTEST_INCLUDE_DIRS}) # # A second label is a configure-time FATAL_ERROR rather than a line CTest quietly reads as one. # Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it -# composes with the group instead of competing for the single slot. +# composes with the group instead of competing for the single slot. NO_SANITIZE is such a flag. # # TIMEOUT is bounded per test because CTest's implicit default is 1500s: one hung socket wait # would otherwise stall CI for 25 minutes. @@ -73,7 +73,9 @@ include_directories(${GTEST_INCLUDE_DIRS}) # lzma) keeps it last, which is where the linker needs it. function(cockatrice_add_test) - cmake_parse_arguments(PARSE_ARGV 0 ARG "MANUAL" "NAME" "SOURCES;LIBS;LABELS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS") + cmake_parse_arguments( + PARSE_ARGV 0 ARG "MANUAL;NO_SANITIZE" "NAME" "SOURCES;LIBS;LABELS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS" + ) if(NOT ARG_NAME OR NOT ARG_SOURCES) message(FATAL_ERROR "cockatrice_add_test requires at least NAME and SOURCES") @@ -106,6 +108,13 @@ function(cockatrice_add_test) return() endif() + # NO_SANITIZE marks a target the sanitizer leg cannot run. Excluded by not handing its cases + # to CTest at all, rather than by a label, so the exclusion cannot rot into a line CTest + # quietly reads as something else. The target still builds, and still runs on the other legs. + if(ARG_NO_SANITIZE AND COCKATRICE_SANITIZERS) + return() + endif() + # One label only, per the note above. Rejecting a second loudly beats emitting a # CTestTestfile line that CTest will quietly read as one label. list(LENGTH ARG_LABELS _label_count) @@ -116,12 +125,19 @@ function(cockatrice_add_test) set(ARG_LABELS ci) endif() - # GoogleTestAddTests defaults DISCOVERY_TIMEOUT to 5s. A plain debug binary answers - # --gtest_list_tests in 0.04s, but that budget is thin once several binaries discover - # concurrently behind a -j build, and being over it fails at the *link* step with a bare - # "Process terminated due to timeout" that points nowhere near the cause. + # GoogleTestAddTests defaults DISCOVERY_TIMEOUT to 5s, which is ample for a plain debug + # binary (--gtest_list_tests costs 0.04s) but not for an instrumented one (~2.8s cold, and + # discovery runs several binaries concurrently behind a -j build). Left at the default, a + # sanitizer build fails at the *link* step with a bare "Process terminated due to timeout" + # and no hint that discovery, not the test, is what is slow. + if(COCKATRICE_SANITIZERS) + set(_discovery_timeout 300) + else() + set(_discovery_timeout 60) + endif() + gtest_discover_tests( - ${ARG_NAME} DISCOVERY_MODE POST_BUILD DISCOVERY_TIMEOUT 60 + ${ARG_NAME} DISCOVERY_MODE POST_BUILD DISCOVERY_TIMEOUT ${_discovery_timeout} PROPERTIES LABELS "${ARG_LABELS}" TIMEOUT 60 ) @@ -203,6 +219,7 @@ cockatrice_add_test( cockatrice_add_test( NAME server_developer_role_test + NO_SANITIZE SOURCES server_developer_role_test.cpp LIBS diff --git a/tests/oracle/CMakeLists.txt b/tests/oracle/CMakeLists.txt index bbd0f932a..c896c7960 100644 --- a/tests/oracle/CMakeLists.txt +++ b/tests/oracle/CMakeLists.txt @@ -25,6 +25,7 @@ find_package(ZLIB) if(ZLIB_FOUND) add_definitions("-DHAS_ZLIB") set(_ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/zip/unzip.cpp ../../oracle/src/zip/zipglobal.cpp) + cockatrice_disable_sanitizers_for(${_ORACLE_BENCH_EXTRA_SOURCES}) set(_ORACLE_BENCH_EXTRA_LIBRARIES ${ZLIB_LIBRARIES}) include_directories(${ZLIB_INCLUDE_DIRS}) else() @@ -35,6 +36,7 @@ find_package(LibLZMA) if(LIBLZMA_FOUND) add_definitions("-DHAS_LZMA") list(APPEND _ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/lzma/decompress.cpp) + cockatrice_disable_sanitizers_for(../../oracle/src/lzma/decompress.cpp) list(APPEND _ORACLE_BENCH_EXTRA_LIBRARIES ${LIBLZMA_LIBRARIES}) include_directories(${LIBLZMA_INCLUDE_DIRS}) else()