Crash after deleting material leaves invalid pointer in node editor #76559

New Issue

Nathan Kundtz · 2020-05-08T17:49:26+02:00

Nathan Kundtz commented

2020-05-08 17:49:26 +02:00

System Information
Operating system: Linux-5.3.0-51-generic-x86_64-with-debian-buster-sid 64 Bits
Graphics card: GeForce GTX 1050/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 440.59

Blender Version
Broken: version: 2.83 (sub 15), branch: master, commit date: 2020-05-05 22:28, hash: c036ef1369
Worked: 2.83 (sub 11), branch: master, commit date: 2020-03-31 23:07, hash: da3cb514e5
Worked: Blender 2.83 (sub 12) (hash 71b1ee940b built 2020-04-06 14:49:34)

Short description of error
Blender will segfault after loading a collection from a library file under some conditions. The minimal example I found was when removing materials from a file then checking for available cuda devices followed by a load.

Segfault occurs on all example files tested including saving the blender startup file. In the case of the attached script it has been saved as 'untitled.blend'

load_test.py

Exact steps for others to reproduce the error
blender --background --python load_test.py
[Based on the default startup]

Backtrace of segfault

- 0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:62
- 1  0x00007f2836f264d3 in _IO_vfprintf_internal (s=s@entry=0x7fff40ae6f70, format=format@entry=0x76fa300 "ID user count error: %s (from '%s')", 
    ap=ap@entry=0x7fff40ae70e8) at vfprintf.c:1643
#2  0x00007f2836f51910 in _IO_vsnprintf (string=0x7fff40ae720c "ID user count error: x (from '", maxlen=<optimized out>, 
    format=0x76fa300 "ID user count error: %s (from '%s')", args=0x7fff40ae70e8) at vsnprintf.c:114
- 3  0x000000000172862b in ?? ()
- 4  0x0000000001728a45 in CLG_logf ()
- 5  0x0000000000c58a3e in id_us_ensure_real ()
- 6  0x0000000000c58b10 in ?? ()
- 7  0x0000000000c5d4e7 in ?? ()
- 8  0x0000000000c61961 in ?? ()
- 9  0x0000000000c66069 in BKE_library_foreach_ID_link ()
- 10 0x0000000000c5a0af in BKE_main_id_refcount_recompute ()
- 11 0x0000000000f67126 in BLO_library_link_end ()
- 12 0x0000000001203c0e in ?? ()
- 13 0x00000000057096a1 in _PyMethodDef_RawFastCallDict ()
- 14 0x0000000005709765 in _PyCFunction_FastCallDict ()
- 15 0x0000000000c0a0c0 in _PyEval_EvalFrameDefault ()
- 16 0x00000000057ba95c in _PyEval_EvalCodeWithName ()
- 17 0x00000000057baa8e in PyEval_EvalCodeEx ()
- 18 0x00000000057baabb in PyEval_EvalCode ()
- 19 0x00000000057edcb2 in PyRun_FileExFlags ()
- 20 0x00000000011f0f21 in ?? ()
#21 0x0000000000c12558 in _start ()

THANKS
As always, really appreciate your work. This isn't blocking for us right now because we can eliminate the offending material removal line; but I thought it would be helpful to report as a bug ahead of an LTS release.

