From e86934ebd7fe2a2c76e089ceaa078c860fad5c0a Mon Sep 17 00:00:00 2001 From: apple1417 Date: Fri, 25 Apr 2025 13:58:04 +1200 Subject: [PATCH 1/5] don't bother allocating memory for empty strings I still don't fully understand how the nulls are supposed to work in these things a lot of base unreal stuff is happy with empty array == empty string however - so can avoid the allocation this is also helping get a byte-perfect copy between two structs (which had empty strings in them) for some other stuff I was looking into --- src/unrealsdk/unreal/structs/fstring.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/unrealsdk/unreal/structs/fstring.cpp b/src/unrealsdk/unreal/structs/fstring.cpp index 25321fc..7506984 100644 --- a/src/unrealsdk/unreal/structs/fstring.cpp +++ b/src/unrealsdk/unreal/structs/fstring.cpp @@ -43,9 +43,11 @@ UnmanagedFString::UnmanagedFString(std::string_view str) : UnmanagedFString(unrealsdk::utils::widen(str)) {} UnmanagedFString::UnmanagedFString(std::wstring_view str) : TArray{.data = nullptr, .count = 0, .max = 0} { - auto size = valid_size(str); - this->resize(size); - memcpy(this->data, str.data(), size * sizeof(*this->data)); + if (!str.empty()) { + auto size = valid_size(str); + this->resize(size); + memcpy(this->data, str.data(), size * sizeof(*this->data)); + } } UnmanagedFString::UnmanagedFString(UnmanagedFString&& other) noexcept : TArray{.data = std::exchange(other.data, nullptr), From 98c261eb1f845c768e757d67f5dc7b40d97f0b1c Mon Sep 17 00:00:00 2001 From: apple1417 Date: Fri, 25 Apr 2025 14:01:11 +1200 Subject: [PATCH 2/5] when resizing an array, zero any new bytes this makes it consistent with malloc unfortunately can't put this logic in realloc, since we don't know the original size, but this is the only use of it this is also helping get a byte-perfect copy between two structs (which had unused padding bytes) for some other stuff I was looking into --- src/unrealsdk/unreal/structs/tarray_funcs.h | 30 ++++++++++++++------- src/unrealsdk/unrealsdk.h | 2 ++ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/unrealsdk/unreal/structs/tarray_funcs.h b/src/unrealsdk/unreal/structs/tarray_funcs.h index b56ece9..41265c4 100644 --- a/src/unrealsdk/unreal/structs/tarray_funcs.h +++ b/src/unrealsdk/unreal/structs/tarray_funcs.h @@ -1,5 +1,5 @@ -#ifndef PYUNREALSDK_LIBS_UNREALSDK_SRC_UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H -#define PYUNREALSDK_LIBS_UNREALSDK_SRC_UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H +#ifndef UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H +#define UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H #include "unrealsdk/pch.h" @@ -23,31 +23,41 @@ void TArray::reserve(size_t new_cap, size_t element_size) { if (new_cap > MAX_CAPACITY) { throw std::length_error("Tried to increase TArray beyond max capacity!"); } - if (new_cap == this->capacity()) { + + auto old_cap = this->capacity(); + if (new_cap == old_cap) { return; } - size_t new_size = new_cap * element_size; + size_t new_cap_bytes = new_cap * element_size; /* If realloc fails, it'll return null, which we want to handle "gracefully" by throwing on our - thread and letting the game continue on as if nothing happened. + thread and letting the game continue on as if nothing happened. + We have a race condition between the realloc call finishing and us overwriting the data pointer - however, another thread could try access the now free'd memory. + however, another thread could try access the now free'd memory. + Since the realloc failure should be a lot rarer, overwrite the pointer ASAP, and only check if - it's valid after. + it's valid after. */ auto old_data = this->data; - this->data = (this->data == nullptr) ? unrealsdk::u_malloc(new_size) - : unrealsdk::u_realloc(this->data, new_size); + this->data = (this->data == nullptr) ? unrealsdk::u_malloc(new_cap_bytes) + : unrealsdk::u_realloc(this->data, new_cap_bytes); if (this->data == nullptr) { this->data = old_data; throw std::runtime_error("Failed to allocate memory to resize array!"); } + // If the array grew (and wasn't empty before), zero out any new bytes + if (0 < old_cap && old_cap < new_cap) { + auto u8_data = reinterpret_cast(this->data); + memset(u8_data + (old_cap * element_size), 0, (new_cap - old_cap) * element_size); + } + this->max = (decltype(max))new_cap; } } // namespace unrealsdk::unreal -#endif /* PYUNREALSDK_LIBS_UNREALSDK_SRC_UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H */ +#endif /* UNREALSDK_UNREAL_STRUCTS_TARRAY_FUNCS_H */ diff --git a/src/unrealsdk/unrealsdk.h b/src/unrealsdk/unrealsdk.h index 966184c..1c3612c 100644 --- a/src/unrealsdk/unrealsdk.h +++ b/src/unrealsdk/unrealsdk.h @@ -76,6 +76,7 @@ bool init(const std::function(void)>& game_g /** * @brief Calls unreal's malloc function. + * @note This memory is guaranteed to be zero'd. * * @tparam T The type to cast the return to. * @param len The amount of bytes to allocate. @@ -89,6 +90,7 @@ template /** * @brief Calls unreal's realloc function. + * @note This *does not* zero bytes after the original allocation. * * @tparam T The type to cast the return to. * @param original The original memory to re-allocate. From 275bbc8bf3b7560dce8fb6577be2986b50d3a524 Mon Sep 17 00:00:00 2001 From: apple1417 Date: Fri, 25 Apr 2025 14:03:07 +1200 Subject: [PATCH 3/5] fix corrupting memory when assigning entire array passing the wrong element size meant any array of 12/16 byte elements or larger, if it had to resize, didn't request enough memory --- src/unrealsdk/unreal/classes/properties/uarrayproperty.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unrealsdk/unreal/classes/properties/uarrayproperty.cpp b/src/unrealsdk/unreal/classes/properties/uarrayproperty.cpp index 7587c29..0ca373d 100644 --- a/src/unrealsdk/unreal/classes/properties/uarrayproperty.cpp +++ b/src/unrealsdk/unreal/classes/properties/uarrayproperty.cpp @@ -49,9 +49,9 @@ void PropTraits::set(const UArrayProperty* prop, return; } - cast(inner, [prop, &arr, &value](const T* inner) { + cast(inner, [&arr, &value](const T* inner) { auto new_size = value.size(); - arr->resize(new_size, prop->ElementSize); + arr->resize(new_size, inner->ElementSize); for (size_t i = 0; i < new_size; i++) { set_property(inner, 0, From ba9a2d113cf49eac3746fa58686d0c1553969aed Mon Sep 17 00:00:00 2001 From: apple1417 Date: Fri, 25 Apr 2025 14:10:42 +1200 Subject: [PATCH 4/5] update changelog --- changelog.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/changelog.md b/changelog.md index 72ac444..33fe361 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,13 @@ # Changelog +## Upcoming + +- Fixed that assigning an entire array, rather than getting the array and setting it's elements, + would likely cause memory corruption. This was most common when using an array of large structs, + and when assigning to one which was previously empty. + + [275bbc8b](https://github.com/bl-sdk/unrealsdk/commit/275bbc8b) + ## 1.8.0 - Added support for sending property changed events, via `UObject::post_edit_change_property` and From a6b01cd47b327c7759b60e30e50e4fb5873b758d Mon Sep 17 00:00:00 2001 From: apple1417 Date: Fri, 25 Apr 2025 14:20:44 +1200 Subject: [PATCH 5/5] clean up ci a bit some of these changes downstream never made it back up here --- .github/workflows/build.yml | 23 +++++++++++------------ src/unrealsdk/game/bl3/object.cpp | 2 +- src/unrealsdk/unreal/classes/uproperty.h | 2 +- src/unrealsdk/unreal/classes/ustruct.h | 4 ++-- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 76b238f..8e8557b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -6,10 +6,6 @@ on: [ workflow_dispatch ] -env: - LLVM_MINGW_VERSION: llvm-mingw-20240619-msvcrt-ubuntu-20.04-x86_64 - LLVM_MINGW_DOWNLOAD: https://github.com/mstorsjo/llvm-mingw/releases/download/20240619/llvm-mingw-20240619-msvcrt-ubuntu-20.04-x86_64.tar.xz - jobs: build-windows: runs-on: windows-latest @@ -44,16 +40,13 @@ jobs: submodules: recursive - name: Configure CMake - working-directory: ${{ env.GITHUB_WORKSPACE }} run: cmake . --preset ${{ matrix.preset }} - name: Build - working-directory: ${{ env.GITHUB_WORKSPACE }} run: cmake --build out/build/${{ matrix.preset }} build-ubuntu: - # Require at least 24 for the mingw build - runs-on: ubuntu-24.04 + runs-on: ubuntu-latest strategy: fail-fast: false @@ -78,6 +71,13 @@ jobs: with: submodules: recursive + - name: Login to GitHub Container Registry + uses: docker/login-action@v2 + with: + registry: ghcr.io + username: ${{ github.repository_owner }} + password: ${{ secrets.GITHUB_TOKEN }} + - name: Build uses: devcontainers/ci@v0.3 with: @@ -87,6 +87,9 @@ jobs: # The git watcher cmake thinks something's unsafe? Doesn't happen to me locally. runCmd: | git config --global --add safe.directory `pwd` + + set -e + cmake . --preset ${{ matrix.toolchain.preset }} -G Ninja cmake --build out/build/${{ matrix.toolchain.preset }} @@ -106,7 +109,6 @@ jobs: steps: - name: Setup Clang - if: startswith(matrix.preset, 'clang') uses: egor-tensin/setup-clang@v1 - name: Setup CMake and Ninja @@ -122,7 +124,6 @@ jobs: submodules: recursive - name: Configure CMake - working-directory: ${{ env.GITHUB_WORKSPACE }} run: cmake . --preset ${{ matrix.preset }} -DCMAKE_DISABLE_PRECOMPILE_HEADERS=On - name: Remove `.modmap`s from compile commands @@ -133,7 +134,6 @@ jobs: -Path "out\build\${{ matrix.preset }}\compile_commands.json" - name: Run clang-tidy - working-directory: ${{ env.GITHUB_WORKSPACE }} run: | python (Get-Command run-clang-tidy).Source ` -p "out\build\${{ matrix.preset }}" ` @@ -150,7 +150,6 @@ jobs: steps: - name: Setup Clang - if: startswith(matrix.preset, 'clang') uses: egor-tensin/setup-clang@v1 - name: Checkout repository diff --git a/src/unrealsdk/game/bl3/object.cpp b/src/unrealsdk/game/bl3/object.cpp index 30348f9..3f26aaf 100644 --- a/src/unrealsdk/game/bl3/object.cpp +++ b/src/unrealsdk/game/bl3/object.cpp @@ -17,7 +17,7 @@ namespace unrealsdk::game { namespace { -using construct_obj_func = UObject* (*)(UClass* cls, +using construct_obj_func = UObject* (*)(UClass * cls, UObject* obj, FName name, uint32_t flags, diff --git a/src/unrealsdk/unreal/classes/uproperty.h b/src/unrealsdk/unreal/classes/uproperty.h index e955fee..645d443 100644 --- a/src/unrealsdk/unreal/classes/uproperty.h +++ b/src/unrealsdk/unreal/classes/uproperty.h @@ -119,7 +119,7 @@ class UProperty : public UField { template >> - FieldType read_field(FieldType PropertyType::*field) const { + FieldType read_field(FieldType PropertyType::* field) const { #ifdef UE4 return reinterpret_cast(this)->*field; #else diff --git a/src/unrealsdk/unreal/classes/ustruct.h b/src/unrealsdk/unreal/classes/ustruct.h index 30353eb..b37580d 100644 --- a/src/unrealsdk/unreal/classes/ustruct.h +++ b/src/unrealsdk/unreal/classes/ustruct.h @@ -105,7 +105,7 @@ class UStruct : public UField { template >> - [[nodiscard]] const FieldType& get_field(FieldType SubType::*field) const { + [[nodiscard]] const FieldType& get_field(FieldType SubType::* field) const { #ifdef UE4 return reinterpret_cast(this)->*field; #else @@ -117,7 +117,7 @@ class UStruct : public UField { template >> - FieldType& get_field(FieldType SubType::*field) { + FieldType& get_field(FieldType SubType::* field) { return const_cast(const_cast(this)->get_field(field)); }