Fix #106246: Crash on deleting the multi-level hierarchy #106374

Pratik Borhade · 2023-03-31T12:44:13+02:00

Pratik Borhade commented

2023-03-31 12:44:13 +02:00

Crash occurs due to accessing the pointer which points to garbage
memory. In brief, base_next points to next base element (base->next) which is
deleted in previous iteration. After BKE_library_ID_is_indirectly_used
call, viewlayer is synced and frees the object_bases list. It results
base_next pointing to garbage memory.
To fix this, point to base->next after BKE_library_ID_is_indirectly_used function call,

Crash occurs due to accessing the pointer which points to garbage memory. In brief, `base_next` points to next base element (`base->next`) which is deleted in previous iteration. After `BKE_library_ID_is_indirectly_used` call, viewlayer is synced and frees the `object_bases` list. It results `base_next` pointing to garbage memory. To fix this, point to `base->next` after `BKE_library_ID_is_indirectly_used` function call,

Pratik Borhade added 1 commit 2023-03-31 12:44:28 +02:00

4b8200666c Fix #106246 : Crash on deleting the multi-level hierarchy

Crash occurs due to accessing the pointer which points to garbage
memory. In brief, `base_next` points to next base element (`base->next`) which is
deleted in previous iteration. After `BKE_library_ID_is_indirectly_used`
call, viewlayer is synced and frees the `object_bases` list. It results
`base_next` pointing to garbage memory.
To fix this, point to `base->next` after `BKE_library_ID_is_indirectly_used` function call,

Pratik Borhade requested review from Julian Eisel 2023-03-31 12:44:44 +02:00

Pratik Borhade commented

2023-03-31 12:46:18 +02:00

I think we can also remove this assignment, no?
As far as I understand, base->next will always be NULL because this object is removed already from the collection/view layer.

I think we can also remove this assignment, no? As far as I understand, `base->next` will always be NULL because this object is removed already from the collection/view layer.

Pratik Borhade added this to the User Interface project 2023-04-03 15:48:56 +02:00

Pratik Borhade added the

Module

User Interface

label 2023-04-03 15:49:05 +02:00

Julian Eisel commented

2023-04-04 15:31:22 +02:00

Took me a while to understand what's going on and why this patch fixes it. The collection syncing just happens to be done in a foreach_id callback it seems (from the collection I guess). I think it would be better and more clear to do explicit syncing in the code that does the deletion.

This works for me (but I know too little about the collection syncing to judge):

diff --git a/source/blender/editors/space_outliner/outliner_tools.cc b/source/blender/editors/space_outliner/outliner_tools.cc
index d8d9718ccd6..de632f757e7 100644
--- a/source/blender/editors/space_outliner/outliner_tools.cc
+++ b/source/blender/editors/space_outliner/outliner_tools.cc
@@ -2213,6 +2213,7 @@ static Base *outliner_batch_delete_hierarchy(
 
   DEG_id_tag_update_ex(bmain, &object->id, ID_RECALC_BASE_FLAGS);
   BKE_scene_collections_object_remove(bmain, scene, object, false);
+  BKE_scene_view_layers_synced_ensure(scene);
 
   if (object->id.us == 0) {
     object->id.tag |= LIB_TAG_DOIT;

@mont29 what do you think?

Took me a while to understand what's going on and why this patch fixes it. The collection syncing just happens to be done in a `foreach_id` callback it seems (from the collection I guess). I think it would be better and more clear to do explicit syncing in the code that does the deletion. This works for me (but I know too little about the collection syncing to judge): ```diff diff --git a/source/blender/editors/space_outliner/outliner_tools.cc b/source/blender/editors/space_outliner/outliner_tools.cc index d8d9718ccd6..de632f757e7 100644 --- a/source/blender/editors/space_outliner/outliner_tools.cc +++ b/source/blender/editors/space_outliner/outliner_tools.cc @@ -2213,6 +2213,7 @@ static Base *outliner_batch_delete_hierarchy( DEG_id_tag_update_ex(bmain, &object->id, ID_RECALC_BASE_FLAGS); BKE_scene_collections_object_remove(bmain, scene, object, false); + BKE_scene_view_layers_synced_ensure(scene); if (object->id.us == 0) { object->id.tag |= LIB_TAG_DOIT; ``` @mont29 what do you think?

Julian Eisel requested review from Bastien Montagne 2023-04-05 16:27:32 +02:00

Julian Eisel refused to review 2023-04-05 16:27:41 +02:00

Bastien Montagne commented

2023-04-11 18:33:10 +02:00

Neither of these changes would fix the root of the issue imho, they just happen to work here because of 'chance' (and probably how allocation of memory works), but the root of the issue is as usual trying to modify data while iterating on it.

Existing code was also uselessly intricate, PR !106821 should hopefully makes things simpler, more efficient and less fragile altogether.

Neither of these changes would fix the root of the issue imho, they just happen to work here because of 'chance' (and probably how allocation of memory works), but the root of the issue is as usual trying to modify data while iterating on it. Existing code was also uselessly intricate, PR !106821 should hopefully makes things simpler, more efficient and less fragile altogether.

Bastien Montagne closed this pull request

2023-04-11 18:33:11 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #106246: Crash on deleting the multi-level hierarchy #106374

Pull request closed