WIP: Fix #103824: CompositorNodeOutputFile inputs.new() crash #118807

Philipp Oeser · 2024-02-27T15:56:58+01:00

Philipp Oeser commented

2024-02-27 15:56:58 +01:00

Even though this has a more dedicated API in file_slots.new() this
shouldnt crash.

The crash is due to NodeImageMultiFileSocket not being initialized
properly, ntreeBlendWrite then crashes accessing garbage data.

Note this is more of a bandaid, the proper way forward would be some
sort of dynamic declration such as
blender::nodes::node_group_declare_dynamic as outlined in #108728, but
since this is a more involved change, this PR just fixes the crash with
what we have atm. (using ntreeCompositOutputFileAddSocket which takes
care of NodeImageMultiFileSocket data for us).

Even though this has a more dedicated API in `file_slots.new()` this shouldnt crash. The crash is due to `NodeImageMultiFileSocket` not being initialized properly, `ntreeBlendWrite` then crashes accessing garbage data. Note this is more of a bandaid, the proper way forward would be some sort of dynamic declration such as `blender::nodes::node_group_declare_dynamic` as outlined in #108728, but since this is a more involved change, this PR just fixes the crash with what we have atm. (using `ntreeCompositOutputFileAddSocket` which takes care of `NodeImageMultiFileSocket` data for us).

Philipp Oeser added 1 commit 2024-02-27 15:57:09 +01:00

dbe557e8cb Fix #103824 : CompositorNodeOutputFile inputs.new() crash

Even though this has a more dedicated API in `file_slots.new()` this
shouldnt crash.

The crash is due to `NodeImageMultiFileSocket` not being initialized
properly, `ntreeBlendWrite` then crashes accessing garbage data.

Note this is more of a bandaid, the proper way forward would be some
sort of dynamic declration such as
`blender::nodes::node_group_declare_dynamic` as outlined in #108728, but
since this is a more involved change, this PR just fixes the crash with
what we have atm. (using `ntreeCompositOutputFileAddSocket` which takes
care of `NodeImageMultiFileSocket` data for us).

Philipp Oeser added this to the Nodes & Physics project 2024-02-27 15:57:23 +01:00

Philipp Oeser requested review from Hans Goudey 2024-02-27 15:57:33 +01:00

Philipp Oeser referenced this pull request

2024-02-27 16:02:26 +01:00

output_node.inputs.new() crash blender #103824

Raul Fernandez Hernandez approved these changes 2024-02-27 21:54:30 +01:00

Raul Fernandez Hernandez left a comment

With a follow up PR it looks good to me.

Hans Goudey commented

2024-02-27 22:01:59 +01:00

What about changing allow_changing_sockets? That would be an API change technically, but if it's just not working to change the sockets directly, maybe it's better that people just use the dedicated API. Otherwise delaying that to 4.2 and following up with a change to use dynamic declarations would be fine.

What about changing `allow_changing_sockets`? That would be an API change technically, but if it's just not working to change the sockets directly, maybe it's better that people just use the dedicated API. Otherwise delaying that to 4.2 and following up with a change to use dynamic declarations would be fine.

Philipp Oeser commented

2024-02-28 10:00:49 +01:00

What about changing allow_changing_sockets? That would be an API change technically, but if it's just not working to change the sockets directly, maybe it's better that people just use the dedicated API

Possible of course (and I have advised reporters to use the dedicated API), but not sure breaking the API just for a release or two is worth it (if we can make the general API work as well)?

Otherwise delaying that to 4.2 and following up with a change to use dynamic declarations would be fine.

Not sure I understand correctly, you mean delaying this fix to 4.2? Why?

If the way this PR does the fix is a no-go, I would vote for the allow_changing_sockets solution, but still do it for 4.1 (unless having an API change like this is a no-go in bcon3?)

