Page MenuHome

Crash after deleting material leaves invalid pointer in node editor
Closed, ResolvedPublicBUG

Description

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: rBc036ef136960
Worked: 2.83 (sub 11), branch: master, commit date: 2020-03-31 23:07, hash: rBda3cb514e52d
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'

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.

Event Timeline

Brecht Van Lommel (brecht) renamed this task from Segfault from Removing materials + CUDA + loading a collection to Crash after deleting material leaves invalid pointer in node editor.May 9 2020, 11:28 AM
Brecht Van Lommel (brecht) changed the task status from Needs Triage to Confirmed.
Brecht Van Lommel (brecht) changed the subtype of this task from "Report" to "Bug".

@Bastien Montagne (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.

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...