Depsgraph use-after-free after scene switching undo #89196

New Issue

Julian Eisel · 2021-06-16T12:46:43+02:00

Julian Eisel commented

2021-06-16 12:46:43 +02:00

System Information
Operating system: Linux-5.11.0-7614-generic-x86_64-with-glibc2.32 64 Bits
Graphics card: Radeon Vega Frontier Edition (VEGA10, DRM 3.40.0, 5.11.0-7614-generic, LLVM 11.0.0) AMD 4.6 (Core Profile) Mesa 21.0.0

Blender Version
Broken: version: 3.0.0 Alpha

Short description of error
The deg_graph_on_visible_update() triggered by a scene change may access a master collection via IDNode.id_orig that was freed.

Exact steps for others to reproduce the error

The attached .blend was prepared with the following steps:

Factory settings
Create two new scenes
Save file

T89196_depsgraph_scene_switch_undo_crash.blend

With the attached file:

Activate scene "Scene.001"
Undo
Activate scene "Scene"

ASan output: {P2182 lines=14}

Setting a breakpoint in deg_graph_on_visible_update(), I see that:

The IDNode.id_orig of the iterated ID is indeed freed.
Its IDNode.id_cow has a name of "GRMaster Collection"

So it seems the original ID pointer in the master collection (an "embedded ID") node is not updated when the scene is freed on undo.

**System Information** Operating system: Linux-5.11.0-7614-generic-x86_64-with-glibc2.32 64 Bits Graphics card: Radeon Vega Frontier Edition (VEGA10, DRM 3.40.0, 5.11.0-7614-generic, LLVM 11.0.0) AMD 4.6 (Core Profile) Mesa 21.0.0 **Blender Version** Broken: version: 3.0.0 Alpha **Short description of error** The `deg_graph_on_visible_update()` triggered by a scene change may access a master collection via `IDNode.id_orig` that was freed. **Exact steps for others to reproduce the error** The attached .blend was prepared with the following steps: * Factory settings * Create two new scenes * Save file [T89196_depsgraph_scene_switch_undo_crash.blend](https://archive.blender.org/developer/F10174859/T89196_depsgraph_scene_switch_undo_crash.blend) With the attached file: * Activate scene "Scene.001" * Undo * Activate scene "Scene" --- ASan output: {[P2182](https://archive.blender.org/developer/P2182.txt) lines=14} Setting a breakpoint in `deg_graph_on_visible_update()`, I see that: * The `IDNode.id_orig` of the iterated ID is indeed freed. * Its `IDNode.id_cow` has a name of "GRMaster Collection" So it seems the original ID pointer in the master collection (an "embedded ID") node is not updated when the scene is freed on undo.

Julian Eisel commented

2021-06-16 12:46:43 +02:00

Added subscriber: @JulianEisel

Sergey Sharybin commented

2021-06-16 12:58:01 +02:00

Added subscriber: @Sergey

Sergey Sharybin commented

2021-06-16 12:58:01 +02:00

Will have a look in a bit unless someone else does it :)

Jacques Lucke commented

2021-06-16 15:27:55 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Jacques Lucke commented

2021-06-16 16:10:31 +02:00

Added subscriber: @JacquesLucke

Jacques Lucke commented

2021-06-16 16:10:32 +02:00

I was looking into this a little bit and found that the following (wrong) patch would fix it. Maybe that helps someone.

diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc
index 614315c5c5e..19fc7b8393c 100644
--- a/source/blender/depsgraph/intern/depsgraph_tag.cc
+++ b/source/blender/depsgraph/intern/depsgraph_tag.cc
@@ -816,6 +816,7 @@ void DEG_graph_on_visible_update(Main *bmain, Depsgraph *depsgraph, const bool d
 void DEG_on_visible_update(Main *bmain, const bool do_time)
 {
   for (deg::Depsgraph *depsgraph : deg::get_all_registered_graphs(bmain)) {
+    DEG_graph_relations_update((Depsgraph *)depsgraph);
     DEG_graph_on_visible_update(bmain, reinterpret_cast<::Depsgraph *>(depsgraph), do_time);
   }
 }

I was looking into this a little bit and found that the following (wrong) patch would fix it. Maybe that helps someone. ``` diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc index 614315c5c5e..19fc7b8393c 100644 --- a/source/blender/depsgraph/intern/depsgraph_tag.cc +++ b/source/blender/depsgraph/intern/depsgraph_tag.cc @@ -816,6 +816,7 @@ void DEG_graph_on_visible_update(Main *bmain, Depsgraph *depsgraph, const bool d void DEG_on_visible_update(Main *bmain, const bool do_time) { for (deg::Depsgraph *depsgraph : deg::get_all_registered_graphs(bmain)) { + DEG_graph_relations_update((Depsgraph *)depsgraph); DEG_graph_on_visible_update(bmain, reinterpret_cast<::Depsgraph *>(depsgraph), do_time); } } ```

Sergey Sharybin commented

2021-06-16 16:21:40 +02:00

I am not that much sure it is wrong even. In many usages usages (including the most important one in the even loop) the relations are updated prior to DEG_graph_on_visible_update(). Knowing relations are up-to-date does simplify a lot of things, probably without actual slowdown even.

Not only from the use-after-free usecase, if the object is removed from DEG and DEG becomes visible, there is no need to tag that ID.

One thing I wanted to try is to convert DEG_graph_on_visible_update to the tagging flag. As in, tag depsgraph "please tag IDs as if the DEG became visible, whenever it is convenient for you". Then we can ensure valid access and order of things during DEG evaluation and do as following:

If relations are tagged for update: update relations
If visibility is tagged for update: update visibility
If anything is tagged for update in DEG, evaluate the DEG

I am not that much sure it is wrong even. In many usages usages (including the most important one in the even loop) the relations are updated prior to `DEG_graph_on_visible_update()`. Knowing relations are up-to-date does simplify a lot of things, probably without actual slowdown even. Not only from the use-after-free usecase, if the object is removed from DEG and DEG becomes visible, there is no need to tag that ID. One thing I wanted to try is to convert `DEG_graph_on_visible_update` to the tagging flag. As in, tag depsgraph "please tag IDs as if the DEG became visible, whenever it is convenient for you". Then we can ensure valid access and order of things during DEG evaluation and do as following: - If relations are tagged for update: update relations - If visibility is tagged for update: update visibility - If anything is tagged for update in DEG, evaluate the DEG

blender-admin commented

2021-06-21 14:27:26 +02:00

This issue was referenced by 956c539e59

This issue was referenced by 956c539e597aed84c355c8336dfd5797f4e69ea7

Sergey Sharybin self-assigned this 2021-06-21 15:04:36 +02:00

Sergey Sharybin commented

2021-06-22 10:04:14 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Sergey Sharybin closed this issue

2021-06-22 10:04:14 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Depsgraph use-after-free after scene switching undo #89196