From da05bff96c2209c9acd3fa3255ed001198d00cc3 Mon Sep 17 00:00:00 2001
From: Bastien Montagne <bastien@blender.org>
Date: Thu, 27 Jun 2024 09:44:22 +0200
Subject: [PATCH] Fix (unreported) Assets: MEM_new/MEM_freeN mismatch usages.

This one was a bit more involved than the previous ones, since the
mismatch was intentional here, and happened on a non-trivial type.

It was done because the new object (managed by the `unique_ptr`) steals
the internal data of the original object. Calling `MEM_delete` (and
therefore the destructor of the `AssetMetaData` object) would then lead
to access-after-free and double-freeing errors.

This is addressed by adding two new 'copy' and 'move' constructors to this type.

The copy one ensures that deep-copy of internal data happens as expected, and
allows to simplify greatly the code in `BKE_asset_metadata_copy`, which
becomes a mere wrapper around it.

The move one allows `make_unique` to properly steal (and clear) the internal
data of the source object, which can then safely be deleted.

Pull Request: https://projects.blender.org/blender/blender/pulls/123693
---
 source/blender/blenkernel/intern/asset.cc     | 45 +++++++++++++------
 source/blender/editors/space_file/filelist.cc |  5 ++-
 source/blender/makesdna/DNA_asset_types.h     |  3 ++
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/source/blender/blenkernel/intern/asset.cc b/source/blender/blenkernel/intern/asset.cc
index 56b3df6eeac..bb99d5b8b42 100644
--- a/source/blender/blenkernel/intern/asset.cc
+++ b/source/blender/blenkernel/intern/asset.cc
@@ -7,6 +7,7 @@
  */
 
 #include <cstring>
+#include <utility>
 
 #include "DNA_ID.h"
 #include "DNA_defaults.h"
@@ -42,26 +43,42 @@ void BKE_asset_metadata_free(AssetMetaData **asset_data)
 
 AssetMetaData *BKE_asset_metadata_copy(const AssetMetaData *source)
 {
-  AssetMetaData *copy = BKE_asset_metadata_create();
+  return MEM_new<AssetMetaData>(__func__, *source);
+}
 
-  copy->local_type_info = source->local_type_info;
-
-  if (source->properties) {
-    copy->properties = IDP_CopyProperty(source->properties);
+AssetMetaData::AssetMetaData(const AssetMetaData &other)
+    : local_type_info(other.local_type_info),
+      catalog_id(other.catalog_id),
+      active_tag(other.active_tag),
+      tot_tags(other.tot_tags)
+{
+  if (other.properties) {
+    properties = IDP_CopyProperty(other.properties);
   }
 
-  BKE_asset_metadata_catalog_id_set(copy, source->catalog_id, source->catalog_simple_name);
+  STRNCPY(catalog_simple_name, other.catalog_simple_name);
 
-  copy->author = BLI_strdup_null(source->author);
-  copy->description = BLI_strdup_null(source->description);
-  copy->copyright = BLI_strdup_null(source->copyright);
-  copy->license = BLI_strdup_null(source->license);
+  author = BLI_strdup_null(other.author);
+  description = BLI_strdup_null(other.description);
+  copyright = BLI_strdup_null(other.copyright);
+  license = BLI_strdup_null(other.license);
 
-  BLI_duplicatelist(&copy->tags, &source->tags);
-  copy->active_tag = source->active_tag;
-  copy->tot_tags = source->tot_tags;
+  BLI_duplicatelist(&tags, &other.tags);
+}
 
-  return copy;
+AssetMetaData::AssetMetaData(AssetMetaData &&other)
+    : local_type_info(other.local_type_info),
+      properties(std::exchange(other.properties, nullptr)),
+      catalog_id(other.catalog_id),
+      author(std::exchange(other.author, nullptr)),
+      description(std::exchange(other.description, nullptr)),
+      copyright(std::exchange(other.copyright, nullptr)),
+      active_tag(other.active_tag),
+      tot_tags(other.tot_tags)
+{
+  STRNCPY(catalog_simple_name, other.catalog_simple_name);
+  tags = other.tags;
+  BLI_listbase_clear(&other.tags);
 }
 
 AssetMetaData::~AssetMetaData()
diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc
index a262bc64588..e0c08259a0d 100644
--- a/source/blender/editors/space_file/filelist.cc
+++ b/source/blender/editors/space_file/filelist.cc
@@ -3210,8 +3210,9 @@ static void filelist_readjob_list_lib_add_datablock(FileListReadJob *job_params,
       if (job_params->load_asset_library) {
         /* Take ownership over the asset data (shallow copies into unique_ptr managed memory) to
          * pass it on to the asset system. */
-        std::unique_ptr metadata = std::make_unique<AssetMetaData>(*datablock_info->asset_data);
-        MEM_freeN(datablock_info->asset_data);
+        std::unique_ptr metadata = std::make_unique<AssetMetaData>(
+            std::move(*datablock_info->asset_data));
+        MEM_delete(datablock_info->asset_data);
         /* Give back a non-owning pointer, because the data-block info is still needed (e.g. to
          * update the asset index). */
         datablock_info->asset_data = metadata.get();
diff --git a/source/blender/makesdna/DNA_asset_types.h b/source/blender/makesdna/DNA_asset_types.h
index b92378393e3..51f5c9b906f 100644
--- a/source/blender/makesdna/DNA_asset_types.h
+++ b/source/blender/makesdna/DNA_asset_types.h
@@ -84,6 +84,9 @@ typedef struct AssetMetaData {
   char _pad[4];
 
 #ifdef __cplusplus
+  AssetMetaData() = default;
+  AssetMetaData(const AssetMetaData &other);
+  AssetMetaData(AssetMetaData &&other);
   /** Enables use with `std::unique_ptr<AssetMetaData>`. */
   ~AssetMetaData();
 #endif