> What about changing `allow_changing_sockets`? That would be an API change technically, but if it's just not working to change the sockets directly, maybe it's better that people just use the dedicated API Possible of course (and I have advised reporters to use the dedicated API), but not sure breaking the API just for a release or two is worth it (if we can make the general API work as well)? >Otherwise delaying that to 4.2 and following up with a change to use dynamic declarations would be fine. Not sure I understand correctly, you mean delaying this fix to 4.2? Why? If the way this PR does the fix is a no-go, I would vote for the `allow_changing_sockets` solution, but still do it for 4.1 (unless having an API change like this is a no-go in bcon3?)

Hans Goudey requested review from Jacques Lucke 2024-02-28 17:24:22 +01:00

Jacques Lucke commented

2024-02-28 22:09:22 +01:00

I think c0f33814c1 might have been the wrong fix for #102611. The bug report specifically asks for file_output_node.file_slots.remove and not to remove sockets directly. However, the fix allowed that.

I think the fix here is to remove CMP_NODE_OUTPUT_FILE from allow_changing_sockets and to use more specific rna functions in rna_def_cmp_output_file_slots_api. See for example how rna_Node_socket_remove is reused.

I think c0f33814c13c38aa49b909ba2a8946798d14d078 might have been the wrong fix for #102611. The bug report specifically asks for `file_output_node.file_slots.remove` and not to remove sockets directly. However, the fix allowed that. I think the fix here is to remove `CMP_NODE_OUTPUT_FILE` from `allow_changing_sockets` and to use more specific rna functions in `rna_def_cmp_output_file_slots_api`. See for example how `rna_Node_socket_remove` is reused.

👍 1 👀 1

Philipp Oeser commented

2024-02-29 09:43:18 +01:00

I think c0f33814c1 might have been the wrong fix for #102611. The bug report specifically asks for file_output_node.file_slots.remove and not to remove sockets directly. However, the fix allowed that.

I think the fix here is to remove CMP_NODE_OUTPUT_FILE from allow_changing_sockets and to use more specific rna functions in rna_def_cmp_output_file_slots_api. See for example how rna_Node_socket_remove is reused.

OK, will take that route.
@3di : that would work for you as well, right?

> I think c0f33814c13c38aa49b909ba2a8946798d14d078 might have been the wrong fix for #102611. The bug report specifically asks for `file_output_node.file_slots.remove` and not to remove sockets directly. However, the fix allowed that. > > I think the fix here is to remove `CMP_NODE_OUTPUT_FILE` from `allow_changing_sockets` and to use more specific rna functions in `rna_def_cmp_output_file_slots_api`. See for example how `rna_Node_socket_remove` is reused. OK, will take that route. @3di : that would work for you as well, right?

Philipp Oeser changed title from ~~Fix #103824: CompositorNodeOutputFile inputs.new() crash~~ to WIP: Fix #103824: CompositorNodeOutputFile inputs.new() crash

2024-02-29 09:43:34 +01:00

michael campbell commented

2024-02-29 12:16:51 +01:00

First-time contributor

@lichtwerk as long as the below code continues remove the actual socket I'm happy (or if there's an alternative way) 👍

import bpy
scene = bpy.context.scene
file_output_node =scene.node_tree.nodes.new("CompositorNodeOutputFile")
file_output_node.file_slots.remove(file_output_node.inputs[0])

@lichtwerk as long as the below code continues remove the actual socket I'm happy (or if there's an alternative way) 👍 ``` import bpy scene = bpy.context.scene file_output_node =scene.node_tree.nodes.new("CompositorNodeOutputFile") file_output_node.file_slots.remove(file_output_node.inputs[0]) ```

Weikang-Qiu referenced this pull request

2024-04-13 20:49:46 +02:00

Rewrite some compositing nodes to using declarations #108728

Iliya Katushenock referenced this pull request

2024-04-13 20:56:09 +02:00

Rewrite some compositing nodes to using declarations #108728

This pull request is marked as a work in progress.

This branch is out-of-date with the base branch

View command line instructions.