Fix #111607: Do not unregister internal nodes #111615

YimingWu · 2023-08-28T15:47:36+02:00

YimingWu commented

2023-08-28 15:47:36 +02:00

Registering a node with a bl_idname same as a built-in node will lead
to crash because rna_Node_unregister does not check nt->rna_ext.data
to see whether it's not null (which indicates whether this node is
registered with python or not). Now fixed.

Registering a node with a `bl_idname` same as a built-in node will lead to crash because `rna_Node_unregister` does not check `nt->rna_ext.data` to see whether it's not null (which indicates whether this node is registered with python or not). Now fixed.

YimingWu added 1 commit 2023-08-28 15:47:50 +02:00

ce78f5a24a Fix #111607 : Do not unregister internal nodes

Registering a node with a `bl_idname` same as a built-in node will lead
to crash because `rna_Node_unregister` does not check `nt->rna_ext.data`
to see whether it's not null (which indicates whether this node is
registered with python or not). Now fixed.

Iliya Katushenock added this to the Nodes & Physics project 2023-08-28 15:49:58 +02:00

Iliya Katushenock added the

Module

Python API

label 2023-08-28 15:50:04 +02:00

Hans Goudey reviewed 2023-08-28 15:53:39 +02:00

source/blender/makesrna/intern/rna_nodetree.cc Outdated

						
				@ -1769,3 +1769,3 @@

				  bNodeType *nt = static_cast<bNodeType *>(RNA_struct_blender_type_get(type));

				  if (!nt) {

				  /** `nt->rna_ext.data` is the python object. If it's nullptr then it's a

Hans Goudey commented

2023-08-28 15:53:39 +02:00

Picky:

The /** comment format is meant for function/struct documentation. Comments inside of functions can just use /* like normal.

Also, it's a internally -> it's an internally

Picky: The `/**` comment format is meant for function/struct documentation. Comments inside of functions can just use `/*` like normal. Also, `it's a internally` -> `it's an internally`

ChengduLittleA marked this conversation as resolved

Vitor Boschi commented

2023-08-28 18:55:30 +02:00

Even though this patch prevents the crash, it looks to me like it still allows the node to be created with a duplicated identifier, no? Isn't that a problem?

YimingWu added 1 commit 2023-08-29 03:48:37 +02:00

65f58de768 Fix comment block and typo.

YimingWu commented

2023-08-29 03:50:59 +02:00

@vitorboschi ah, the code will return null in rna_Node_unregister and error out on rna_Node_register_base, and a new node type is never created. :D

@vitorboschi ah, the code will return null in `rna_Node_unregister` and error out on `rna_Node_register_base`, and a new node type is never created. :D

Hans Goudey approved these changes 2023-08-29 03:52:01 +02:00

YimingWu merged commit 0798aae01c into main

2023-08-29 09:35:53 +02:00

YimingWu deleted branch fix-111607

2023-08-29 09:35:54 +02:00

YimingWu referenced this issue from a commit

2023-08-29 09:35:57 +02:00

Fix #111607: Do not unregister internal nodes

YimingWu referenced this pull request

2023-08-29 09:36:44 +02:00

Blender LTS: Maintenance Task 3.6 #109399

Vitor Boschi commented

2023-08-29 13:51:02 +02:00

@vitorboschi ah, the code will return null in rna_Node_unregister and error out on rna_Node_register_base, and a new node type is never created. :D

I missed that! Thanks for the clarification!
Would it make sense to add the same check at rna_Node_register_base so we can generate a better error message when this happens? Currently it will show RuntimeError: Error: Registering node class: 'NodeDummy', bl_idname 'NodeGroupInput' could not be unregistered, which is kinda cryptic considering the user asked to register a class.

I can create a patch for that if the change is desirable.

> @vitorboschi ah, the code will return null in `rna_Node_unregister` and error out on `rna_Node_register_base`, and a new node type is never created. :D I missed that! Thanks for the clarification! Would it make sense to add the same check at `rna_Node_register_base` so we can generate a better error message when this happens? Currently it will show `RuntimeError: Error: Registering node class: 'NodeDummy', bl_idname 'NodeGroupInput' could not be unregistered`, which is kinda cryptic considering the user asked to register a class. I can create a patch for that if the change is desirable.

YimingWu commented

2023-09-01 04:44:07 +02:00

@vitorboschi Oh it's definitely better if you could make a more clarified message, yes :D

Vitor Boschi referenced this pull request

2023-09-01 18:52:19 +02:00

PyAPI: Better error msg when re-using internal node id #111799

YimingWu referenced this issue from a commit

2023-09-02 04:41:15 +02:00

PyAPI: Better error msg when re-using internal node id

Philipp Oeser referenced this issue from a commit

2023-09-05 15:05:56 +02:00

Fix #111607: Do not unregister internal nodes