From 12867be2259e73cadaa61241406e11ecc2d3d482 Mon Sep 17 00:00:00 2001 From: Florian Rival Date: Thu, 21 May 2026 12:46:30 +0200 Subject: [PATCH] Fix potential crashes (#8625) And add GDCore tests with ASan/UBSan as a new CI job to catch any regression in the future --- .circleci/config.yml | 103 +++++++++++++++++- CMakeLists.txt | 16 +++ .../GDCore/IDE/Events/ExpressionValidator.cpp | 1 + Core/GDCore/IDE/Events/ExpressionValidator.h | 6 +- .../Serialization/rapidjson/internal/stack.h | 10 +- Core/GDCore/String.cpp | 3 + Core/GDCore/Utf8/utf8proc.c | 9 +- 7 files changed, 137 insertions(+), 11 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index c775563fb1..9ede118ff3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -194,7 +194,7 @@ jobs: # System dependencies (for Emscripten) - run: name: Install dependencies for Emscripten - command: sudo apt-get update && sudo apt install cmake + command: sudo apt-get update && sudo apt install cmake ccache - run: name: Install Emscripten (for GDevelop.js) @@ -211,11 +211,37 @@ jobs: name: Install GDevelop.js dependencies and build it command: cd GDevelop.js && npm install && cd .. - # Build GDevelop.js (and run tests to ensure it works) + # Cache emcc/em++ output via ccache (keyed by content hash, immune to + # git checkout resetting source mtimes). + - restore_cache: + name: Restore ccache (Emscripten) + keys: + - gdevelop.js-emscripten-ccache-v1-{{ .Branch }}-{{ .Revision }} + - gdevelop.js-emscripten-ccache-v1-{{ .Branch }}- + - gdevelop.js-emscripten-ccache-v1- + + # Build GDevelop.js - run: name: Build GDevelop.js - # Use "--runInBand" as it's faster and avoid deadlocks on CircleCI Linux machines (probably because limited in processes number). - command: cd GDevelop.js && source ../emsdk/emsdk_env.sh && npm run build && npm test -- --runInBand && cd .. + # CMAKE_(C|CXX)_COMPILER_LAUNCHER are read by CMake (>=3.17) from the + # environment and prepended to every compiler invocation. + command: | + export CMAKE_C_COMPILER_LAUNCHER=ccache + export CMAKE_CXX_COMPILER_LAUNCHER=ccache + cd GDevelop.js && source ../emsdk/emsdk_env.sh && npm run build && cd .. + ccache --show-stats + + - save_cache: + name: Save ccache (Emscripten) + key: gdevelop.js-emscripten-ccache-v1-{{ .Branch }}-{{ .Revision }} + paths: + - ~/.cache/ccache + + # "--runInBand" is faster and avoids deadlocks on CircleCI Linux machines + # (probably because of limited process numbers). + - run: + name: Run GDevelop.js tests + command: cd GDevelop.js && source ../emsdk/emsdk_env.sh && npm test -- --runInBand && cd .. - save_cache: paths: @@ -292,6 +318,74 @@ jobs: name: Deploy to S3 (specific commit) command: aws s3 sync Binaries/embuild/GDevelop.js s3://gdevelop-gdevelop.js/$(git rev-parse --abbrev-ref HEAD)/variant/debug-sanitizers/commit/$(git rev-parse HEAD)/ + # Build and run the native GDCore_tests with AddressSanitizer + UBSan, to + # catch heap-use-after-free, buffer overflows and undefined behaviour that + # only sometimes manifest as flaky segfaults. + # The Emscripten build has its own sanitizer job: + # build-gdevelop_js-debug-sanitizers-and-extra-checks. + build-gdcore-tests-sanitizers: + resource_class: medium+ + docker: + - image: cimg/base:current + + working_directory: ~/GDevelop + + steps: + - checkout + + - run: + name: Install build dependencies (cmake, clang, ccache, libstdc++ dev) + command: | + sudo apt-get update + sudo apt-get install -y cmake clang ccache libstdc++-12-dev + + # Cache the compiler output via ccache (keyed by content hash, so it is + # immune to the fact that 'git checkout' resets every source file's mtime + # to "now" on each run). + - restore_cache: + name: Restore ccache + keys: + - gdcore-tests-asan-ccache-v1-{{ .Branch }}-{{ .Revision }} + - gdcore-tests-asan-ccache-v1-{{ .Branch }}- + - gdcore-tests-asan-ccache-v1- + + - run: + name: Configure GDCore_tests with ASan + UBSan (Debug) + command: | + mkdir -p .build-tests-asan + cd .build-tests-asan + cmake \ + -DBUILD_GDJS=FALSE \ + -DBUILD_EXTENSIONS=FALSE \ + -DBUILD_TESTS=TRUE \ + -DCMAKE_BUILD_TYPE=Debug \ + -DUSE_SANITIZERS="address,undefined" \ + -DCMAKE_CXX_COMPILER=$(which clang++) \ + -DCMAKE_C_COMPILER=$(which clang) \ + -DCMAKE_C_COMPILER_LAUNCHER=ccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ + .. + + - run: + name: Build GDCore_tests + command: | + cd .build-tests-asan && make -j 4 GDCore_tests + ccache --show-stats + + - save_cache: + name: Save ccache + key: gdcore-tests-asan-ccache-v1-{{ .Branch }}-{{ .Revision }} + paths: + - ~/.cache/ccache + + - run: + name: Run GDCore_tests with sanitizers + command: | + cd .build-tests-asan + export ASAN_OPTIONS=detect_leaks=0:abort_on_error=1:print_stacktrace=1 + export UBSAN_OPTIONS=print_stacktrace=1 + Core/GDCore_tests + # Trigger AppVeyor build, which also does a Windows build (keep it for redundancy). trigger-appveyor-windows-build: docker: @@ -502,6 +596,7 @@ workflows: builds: jobs: - build-gdevelop_js-wasm-only + - build-gdcore-tests-sanitizers - build-macos: # The macOS version builds by itself GDevelop.js # (so we verify we can build it on macOS). diff --git a/CMakeLists.txt b/CMakeLists.txt index fe858817e5..09a13098f4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,6 +19,7 @@ gd_set_option(BUILD_CORE TRUE BOOL "TRUE to build GDevelop Core library") gd_set_option(BUILD_GDJS TRUE BOOL "TRUE to build GDevelop JS Platform") gd_set_option(BUILD_EXTENSIONS TRUE BOOL "TRUE to build the extensions") gd_set_option(BUILD_TESTS TRUE BOOL "TRUE to build the tests") +gd_set_option(USE_SANITIZERS "" STRING "Comma-separated sanitizers to enable for native builds (e.g. \"address,undefined\"). Empty to disable. Ignored when building with Emscripten.") # Disable deprecated code set(NO_GUI TRUE CACHE BOOL "" FORCE) # Force disable old GUI related code. @@ -87,6 +88,21 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") -Werror=return-type) endif() +# Sanitizers (ASan/UBSan) for native builds. Use e.g.: +# cmake -DUSE_SANITIZERS="address,undefined" -DCMAKE_BUILD_TYPE=Debug .. +# Then run with: +# ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 ./GDCore_tests +# Some checks (use-after-scope) need at least -O1; the Debug build type's -O0 +# still catches use-after-free, heap-buffer-overflow and the UBSan checks. +if(USE_SANITIZERS AND NOT EMSCRIPTEN) + message(STATUS "Enabling sanitizers: ${USE_SANITIZERS}") + add_compile_options(-fsanitize=${USE_SANITIZERS} -fno-omit-frame-pointer -fno-sanitize-recover=all) + # add_link_options requires CMake >= 3.13; append to linker flag vars for wider compatibility. + set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=${USE_SANITIZERS}") + set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=${USE_SANITIZERS}") + set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -fsanitize=${USE_SANITIZERS}") +endif() + # Define common directories: set(GD_base_dir ${CMAKE_CURRENT_SOURCE_DIR}) diff --git a/Core/GDCore/IDE/Events/ExpressionValidator.cpp b/Core/GDCore/IDE/Events/ExpressionValidator.cpp index b370fc4e13..cacb35887f 100644 --- a/Core/GDCore/IDE/Events/ExpressionValidator.cpp +++ b/Core/GDCore/IDE/Events/ExpressionValidator.cpp @@ -460,6 +460,7 @@ const gd::String ExpressionValidator::numberOrStringTypeString = const gd::String ExpressionValidator::variableTypeString = "variable"; const gd::String ExpressionValidator::objectTypeString = "object"; const gd::String ExpressionValidator::emptyTypeString = "empty"; +const gd::String ExpressionValidator::emptyParameterExtraInfo; const gd::String& ExpressionValidator::TypeToString(Type type) { switch (type) { diff --git a/Core/GDCore/IDE/Events/ExpressionValidator.h b/Core/GDCore/IDE/Events/ExpressionValidator.h index 366be17a36..c46f4724ef 100644 --- a/Core/GDCore/IDE/Events/ExpressionValidator.h +++ b/Core/GDCore/IDE/Events/ExpressionValidator.h @@ -40,7 +40,7 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker { ExpressionValidator(const gd::Platform &platform_, const gd::ProjectScopedContainers & projectScopedContainers_, const gd::String &rootType_, - const gd::String &extraInfo_ = "") + const gd::String &extraInfo_ = emptyParameterExtraInfo) : platform(platform_), projectScopedContainers(projectScopedContainers_), parentType(StringToType(gd::ValueTypeMetadata::GetExpressionPrimitiveValueType(rootType_))), @@ -555,6 +555,10 @@ private: static const gd::String objectTypeString; static const gd::String identifierTypeString; static const gd::String emptyTypeString; + // Used as the default for the `extraInfo_` constructor argument: a long-lived + // empty string, so that storing &extraInfo_ in currentParameterExtraInfo + // never dangles when no explicit extraInfo is provided. + static const gd::String emptyParameterExtraInfo; std::vector fatalErrors; std::vector allErrors; diff --git a/Core/GDCore/Serialization/rapidjson/internal/stack.h b/Core/GDCore/Serialization/rapidjson/internal/stack.h index 022c9aab41..cfe9a814ec 100644 --- a/Core/GDCore/Serialization/rapidjson/internal/stack.h +++ b/Core/GDCore/Serialization/rapidjson/internal/stack.h @@ -17,6 +17,7 @@ #include "../allocators.h" #include "swap.h" +#include #if defined(__clang__) RAPIDJSON_DIAG_PUSH @@ -113,8 +114,11 @@ public: // Expansion is run very infrequently, so it is moved to another (probably non-inline) function. template RAPIDJSON_FORCEINLINE void Reserve(size_t count = 1) { - // Expand the stack if needed - if (RAPIDJSON_UNLIKELY(stackTop_ + sizeof(T) * count > stackEnd_)) + // Expand the stack if needed. + // Use pointer subtraction rather than `stackTop_ + N > stackEnd_` because the + // stack is lazily allocated: stackTop_ and stackEnd_ are both null until the + // first Expand(), and `null + non-zero` is undefined behaviour (caught by UBSan). + if (RAPIDJSON_UNLIKELY(static_cast(sizeof(T) * count) > (stackEnd_ - stackTop_))) Expand(count); } @@ -126,7 +130,7 @@ public: template RAPIDJSON_FORCEINLINE T* PushUnsafe(size_t count = 1) { - RAPIDJSON_ASSERT(stackTop_ + sizeof(T) * count <= stackEnd_); + RAPIDJSON_ASSERT(static_cast(sizeof(T) * count) <= (stackEnd_ - stackTop_)); T* ret = reinterpret_cast(stackTop_); stackTop_ += sizeof(T) * count; return ret; diff --git a/Core/GDCore/String.cpp b/Core/GDCore/String.cpp index f97d234004..8a6110dfde 100644 --- a/Core/GDCore/String.cpp +++ b/Core/GDCore/String.cpp @@ -232,6 +232,9 @@ void String::pop_back() String& String::insert( size_type pos, const String &str ) { + if(pos > size()) + throw std::out_of_range("[gd::String::insert] starting pos greater than size"); + iterator it = begin(); std::advance(it, pos); diff --git a/Core/GDCore/Utf8/utf8proc.c b/Core/GDCore/Utf8/utf8proc.c index 814cef4ec6..5679a2956a 100644 --- a/Core/GDCore/Utf8/utf8proc.c +++ b/Core/GDCore/Utf8/utf8proc.c @@ -361,7 +361,8 @@ UTF8PROC_DLLEXPORT utf8proc_ssize_t utf8proc_decompose_char(utf8proc_int32_t uc, utf8proc_ssize_t written = 0; for (casefold_entry = property->casefold_mapping; *casefold_entry >= 0; casefold_entry++) { - written += utf8proc_decompose_char(*casefold_entry, dst+written, + written += utf8proc_decompose_char(*casefold_entry, + dst ? dst+written : NULL, (bufsize > written) ? (bufsize - written) : 0, options, last_boundclass); if (written < 0) return UTF8PROC_ERROR_OVERFLOW; @@ -376,7 +377,8 @@ UTF8PROC_DLLEXPORT utf8proc_ssize_t utf8proc_decompose_char(utf8proc_int32_t uc, utf8proc_ssize_t written = 0; for (decomp_entry = property->decomp_mapping; *decomp_entry >= 0; decomp_entry++) { - written += utf8proc_decompose_char(*decomp_entry, dst+written, + written += utf8proc_decompose_char(*decomp_entry, + dst ? dst+written : NULL, (bufsize > written) ? (bufsize - written) : 0, options, last_boundclass); if (written < 0) return UTF8PROC_ERROR_OVERFLOW; @@ -429,7 +431,8 @@ UTF8PROC_DLLEXPORT utf8proc_ssize_t utf8proc_decompose( if (uc < 0) return UTF8PROC_ERROR_INVALIDUTF8; } decomp_result = utf8proc_decompose_char( - uc, buffer + wpos, (bufsize > wpos) ? (bufsize - wpos) : 0, options, + uc, buffer ? buffer + wpos : NULL, + (bufsize > wpos) ? (bufsize - wpos) : 0, options, &boundclass ); if (decomp_result < 0) return decomp_result;