From 94e6ab6d718735b9646bd11fb3196c5fe224fd74 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Wed, 21 Jun 2023 15:21:04 +0200 Subject: [PATCH] Fix #108923: Serious issues in 'undo_preserve' process of Scene's toolsettings. 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. --- source/blender/blenkernel/intern/scene.cc | 377 ++++++++++--------- source/blender/blenloader/intern/readfile.cc | 14 +- 2 files changed, 204 insertions(+), 187 deletions(-) diff --git a/source/blender/blenkernel/intern/scene.cc b/source/blender/blenkernel/intern/scene.cc index 5f95b3104c8..fdf4c070788 100644 --- a/source/blender/blenkernel/intern/scene.cc +++ b/source/blender/blenkernel/intern/scene.cc @@ -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); } } diff --git a/source/blender/blenloader/intern/readfile.cc b/source/blender/blenloader/intern/readfile.cc index 442010955e8..2354c174fa4 100644 --- a/source/blender/blenloader/intern/readfile.cc +++ b/source/blender/blenloader/intern/readfile.cc @@ -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);