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
This commit is contained in:
Bastien Montagne 2024-06-27 09:44:22 +02:00 committed by Bastien Montagne
parent f43cf39689
commit da05bff96c
3 changed files with 37 additions and 16 deletions

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

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

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