From f7a033e54722cd712bef3f8c06e9eb9bca4100f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 5 Aug 2025 11:03:31 +0200 Subject: [PATCH] Fix: AssetShelfSettings memory management issues (assignment & destructor) Rewrite `AssetShelfSettings::operator=` and fix `~AssetShelfSettings()` to address some memory management issues: - `memcpy` was used to copy a C++ struct, which shouldn't be done: it can cause undefined behaviour if the class has vtables or internal padding. This isn't the case here, but GCC nonetheless warns for it. - `active_catalog_path` was incorrectly freed via `MEM_delete()`; it was allocated with `BLI_strdup()`, which uses `MEM_malloc_arrayN()` and thus should be freed with `MEM_freeN()`. - Self-assignments weren't handled properly, and could cause data loss. These are now all fixed. Pull Request: https://projects.blender.org/blender/blender/pulls/143701 --- .../asset/intern/asset_shelf_settings.cc | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/source/blender/editors/asset/intern/asset_shelf_settings.cc b/source/blender/editors/asset/intern/asset_shelf_settings.cc index c49145f24ab..e4c84edea1c 100644 --- a/source/blender/editors/asset/intern/asset_shelf_settings.cc +++ b/source/blender/editors/asset/intern/asset_shelf_settings.cc @@ -17,6 +17,7 @@ #include "BLI_listbase.h" #include "BLI_string.h" +#include "BLI_string_utf8.h" #include "BKE_asset.hh" #include "BKE_preferences.h" @@ -36,13 +37,24 @@ AssetShelfSettings::AssetShelfSettings(const AssetShelfSettings &other) AssetShelfSettings &AssetShelfSettings::operator=(const AssetShelfSettings &other) { - /* Start with a shallow copy. */ - memcpy(this, &other, sizeof(AssetShelfSettings)); - - if (active_catalog_path) { - active_catalog_path = BLI_strdup(other.active_catalog_path); + if (this == &other) { + return *this; /* Handle self-assignment safely. */ } - enabled_catalog_paths = BKE_asset_catalog_path_list_duplicate(other.enabled_catalog_paths); + + /* Free existing properties. */ + BKE_asset_catalog_path_list_free(this->enabled_catalog_paths); + MEM_SAFE_FREE(this->active_catalog_path); + + /* Copy from 'other'. */ + this->asset_library_reference = other.asset_library_reference; + STRNCPY_UTF8(this->search_string, other.search_string); + this->preview_size = other.preview_size; + this->display_flag = other.display_flag; + + if (other.active_catalog_path) { + this->active_catalog_path = BLI_strdup(other.active_catalog_path); + } + this->enabled_catalog_paths = BKE_asset_catalog_path_list_duplicate(other.enabled_catalog_paths); return *this; } @@ -50,7 +62,7 @@ AssetShelfSettings &AssetShelfSettings::operator=(const AssetShelfSettings &othe AssetShelfSettings::~AssetShelfSettings() { BKE_asset_catalog_path_list_free(enabled_catalog_paths); - MEM_delete(active_catalog_path); + MEM_SAFE_FREE(active_catalog_path); } namespace blender::ed::asset::shelf {