Fix Resource Importer use after free

[alesliehughes]

Coverity reported issue.

If scene is a ImporterMeshInstance3D, it will be recreated which will leave the pointer (scene) in a bad state.

Testing with a single blend file, it appear not be this scenario. Only the child items appeared to be updated, so I'm not sure if this is the normal case for all import files.

These raise the questions

1. Will the first level scene ever be an ImporterMeshInstance3D object?
2. Does anyone have an example that would have ImporterMeshInstance3D as the scene. Or instructions to do it

Found code just down from my change,

err = OK;"
...
if (!scene) {
		 EditorNode::add_io_error(
				TTR("Error running post-import script:") + " " + post_import_script_path + "\n" +
				TTR("Did you return a Node-derived object in the `_post_import()` method?"));
		return err;
	}

Should this be returning an error of some sort?

[akien-mga]

I'm not familiar with this code, but the proposed solution seems unconventional. Both returning the pointer while keeping modifying it passed as argument in recursive calls seems like a recipe for disaster, or at least not knowing what one ends up with when reading that code.

[alesliehughes]

Sure. That makes sense. I'll give it another go.

Was just looking for some feedback as the right direction to take.

[fire]

@lyuma You may be interested in looking at this.

[lyuma]

Reproduction steps.

1. Make a new "MeshInstance3D" scene (or use "Convert To" on the root node to make it MeshInstance3D.
2. Export the glTF in Godot 4.2: Due to GLTF: Add root node export options and GODOT_single_root extension #81851 being merged and Add export settings to the export dialog for GLTF #79316 not being merged, export now always forces the GODOT_single_root extension which tells the importer to exclude the root node.
3. Godot will crash immediately after export due to the root node being an ImporterMeshInstance3D (this issue).

Reproduction glb model (rename to .glb)
CubeExported.glb.cpuprofile

[aaronfranke]

I investigated the code without looking at this PR, and I ended up coming to the same conclusion as the OP. We need to return a pointer to the new node, because we are replacing the node. This PR is exactly what I'd do to fix the problem.

@akien-mga: Both returning the pointer while keeping modifying it passed as argument in recursive calls seems like a recipe for disaster, or at least not knowing what one ends up with when reading that code.

Well, it's not passed in recursive calls. The recursive calls have p_node set to the previous call's p_node->get_child(i). Which are the same children as the original p_node's children, because we are using replace_by.

Once we run replace_by, we are freeing the old node, so there is no purpose to having a reference to the old p_node, so it makes perfect sense to me to overwrite that.

[akien-mga]

Thanks!