**System Information** Operating system: Linux-5.3.0-51-generic-x86_64-with-debian-buster-sid 64 Bits Graphics card: GeForce GTX 1050/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 440.59 **Blender Version** Broken: version: 2.83 (sub 15), branch: master, commit date: 2020-05-05 22:28, hash: `c036ef1369` Worked: 2.83 (sub 11), branch: master, commit date: 2020-03-31 23:07, hash: `da3cb514e5` Worked: Blender 2.83 (sub 12) (hash 71b1ee940bb0 built 2020-04-06 14:49:34) **Short description of error** Blender will segfault after loading a collection from a library file under some conditions. The minimal example I found was when removing materials from a file then checking for available cuda devices followed by a load. Segfault occurs on all example files tested including saving the blender startup file. In the case of the attached script it has been saved as 'untitled.blend' [load_test.py](https://archive.blender.org/developer/F8520970/load_test.py) **Exact steps for others to reproduce the error** blender --background --python load_test.py [Based on the default startup] **Backtrace of segfault** ``` - 0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:62 - 1 0x00007f2836f264d3 in _IO_vfprintf_internal (s=s@entry=0x7fff40ae6f70, format=format@entry=0x76fa300 "ID user count error: %s (from '%s')", ap=ap@entry=0x7fff40ae70e8) at vfprintf.c:1643 #2 0x00007f2836f51910 in _IO_vsnprintf (string=0x7fff40ae720c "ID user count error: x (from '", maxlen=<optimized out>, format=0x76fa300 "ID user count error: %s (from '%s')", args=0x7fff40ae70e8) at vsnprintf.c:114 - 3 0x000000000172862b in ?? () - 4 0x0000000001728a45 in CLG_logf () - 5 0x0000000000c58a3e in id_us_ensure_real () - 6 0x0000000000c58b10 in ?? () - 7 0x0000000000c5d4e7 in ?? () - 8 0x0000000000c61961 in ?? () - 9 0x0000000000c66069 in BKE_library_foreach_ID_link () - 10 0x0000000000c5a0af in BKE_main_id_refcount_recompute () - 11 0x0000000000f67126 in BLO_library_link_end () - 12 0x0000000001203c0e in ?? () - 13 0x00000000057096a1 in _PyMethodDef_RawFastCallDict () - 14 0x0000000005709765 in _PyCFunction_FastCallDict () - 15 0x0000000000c0a0c0 in _PyEval_EvalFrameDefault () - 16 0x00000000057ba95c in _PyEval_EvalCodeWithName () - 17 0x00000000057baa8e in PyEval_EvalCodeEx () - 18 0x00000000057baabb in PyEval_EvalCode () - 19 0x00000000057edcb2 in PyRun_FileExFlags () - 20 0x00000000011f0f21 in ?? () #21 0x0000000000c12558 in _start () ``` **THANKS** As always, really appreciate your work. This isn't blocking for us right now because we can eliminate the offending material removal line; but I thought it would be helpful to report as a bug ahead of an LTS release.

Nathan Kundtz commented

2020-05-08 17:49:27 +02:00

Added subscriber: @nkundtz

Brecht Van Lommel changed title from ~~Segfault from Removing materials + CUDA + loading a collection~~ to Crash after deleting material leaves invalid pointer in node editor

2020-05-09 11:28:59 +02:00

Brecht Van Lommel commented

2020-05-09 11:30:30 +02:00

Added subscribers: @mont29, @brecht

Brecht Van Lommel commented

2020-05-09 11:30:30 +02:00

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

Brecht Van Lommel commented

2020-05-09 11:30:30 +02:00

@mont29, I believe the issue that the material datablock gets deleted, then in the node editor snode->id is updated but snode->nodetree pointing to the embedded node tree of snode->id is not.

snode->nodetree has a dual purpose that is quite weak. It can either point to the embedded nodetree of snode->id, or to a node group. And depending on that the user counting is different. For the embedded case, lib query should not iterate over it I think, there's nothing useful that can be done with that pointer without also knowing snode->id.

This seems to work, but it's unclear to me if updating pointers here is OK.

diff --git a/source/blender/blenkernel/intern/lib_query.c b/source/blender/blenkernel/intern/lib_query.c
index 0acd4c3..e723618 100644
--- a/source/blender/blenkernel/intern/lib_query.c
+++ b/source/blender/blenkernel/intern/lib_query.c
@@ -488,21 +488,32 @@ static void library_foreach_screen_area(LibraryForeachIDData *data, ScrArea *are
       case SPACE_NODE: {
         SpaceNode *snode = (SpaceNode *)sl;

-        const bool is_private_nodetree = snode->id != NULL &&
-                                         ntreeFromID(snode->id) == snode->nodetree;
+        const bNodeTree *embedded_nodetree = (snode->id) ? ntreeFromID(snode->id) : NULL;

         FOREACH_CALLBACK_INVOKE_ID(data, snode->id, IDWALK_CB_NOP);
         FOREACH_CALLBACK_INVOKE_ID(data, snode->from, IDWALK_CB_NOP);

-        FOREACH_CALLBACK_INVOKE(
-            data, snode->nodetree, is_private_nodetree ? IDWALK_CB_EMBEDDED : IDWALK_CB_USER_ONE);
+        bNodeTree *new_embedded_nodetree = (snode->id) ? ntreeFromID(snode->id) : NULL;
+        if (embedded_nodetree) {
+          /* Update embdeed node tree pointer if ID changed. */
+          snode->nodetree = new_embedded_nodetree;
+        }
+        else {
+          /* Iterate over node group reference. */
+          FOREACH_CALLBACK_INVOKE(data, snode->nodetree, IDWALK_CB_USER_ONE);
+        }

         LISTBASE_FOREACH (bNodeTreePath *, path, &snode->treepath) {
           if (path == snode->treepath.first) {
-            /* first nodetree in path is same as snode->nodetree */
-            FOREACH_CALLBACK_INVOKE(data,
-                                    path->nodetree,
-                                    is_private_nodetree ? IDWALK_CB_EMBEDDED : IDWALK_CB_USER_ONE);
+            /* First nodetree in path is same as snode->nodetree. */
+            if (embedded_nodetree) {
+              /* Update embdeed node tree pointer if ID changed. */
+              path->nodetree = new_embedded_nodetree;
+            }
+            else {
+              /* Iterate over node group reference. */
+              FOREACH_CALLBACK_INVOKE(data, path->nodetree, IDWALK_CB_USER_ONE);
+            }
           }
           else {
             FOREACH_CALLBACK_INVOKE(data, path->nodetree, IDWALK_CB_USER_ONE);

Maybe a better solution would be to set snode->nodetree and path->nodetree always to NULL for the embedded nodetree case, and instead have an accessor for every place that needs to read it.

@mont29, I believe the issue that the material datablock gets deleted, then in the node editor `snode->id` is updated but `snode->nodetree` pointing to the embedded node tree of `snode->id` is not. `snode->nodetree` has a dual purpose that is quite weak. It can either point to the embedded nodetree of `snode->id`, or to a node group. And depending on that the user counting is different. For the embedded case, lib query should not iterate over it I think, there's nothing useful that can be done with that pointer without also knowing `snode->id`. This seems to work, but it's unclear to me if updating pointers here is OK. ``` diff --git a/source/blender/blenkernel/intern/lib_query.c b/source/blender/blenkernel/intern/lib_query.c index 0acd4c3..e723618 100644 --- a/source/blender/blenkernel/intern/lib_query.c +++ b/source/blender/blenkernel/intern/lib_query.c @@ -488,21 +488,32 @@ static void library_foreach_screen_area(LibraryForeachIDData *data, ScrArea *are case SPACE_NODE: { SpaceNode *snode = (SpaceNode *)sl; - const bool is_private_nodetree = snode->id != NULL && - ntreeFromID(snode->id) == snode->nodetree; + const bNodeTree *embedded_nodetree = (snode->id) ? ntreeFromID(snode->id) : NULL; FOREACH_CALLBACK_INVOKE_ID(data, snode->id, IDWALK_CB_NOP); FOREACH_CALLBACK_INVOKE_ID(data, snode->from, IDWALK_CB_NOP); - FOREACH_CALLBACK_INVOKE( - data, snode->nodetree, is_private_nodetree ? IDWALK_CB_EMBEDDED : IDWALK_CB_USER_ONE); + bNodeTree *new_embedded_nodetree = (snode->id) ? ntreeFromID(snode->id) : NULL; + if (embedded_nodetree) { + /* Update embdeed node tree pointer if ID changed. */ + snode->nodetree = new_embedded_nodetree; + } + else { + /* Iterate over node group reference. */ + FOREACH_CALLBACK_INVOKE(data, snode->nodetree, IDWALK_CB_USER_ONE); + } LISTBASE_FOREACH (bNodeTreePath *, path, &snode->treepath) { if (path == snode->treepath.first) { - /* first nodetree in path is same as snode->nodetree */ - FOREACH_CALLBACK_INVOKE(data, - path->nodetree, - is_private_nodetree ? IDWALK_CB_EMBEDDED : IDWALK_CB_USER_ONE); + /* First nodetree in path is same as snode->nodetree. */ + if (embedded_nodetree) { + /* Update embdeed node tree pointer if ID changed. */ + path->nodetree = new_embedded_nodetree; + } + else { + /* Iterate over node group reference. */ + FOREACH_CALLBACK_INVOKE(data, path->nodetree, IDWALK_CB_USER_ONE); + } } else { FOREACH_CALLBACK_INVOKE(data, path->nodetree, IDWALK_CB_USER_ONE); ``` Maybe a better solution would be to set `snode->nodetree` and `path->nodetree` always to `NULL` for the embedded nodetree case, and instead have an accessor for every place that needs to read it.

Bastien Montagne self-assigned this 2020-05-11 15:50:24 +02:00

Bastien Montagne commented

2020-05-11 15:50:24 +02:00

Uuuuuhhhh.... I'd really rather avoid that kind of super-hacky things in that code, it's kind of violating the whole design (though it would probably work fine in all current usecases)...

I'll try just removing the 'embedded' flag, since this is actually wrong here (those nodetrees are not owned by, embedded in the editor). If that does not works, and on the longer run in any case, I think we should rather have accessors for those indeed, assuming that NULL pointer == use embedded tree from id...

Uuuuuhhhh.... I'd really rather avoid that kind of super-hacky things in that code, it's kind of violating the whole design (though it would probably work fine in all current usecases)... I'll try just removing the 'embedded' flag, since this is actually wrong here (those nodetrees are not owned by, embedded in the editor). If that does not works, and on the longer run in any case, I think we should rather have accessors for those indeed, assuming that NULL pointer == use embedded tree from id...

blender-admin commented

2020-05-11 17:24:56 +02:00

This issue was referenced by 4e1a6b48e7

This issue was referenced by 4e1a6b48e725186822117a9cce86e07ec85de28b

Bastien Montagne commented

2020-05-11 17:30:50 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Bastien Montagne closed this issue

2020-05-11 17:30:50 +02:00

Duane Harkness commented

2020-05-11 23:01:55 +02:00

Added subscriber: @dharkness

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

Crash after deleting material leaves invalid pointer in node editor #76559