griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix #108923: Serious issues in 'undo_preserve' process of Scene's toolsettings.

Browse Source 
By removing the extra complete check/remapping of ID pointers in undo
case, ebb5643e59 merely revealed how broken the 'undo_preserve' code of
Scene was.

It cumulated a flock of issues, all more or less related to ID pointers:
* 'source of truth' should be the _old_ data (toolsettings), not the new
  one, since this is the one kept at the end of the process;
* In some cases, some paint data may exist in one, but not the other
  of the two 'old' and 'new' toolsettings data.
* Since this data is preserved to its latest version accross undos, its
  ID pointers can become completely unrelated to these read from the
  undo memfile, _even when the Scene itself is detected as unchanged_!
  This implies that:
  + undo_preserve code has to be called even when there is no liblinking
    (when the ID is detected as unchanged and re-used 'as-is').
  + Using existing ID addresses to find/validate an ID pointer in
    undo_preserve process is like playing Russian roulette - invalid
    memory access and crash is guaranteed at some point or another.
    Use `session_uuid` value instead to ensure a valid ID pointer is set
    (or null in case none can be found).

NOTE: while these issues also exist in previous releases (including both
latest LTSs), they were hidden by the code later in `setup_app_data`,
preventing any crash to happen. So backporting this fix would be far too
risky for a very minimal benefit imho.
This commit is contained in:
Bastien Montagne
2023-06-21 15:21:04 +02:00
parent b5db6fe5d2
commit 94e6ab6d71
2 changed files with 204 additions and 187 deletions
Download Patch File Download Diff File Expand all files Collapse all files

377
source/blender/blenkernel/intern/scene.cc
View File

