Fix #113919: Avoid crashes with unsupported new socket types #114056

Lukas Tönne · 2023-10-23T11:51:31+02:00

Lukas Tönne commented

2023-10-23 11:51:31 +02:00

Blender 4.0 added new socket types that get written into legacy node
group interfaces by forward compatibility code. Such unsupported socket
types have to be handled by the socket declaration system and ignored
during execution.

Blender 4.0 added new socket types that get written into legacy node group interfaces by forward compatibility code. Such unsupported socket types have to be handled by the socket declaration system and ignored during execution.

Lukas Tönne added 1 commit 2023-10-23 11:51:45 +02:00

2a0fdd58d4 Fix #113919 : Avoid crashes with unsupported new socket types.

Blender 4.0 added new socket types that get written into legacy node
group interfaces by forward compatibility code. Such unsupported socket
types have to be handled by the socket declaration system and ignored
during execution.

Lukas Tönne commented

2023-10-23 11:54:19 +02:00

This patch should be cherry-picked into blender-v4.0-release when accepted.

The rotation socket now shows up as "undefined" and does not crash Blender any more:

This patch should be cherry-picked into `blender-v4.0-release` when accepted. The rotation socket now shows up as "undefined" and does not crash Blender any more: ![Screenshot_20231023_115239](/attachments/801819eb-42c3-4f75-8145-b9eaf6f133b3) ![Screenshot_20231023_115400](/attachments/e6fdec09-44a3-4159-bf70-60b9dcaa7e33)

Screenshot_20231023_115400.png

17 KiB

Screenshot_20231023_115239.png

8.7 KiB

Lukas Tönne requested review from Hans Goudey 2023-10-23 11:54:38 +02:00

Lukas Tönne added this to the Nodes & Physics project 2023-10-23 11:54:42 +02:00

Lukas Tönne added this to the 3.6 LTS milestone 2023-10-23 12:09:29 +02:00

Hans Goudey reviewed 2023-10-23 12:36:19 +02:00

Hans Goudey left a comment

Makes sense to me I suppose. Do you think it would be reasonable to continue writing the original idname to try to make going back to the newer version possible? Not sure how doable that would be.

source/blender/nodes/NOD_socket_declarations.hh Outdated

						
				@ -253,6 +253,17 @@ class Custom : public SocketDeclaration {

				  bool can_connect(const bNodeSocket &socket) const override;

				};

				/* Declare an undefined socket, to handle cases where an unknown builtin socket type is loaded.

Hans Goudey commented

2023-10-23 12:35:20 +02:00

/**
 * Declare an undefined socket, to handle cases where an unknown builtin socket type is loaded.
 * This should never be added deliberately, is only used as a fallback for forward compatibility. 
 */

