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

@@ -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).

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})
 

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) {

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<ExpressionParserError*> fatalErrors;
   std::vector<ExpressionParserError*> allErrors;

Core/GDCore/Serialization/rapidjson/internal/stack.h

@@ -17,6 +17,7 @@
 
 #include "../allocators.h"
 #include "swap.h"
+#include <cstddef>
 
 #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<typename T>
     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<std::ptrdiff_t>(sizeof(T) * count) > (stackEnd_ - stackTop_)))
             Expand<T>(count);
     }
 
@@ -126,7 +130,7 @@ public:
 
     template<typename T>
     RAPIDJSON_FORCEINLINE T* PushUnsafe(size_t count = 1) {
-        RAPIDJSON_ASSERT(stackTop_ + sizeof(T) * count <= stackEnd_);
+        RAPIDJSON_ASSERT(static_cast<std::ptrdiff_t>(sizeof(T) * count) <= (stackEnd_ - stackTop_));
         T* ret = reinterpret_cast<T*>(stackTop_);
         stackTop_ += sizeof(T) * count;
         return ret;

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);
 

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;