@@ -488,7 +488,6 @@ enum eSceneForeachUndoPreserveProcess {
};
static void scene_foreach_toolsettings_id_pointer_process(
Scene *scene,
ID **id_p,
const eSceneForeachUndoPreserveProcess action,
BlendLibReader *reader,
@@ -500,11 +499,11 @@ static void scene_foreach_toolsettings_id_pointer_process(
ID *id_old = *id_old_p;
/* Old data has not been remapped to new values of the pointers, if we want to keep the old
* pointer here we need its new address. */
ID *id_old_new = id_old != nullptr ? BLO_read_get_new_id_address(
reader, &scene->id, ID_IS_LINKED(scene), id_old) :
ID *id_old_new = id_old != nullptr ? BLO_read_get_new_id_address_from_session_uuid(
reader, id_old->session_uuid) :
nullptr;
if (id_old_new != nullptr) {
BLI_assert(ELEM(id_old, id_old_new, id_old_new->orig_id));
if (!ELEM(id_old_new, id_old, nullptr)) {
BLI_assert(id_old == id_old_new->orig_id);
*id_old_p = id_old_new;
if (cb_flag & IDWALK_CB_USER) {
id_us_plus_no_lib(id_old_new);
@@ -512,8 +511,25 @@ static void scene_foreach_toolsettings_id_pointer_process(
}
break;
}
/* We failed to find a new valid pointer for the previous ID, just keep the current one as
* if we had been under SCENE_FOREACH_UNDO_NO_RESTORE case. */
* if we had been under SCENE_FOREACH_UNDO_NO_RESTORE case.
*
* There is a nasty twist here though: a prvious call to 'undo_preserve' on the Scene ID may
* have modified it, even though the undo step detected it as unmodified. In such case, the
* value of `*id_p` may end up also pointing to an invalid (no more in newly read Main) ID,
* se it also needs to be checked from its `session_uuid`. */
ID *id = *id_p;
ID *id_new = id != nullptr ?
BLO_read_get_new_id_address_from_session_uuid(reader, id->session_uuid) :
nullptr;
if (id_new != id) {
*id_p = id_new;
if (cb_flag & IDWALK_CB_USER) {
id_us_plus_no_lib(id_new);
id_us_min(id);
}
}
std::swap(*id_p, *id_old_p);
break;
}
@@ -527,48 +543,56 @@ static void scene_foreach_toolsettings_id_pointer_process(
/* Special handling is needed here, as `scene_foreach_toolsettings` (and its dependency
* `scene_foreach_paint`) are also used by `scene_undo_preserve`, where `LibraryForeachIDData
* *data` is nullptr. */
#define BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER( \
__data, __scene, __id, __do_undo_restore, __action, __reader, __id_old, __cb_flag) \
#define BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P( \
_data, _id_p, _do_undo_restore, _action, _reader, _id_old_p, _cb_flag) \
{ \
if (__do_undo_restore) { \
if (_do_undo_restore) { \
scene_foreach_toolsettings_id_pointer_process( \
__scene, (ID **)&(__id), __action, __reader, (ID **)&(__id_old), __cb_flag); \
(ID **)(_id_p), _action, _reader, (ID **)(_id_old_p), _cb_flag); \
} \
else { \
BLI_assert((__data) != nullptr); \
BKE_LIB_FOREACHID_PROCESS_IDSUPER(__data, __id, __cb_flag); \
BLI_assert((_data) != nullptr); \
BKE_LIB_FOREACHID_PROCESS_IDSUPER_P(_data, _id_p, _cb_flag); \
} \
} \
(void)0
#define BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL( \
__data, __do_undo_restore, __func_call) \
_data, _do_undo_restore, _func_call) \
{ \
if (__do_undo_restore) { \
__func_call; \
if (_do_undo_restore) { \
_func_call; \
} \
else { \
BLI_assert((__data) != nullptr); \
BKE_LIB_FOREACHID_PROCESS_FUNCTION_CALL(__data, __func_call); \
BLI_assert((_data) != nullptr); \
BKE_LIB_FOREACHID_PROCESS_FUNCTION_CALL(_data, _func_call); \
} \
} \
(void)0
static void scene_foreach_paint(LibraryForeachIDData *data,
Scene *scene,
Paint *paint,
const bool do_undo_restore,
BlendLibReader *reader,
Paint *paint_old)
{
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
paint->brush,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
paint_old->brush,
IDWALK_CB_USER);
/* `paint` may be nullptr in 'undo_preserve' case, when the relevant sub-data does not exist in
* newly read toolsettings, but does exist in old existing ones.
*
* This function should never be called in case the old toolsettings do not have the relevant
* `paint_old` data. */
BLI_assert(paint_old != nullptr);
Brush *brush_tmp = nullptr;
Brush **brush_p = paint ? &paint->brush : &brush_tmp;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
brush_p,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
&paint_old->brush,
IDWALK_CB_USER);
for (int i = 0; i < paint_old->tool_slots_len; i++) {
/* This is a bit tricky.
* - In case we do not do `undo_restore`, `paint` and `paint_old` pointers are the same, so
@@ -578,206 +602,189 @@ static void scene_foreach_paint(LibraryForeachIDData *data,
* + In case the new data has less valid slots, we feed in a dummy null pointer.
* + In case the new data has more valid slots, the extra ones are ignored.
*/
Brush *brush_tmp = nullptr;
Brush **brush_p = i < paint->tool_slots_len ? &paint->tool_slots[i].brush : &brush_tmp;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
*brush_p,
brush_tmp = nullptr;
brush_p = (paint && i < paint->tool_slots_len) ? &paint->tool_slots[i].brush : &brush_tmp;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
brush_p,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
&paint_old->tool_slots[i].brush,
IDWALK_CB_USER);
}
Palette *palette_tmp = nullptr;
Palette **palette_p = paint ? &paint->palette : &palette_tmp;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
palette_p,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
paint_old->brush,
&paint_old->palette,
IDWALK_CB_USER);
}
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
paint->palette,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
paint_old->palette,
IDWALK_CB_USER);
}
static void scene_foreach_toolsettings(LibraryForeachIDData *data,
Scene *scene,
ToolSettings *toolsett,
const bool do_undo_restore,
BlendLibReader *reader,
ToolSettings *toolsett_old)
{
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->particle.scene,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
toolsett_old->particle.scene,
IDWALK_CB_NOP);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->particle.object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
toolsett_old->particle.object,
IDWALK_CB_NOP);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->particle.shape_object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
toolsett_old->particle.shape_object,
IDWALK_CB_NOP);
/* In regular foreach_id case, only one set of data is processed, both pointers are expected to
* be the same.
*
* In undo_preserve case, both pointers may be different (see #lib_link_all for why they may be
* the same in some cases). */
BLI_assert(do_undo_restore || (toolsett == toolsett_old));
BLI_assert(!ELEM(nullptr, toolsett, toolsett_old));
scene_foreach_paint(data,
scene,
&toolsett->imapaint.paint,
do_undo_restore,
reader,
&toolsett_old->imapaint.paint);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->imapaint.stencil,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
toolsett_old->imapaint.stencil,
IDWALK_CB_USER);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->imapaint.clone,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
toolsett_old->imapaint.clone,
IDWALK_CB_USER);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->imapaint.canvas,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
toolsett_old->imapaint.canvas,
IDWALK_CB_USER);
/* NOTE: In 'undo_preserve' case, the 'old' data is the source of truth here, since it is the one
* that will be re-used in newly read Main and therefore needs valid, existing in new Main, ID
* pointers. */
if (toolsett->vpaint) {
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->vpaint->paint,
do_undo_restore,
reader,
&toolsett_old->vpaint->paint));
}
if (toolsett->wpaint) {
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->wpaint->paint,
do_undo_restore,
reader,
&toolsett_old->wpaint->paint));
}
if (toolsett->sculpt) {
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->sculpt->paint,
do_undo_restore,
reader,
&toolsett_old->sculpt->paint));
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->sculpt->gravity_object,
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->particle.scene,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
toolsett_old->sculpt->gravity_object,
&toolsett_old->particle.scene,
IDWALK_CB_NOP);
}
if (toolsett->uvsculpt) {
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->particle.object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
&toolsett_old->particle.object,
IDWALK_CB_NOP);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->particle.shape_object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
&toolsett_old->particle.shape_object,
IDWALK_CB_NOP);
scene_foreach_paint(
data, &toolsett->imapaint.paint, do_undo_restore, reader, &toolsett_old->imapaint.paint);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->imapaint.stencil,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
&toolsett_old->imapaint.stencil,
IDWALK_CB_USER);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->imapaint.clone,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
&toolsett_old->imapaint.clone,
IDWALK_CB_USER);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&toolsett->imapaint.canvas,
do_undo_restore,
SCENE_FOREACH_UNDO_RESTORE,
reader,
&toolsett_old->imapaint.canvas,
IDWALK_CB_USER);
Paint *paint, *paint_old;
if (toolsett_old->vpaint) {
paint = toolsett->vpaint ? &toolsett->vpaint->paint : nullptr;
paint_old = &toolsett_old->vpaint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->uvsculpt->paint,
do_undo_restore,
reader,
&toolsett_old->uvsculpt->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett->gp_paint) {
if (toolsett_old->wpaint) {
paint = toolsett->wpaint ? &toolsett->wpaint->paint : nullptr;
paint_old = &toolsett_old->wpaint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->gp_paint->paint,
do_undo_restore,
reader,
&toolsett_old->gp_paint->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett->gp_vertexpaint) {
if (toolsett_old->sculpt) {
paint = toolsett->sculpt ? &toolsett->sculpt->paint : nullptr;
paint_old = &toolsett_old->sculpt->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->gp_vertexpaint->paint,
do_undo_restore,
reader,
&toolsett_old->gp_vertexpaint->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
Object *gravity_object = toolsett->sculpt ? toolsett->sculpt->gravity_object : nullptr;
Object *gravity_object_old = toolsett_old->sculpt->gravity_object;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(data,
&gravity_object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
&gravity_object_old,
IDWALK_CB_NOP);
if (toolsett->sculpt) {
toolsett->sculpt->gravity_object = gravity_object;
}
toolsett_old->sculpt->gravity_object = gravity_object_old;
}
if (toolsett->gp_sculptpaint) {
if (toolsett_old->uvsculpt) {
paint = toolsett->uvsculpt ? &toolsett->uvsculpt->paint : nullptr;
paint_old = &toolsett_old->uvsculpt->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->gp_sculptpaint->paint,
do_undo_restore,
reader,
&toolsett_old->gp_sculptpaint->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett->gp_weightpaint) {
if (toolsett_old->gp_paint) {
paint = toolsett->gp_paint ? &toolsett->gp_paint->paint : nullptr;
paint_old = &toolsett_old->gp_paint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->gp_weightpaint->paint,
do_undo_restore,
reader,
&toolsett_old->gp_weightpaint->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett->curves_sculpt) {
if (toolsett_old->gp_vertexpaint) {
paint = toolsett->gp_vertexpaint ? &toolsett->gp_vertexpaint->paint : nullptr;
paint_old = &toolsett_old->gp_vertexpaint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data,
scene,
&toolsett->curves_sculpt->paint,
do_undo_restore,
reader,
&toolsett_old->curves_sculpt->paint));
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett_old->gp_sculptpaint) {
paint = toolsett->gp_sculptpaint ? &toolsett->gp_sculptpaint->paint : nullptr;
paint_old = &toolsett_old->gp_sculptpaint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett_old->gp_weightpaint) {
paint = toolsett->gp_weightpaint ? &toolsett->gp_weightpaint->paint : nullptr;
paint_old = &toolsett_old->gp_weightpaint->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
if (toolsett_old->curves_sculpt) {
paint = toolsett->curves_sculpt ? &toolsett->curves_sculpt->paint : nullptr;
paint_old = &toolsett_old->curves_sculpt->paint;
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_FUNCTION_CALL(
data,
do_undo_restore,
scene_foreach_paint(data, paint, do_undo_restore, reader, paint_old));
}
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER(data,
scene,
toolsett->gp_sculpt.guide.reference_object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
toolsett_old->gp_sculpt.guide.reference_object,
IDWALK_CB_NOP);
BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P(
data,
&toolsett->gp_sculpt.guide.reference_object,
do_undo_restore,
SCENE_FOREACH_UNDO_NO_RESTORE,
reader,
&toolsett_old->gp_sculpt.guide.reference_object,
IDWALK_CB_NOP);
}
#undef BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER
@@ -904,7 +911,7 @@ static void scene_foreach_id(ID *id, LibraryForeachIDData *data)
ToolSettings *toolsett = scene->toolsettings;
if (toolsett) {
BKE_LIB_FOREACHID_PROCESS_FUNCTION_CALL(
data, scene_foreach_toolsettings(data, scene, toolsett, false, nullptr, toolsett));
data, scene_foreach_toolsettings(data, toolsett, false, nullptr, toolsett));
}
if (scene->rigidbody_world) {
@@ -1742,7 +1749,7 @@ static void scene_undo_preserve(BlendLibReader *reader, ID *id_new, ID *id_old)
* palettes), and counteract the swap of the whole ToolSettings structs below for the others
* (like object ones). */
scene_foreach_toolsettings(
nullptr, scene_new, scene_new->toolsettings, true, reader, scene_old->toolsettings);
nullptr, scene_new->toolsettings, true, reader, scene_old->toolsettings);
std::swap(*scene_old->toolsettings, *scene_new->toolsettings);
}
}