``` /** * Declare an undefined socket, to handle cases where an unknown builtin socket type is loaded. * This should never be added deliberately, is only used as a fallback for forward compatibility. */

Lukas Tönne commented

2023-10-23 12:56:16 +02:00

Yeah, should be possible to copy the idname from the iosocket to the declaration and then use that instead of bke::NodeSocketTypeUndefined.idname. Then node_socket_set_typeinfo will set the NodeSocketTypeUndefined typeinfo when it can't find the socket type.

It won't be super useful in general because all the socket value details are missing (for rotation sockets that doesn't matter much). Primary concern here is to not crash Blender, saving new socket types in an older Blender version can't be expected to work of course.

Yeah, should be possible to copy the idname from the `iosocket` to the declaration and then use that instead of `bke::NodeSocketTypeUndefined.idname`. Then `node_socket_set_typeinfo` will set the `NodeSocketTypeUndefined` typeinfo when it can't find the socket type. It won't be super useful in general because all the socket value details are missing (for rotation sockets that doesn't matter much). Primary concern here is to not crash Blender, saving new socket types in an older Blender version can't be expected to work of course.

Hans Goudey commented

2023-10-23 13:01:22 +02:00

Primary concern here is to not crash Blender

Okay, right, easy to get carried away here.

>Primary concern here is to not crash Blender Okay, right, easy to get carried away here.

Lukas Tönne commented

2023-10-23 13:34:01 +02:00

Did a test with retaining the socket idname and then saving the file in 3.6. But this crashes when loading in 4.0 again, because when 4.0 finds a rotation socket it expects there to be valid default_value data, which does not get saved in 3.6. So we shouldn't even retain the idname if we don't know what to do with the default_value, and instead just create either an undefined socket (empty idname) or remove it altogether.

Did a test with retaining the socket idname and then saving the file in 3.6. But this crashes when loading in 4.0 again, because when 4.0 finds a rotation socket it expects there to be valid `default_value` data, which does not get saved in 3.6. So we shouldn't even retain the idname if we don't know what to do with the `default_value`, and instead just create either an undefined socket (empty idname) or remove it altogether.

👍 1

Lukas Tönne commented

2023-10-23 14:14:20 +02:00

I think this needs handling in the ntreeBlendReadData stage:
The socket->default_value is currently mapped regardless of whether Blender knows the socket type, and then carried around until some code doesn't know how to deal with it. If we then write a file with such an unknown socket type it might store default_value correctly, but if it's used as a node group interface socket the value gets discarded because undefined socket types don't handle default_value (memleak!). So to be safe we need to check if the socket type is a known built-in type and not do BLO_read_data_address on the default_value otherwise.

I think this needs handling in the `ntreeBlendReadData` stage: The `socket->default_value` is currently mapped regardless of whether Blender knows the socket type, and then carried around until some code doesn't know how to deal with it. If we then write a file with such an unknown socket type it _might_ store `default_value` correctly, but if it's used as a node group interface socket the value gets discarded because undefined socket types don't handle `default_value` (memleak!). So to be safe we need to check if the socket type is a known built-in type and not do `BLO_read_data_address` on the default_value otherwise.

LukasTonne marked this conversation as resolved

Hans Goudey approved these changes 2023-10-23 13:01:32 +02:00

Lukas Tönne added 2 commits 2023-10-23 15:03:25 +02:00

5553d4e65b Formatting fix.

ee85d98887 More robust solution, don't load sockets with default_value but unknown type.

Lukas Tönne requested review from Hans Goudey 2023-10-23 15:04:25 +02:00

Hans Goudey reviewed 2023-10-24 12:01:49 +02:00

source/blender/blenkernel/intern/node.cc

						
				@ -678,0 +713,4 @@

				    }

				    else {

				      /* Remove unsupported sockets. */

				      BLI_remlink(socket_list, sock);

Hans Goudey commented

2023-10-24 12:01:42 +02:00

What about making these sockets Undefined rather than removing them?

What about making these sockets `Undefined` rather than removing them?

Lukas Tönne commented

2023-10-24 12:23:39 +02:00

An "undefined" socket only makes sense when the socket retains its idname, so that the typeinfo can be restored to a known type later on. This feature was designed for custom (python) nodes, which works because they store all type-specific data in ID properties. In the case of built-in sockets of unknown type it doesn't work because the default_value is unknown and cannot be saved to files again. When storing an "undefined" builtin socket in 3.6 it won't write the default_value and then loading that in 4.0 will crash.

An "undefined" socket only makes sense when the socket retains its `idname`, so that the `typeinfo` can be restored to a known type later on. This feature was designed for custom (python) nodes, which works because they store all type-specific data in ID properties. In the case of built-in sockets of unknown type it doesn't work because the `default_value` is unknown and cannot be saved to files again. When storing an "undefined" builtin socket in 3.6 it won't write the `default_value` and then loading that in 4.0 will crash.

Hans Goudey commented

2023-10-30 17:12:18 +01:00

An "undefined" socket only makes sense when the socket retains its idname

Hmm, I see your point, but it still feels safer and more helpful to keep the socket around, even if the idname is set to NodeSocketUndefined (and the default value set to null)

>An "undefined" socket only makes sense when the socket retains its idname Hmm, I see your point, but it still feels safer and more helpful to keep the socket around, even if the idname is set to `NodeSocketUndefined` (and the default value set to null)

Lukas Tönne commented

2023-10-31 12:49:03 +01:00

It's doable but requires fixes in 4.0 if we want to make round trips from 4.0 to 3.6 and back to 4.0 possible.

Mainly geometry nodes (and possibly other node systems too) does not expect to find any unsupported socket types. If we replace the socket with a SOCK_CUSTOM/"NodeSocketUndefined" type then geometry nodes asserts will fail:
0dbc13529b/source/blender/nodes/intern/geometry_nodes_execute.cc (L705)

I have not tested beyond this assert, basically geometry nodes was never really tested with unknown socket types and i think that goes beyond the scope here.

It's doable but requires fixes in 4.0 if we want to make round trips from 4.0 to 3.6 and back to 4.0 possible. Mainly geometry nodes (and possibly other node systems too) does not expect to find any unsupported socket types. If we replace the socket with a `SOCK_CUSTOM`/`"NodeSocketUndefined"` type then geometry nodes asserts will fail: https://projects.blender.org/blender/blender/src/commit/0dbc13529b2c1cf07d1785db49208158b12c3e92/source/blender/nodes/intern/geometry_nodes_execute.cc#L705 I have not tested beyond this assert, basically geometry nodes was never really tested with unknown socket types and i think that goes beyond the scope here.

Hans Goudey reviewed 2023-10-30 17:12:40 +01:00

source/blender/blenkernel/intern/node.cc

						
				@ -665,0 +689,4 @@

				      is_known_builtin_type = true;

				      break;

				  }

				  return is_known_builtin_type || sock->default_value == nullptr;

Hans Goudey commented

2023-10-30 17:12:24 +01:00

sock->default_value == nullptr seems like a slightly weird way to test for SOCK_CUSTOM, was that the intent?

`sock->default_value == nullptr` seems like a slightly weird way to test for `SOCK_CUSTOM`, was that the intent?

Lukas Tönne commented

2023-10-30 18:47:35 +01:00

True, we should only need to test for known types, which also accepts SOCK_CUSTOM. Accepting sockets without default_value means newer builtin socket types that don't have DNA value data would still work. But that seems like an arbitrary distinction, so we might as well just reject all unknown type enum values.

True, we should only need to test for known types, which also accepts `SOCK_CUSTOM`. Accepting sockets without `default_value` means newer builtin socket types that don't have DNA value data would still work. But that seems like an arbitrary distinction, so we might as well just reject all unknown type enum values.

LukasTonne marked this conversation as resolved

Hans Goudey requested review from Jacques Lucke 2023-10-31 12:52:13 +01:00

Lukas Tönne added 1 commit 2023-10-31 12:58:14 +01:00

buildbot/vexp-code-patch-coordinator Build done. Details

4e838482ba Ignore default_value data if socket type is unknown.

Jacques Lucke commented

2023-10-31 13:32:08 +01:00

@blender-bot build

Jacques Lucke approved these changes 2023-10-31 13:33:28 +01:00

Jacques Lucke left a comment

Seems reasonable. Features added in newer versions of Blender can be expected to be removed when the file is opened in an older version I think. This can't generally work anyway.

Hans Goudey approved these changes 2023-11-02 10:45:20 +01:00

Lukas Tönne commented

2023-11-02 10:48:51 +01:00

@blender-bot build

Lukas Tönne merged commit 67cc703b99 into blender-v3.6-release

2023-11-02 11:21:01 +01:00

Lukas Tönne referenced this issue from a commit

2023-11-02 11:21:01 +01:00

Fix #113919: Avoid crashes with unsupported new socket types

Lukas Tönne deleted branch fix-unsupported-socket-type-forward-compat

2023-11-02 11:21:06 +01:00

Lukas Tönne referenced this issue from a commit