tmaq
/
tornavis
1
0
Fork 0
Code Issues 2 Pull Requests 29 Packages Projects 24 Releases Wiki Activity

Fix (unreported) crash in readfile code when deleting an invalid shapekey. 

The root of the issue was that while reading a new blendfile, the
current `G_MAIN` is still the old one, not the one in which new data is
being read.

Since the remapping callback taking care of UI data during ID remapping
is using `G_MAIN`, it is processing the wrong data, so when deleting the
invalid shapekey (from `BLO_main_validate_shapekeys`), the UI data of
the new bmain would not be properly remapped, causing invalid memory
access later when recomputing user counts (calls to
`BKE_main_id_refcount_recompute`).

This is fixed by adding a new `BKE_id_delete_ex` function that takes
extra remapping options parameter, and calling it from
`BLO_main_validate_shapekeys` with extra option to enforce handling of
UI data by remapping code.

NOTE: At some point we have to check if that whole UI-callback thing is
still needed, would be good to get rid of it and systematically process
UI-related ID pointers like any others. Current situation is... fragile
to say the least.
This commit is contained in:
Bastien Montagne 2023-05-10 16:14:00 +02:00
parent ab106607bd
commit ade83b21d1
3 changed files with 29 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
source/blender/blenkernel/BKE_lib_id.h
View File

@ -308,6 +308,15 @@ void BKE_id_free_us(struct Main *bmain, void *idv) ATTR_NONNULL();
* Properly delete a single ID from given \a bmain database.
*/
void BKE_id_delete(struct Main *bmain, void *idv) ATTR_NONNULL();
/**
* Like BKE_id_delete, but with extra corner-case options.
*
* \param extra_remapping_flags Additional `ID_REMAP_` flags to pass to remapping code when
* ensuring that deleted IDs are not used by any other ID in given `bmain`. Typical example would
* be e.g. `ID_REMAP_FORCE_UI_POINTERS`, required when default UI-handling callbacks of remapping
* code won't be working (e.g. from readfile code). */
void BKE_id_delete_ex(struct Main *bmain, void *idv, const int extra_remapping_flags)
ATTR_NONNULL(1, 2);
/**
* Properly delete all IDs tagged with \a LIB_TAG_DOIT, in given \a bmain database.
*

29
source/blender/blenkernel/intern/lib_id_delete.c
View File

@ -199,7 +199,9 @@ void BKE_id_free_us(Main *bmain, void *idv) /* test users */
}
}
static size_t id_delete(Main *bmain, const bool do_tagged_deletion)
static size_t id_delete(Main *bmain,
const bool do_tagged_deletion,
const int extra_remapping_flags)
{
const int tag = LIB_TAG_DOIT;
ListBase *lbarray[INDEX_ID_MAX];
@ -211,6 +213,8 @@ static size_t id_delete(Main *bmain, const bool do_tagged_deletion)
const int free_flag = LIB_ID_FREE_NO_UI_USER |
(do_tagged_deletion ? LIB_ID_FREE_NO_MAIN | LIB_ID_FREE_NO_USER_REFCOUNT :
0);
const int remapping_flags = (ID_REMAP_FLAG_NEVER_NULL_USAGE | ID_REMAP_FORCE_NEVER_NULL_USAGE |
ID_REMAP_FORCE_INTERNAL_RUNTIME_POINTERS | extra_remapping_flags);
ListBase tagged_deleted_ids = {NULL};
base_count = set_listbasepointers(bmain, lbarray);
@ -261,11 +265,7 @@ static size_t id_delete(Main *bmain, const bool do_tagged_deletion)
* links, this can lead to nasty crashing here in second, actual deleting loop.
* Also, this will also flag users of deleted data that cannot be unlinked
* (object using deleted obdata, etc.), so that they also get deleted. */
BKE_libblock_remap_multiple_locked(bmain,
id_remapper,
ID_REMAP_FLAG_NEVER_NULL_USAGE |
ID_REMAP_FORCE_NEVER_NULL_USAGE |
ID_REMAP_FORCE_INTERNAL_RUNTIME_POINTERS);
BKE_libblock_remap_multiple_locked(bmain, id_remapper, remapping_flags);
BKE_id_remapper_clear(id_remapper);
}
@ -329,11 +329,7 @@ static size_t id_delete(Main *bmain, const bool do_tagged_deletion)
* links, this can lead to nasty crashing here in second, actual deleting loop.
* Also, this will also flag users of deleted data that cannot be unlinked
* (object using deleted obdata, etc.), so that they also get deleted. */
BKE_libblock_remap_multiple_locked(bmain,
remapper,
(ID_REMAP_FLAG_NEVER_NULL_USAGE |
ID_REMAP_FORCE_NEVER_NULL_USAGE |
ID_REMAP_FORCE_INTERNAL_RUNTIME_POINTERS));
BKE_libblock_remap_multiple_locked(bmain, remapper, remapping_flags);
}
BKE_id_remapper_free(remapper);
}
@ -371,7 +367,7 @@ static size_t id_delete(Main *bmain, const bool do_tagged_deletion)
return num_datablocks_deleted;
}
void BKE_id_delete(Main *bmain, void *idv)
void BKE_id_delete_ex(Main *bmain, void *idv, const int extra_remapping_flags)
{
BLI_assert_msg((((ID *)idv)->tag & LIB_TAG_NO_MAIN) == 0,
"Cannot be used with IDs outside of Main");
@ -379,12 +375,17 @@ void BKE_id_delete(Main *bmain, void *idv)
BKE_main_id_tag_all(bmain, LIB_TAG_DOIT, false);
((ID *)idv)->tag |= LIB_TAG_DOIT;
id_delete(bmain, false);
id_delete(bmain, false, extra_remapping_flags);
}
void BKE_id_delete(Main *bmain, void *idv)
{
BKE_id_delete_ex(bmain, idv, 0);
}
size_t BKE_id_multi_tagged_delete(Main *bmain)
{
return id_delete(bmain, true);
return id_delete(bmain, true, 0);
}
/* -------------------------------------------------------------------- */

6
source/blender/blenloader/intern/blend_validate.cc
View File

@ -24,6 +24,7 @@
#include "BKE_key.h"
#include "BKE_lib_id.h"
#include "BKE_lib_remap.h"
#include "BKE_library.h"
#include "BKE_main.h"
#include "BKE_report.h"
@ -196,7 +197,10 @@ bool BLO_main_validate_shapekeys(Main *bmain, ReportList *reports)
"Shapekey %s has an invalid 'from' pointer (%p), it will be deleted",
shapekey->id.name,
shapekey->from);
BKE_id_delete(bmain, shapekey);
/* NOTE: also need to remap UI data ID pointers here, since `bmain` is not the current
* `G_MAIN`, default UI-handling remapping callback (defined by call to
* `BKE_library_callback_remap_editor_id_reference_set`) won't work on exoected data here. */
BKE_id_delete_ex(bmain, shapekey, ID_REMAP_FORCE_UI_POINTERS);
}
return is_valid;