14
source/blender/blenloader/intern/readfile.cc
View File

@@ -3197,6 +3197,8 @@ static void lib_link_all(FileData *fd, Main *bmain)
ID *id;
FOREACH_MAIN_ID_BEGIN (bmain, id) {
const IDTypeInfo *id_type = BKE_idtype_get_info_from_id(id);
if ((id->tag & (LIB_TAG_UNDO_OLD_ID_REUSED_UNCHANGED | LIB_TAG_UNDO_OLD_ID_REUSED_NOUNDO)) !=
0) {
BLI_assert(fd->flags & FD_FLAGS_IS_MEMFILE);
@@ -3204,11 +3206,19 @@ static void lib_link_all(FileData *fd, Main *bmain)
* current undo step, and old IDs re-use their old memory address, we do not need to liblink
* it at all. */
BLI_assert((id->tag & LIB_TAG_NEED_LINK) == 0);
/* Some data that should be persistent, like the 3DCursor or the tool settings, are
* stored in IDs affected by undo, like Scene. So this requires some specific handling. */
/* NOTE: even though the ID may have been detected as unchanged, the 'undo_preserve' may have
* to actually change some of its ID pointers, it's e.g. the case with Scene's toolsettings
* Brush/Palette pointers. This is the case where both new and old ID may be the same. */
if (id_type->blend_read_undo_preserve != nullptr) {
BLI_assert(fd->flags & FD_FLAGS_IS_MEMFILE);
id_type->blend_read_undo_preserve(&reader, id, id->orig_id ? id->orig_id : id);
}
continue;
}
const IDTypeInfo *id_type = BKE_idtype_get_info_from_id(id);
if ((id->tag & LIB_TAG_NEED_LINK) != 0) {
lib_link_id(&reader, id);