Fix crash when extending taken-over named class #84148

KANAjetzt · 2023-10-29T16:11:41Z

Added error handling in _prepare_compilation() to address cases where the base_type cannot be found, preventing a crash.
Added an additional if statement to find_class() to check the script path.

I expect this to not be the ideal solution, as I stated in my issue comment.
But this PR might be a better place to discuss code changes, and I'm looking forward to any feedback.

Nonetheless, I ran all the GDScript tests and tried this fix in the MRP and two bigger projects with success and no obvious side effects.

Cheers.

Note

This now only contains the crash prevention. Future debugging is needed. #83542 will stay open.

adamscott · 2024-01-10T14:16:22Z

Thanks for your first PR!

If we merge this PR, I'm wondering if we should update the GDScriptCache when a path is taken over by a new script.

adamscott

Seems fine!

Thank you again for your first PR!

akien-mga · 2024-02-14T08:52:46Z

modules/gdscript/gdscript_compiler.cpp

@@ -2647,7 +2647,10 @@ Error GDScriptCompiler::_prepare_compilation(GDScript *p_script, const GDScriptP

 	GDScriptDataType base_type = _gdtype_from_datatype(p_class->base_type, p_script, false);

+	ERR_FAIL_COND_V_MSG(base_type.native_type == StringName(), ERR_BUG, vformat(R"(Failed to get base class for "%s")", p_script->path));


Should this be an engine-level error (ERR_BUG), or a user-level error (_set_error + returning the most relevant error code)?

I think it's fine, this should not happen as a result of user error, it's an analyzer/compiler bug.

dalexeev · 2024-02-14T10:05:16Z

modules/gdscript/gdscript.cpp

+	} else if (path.begins_with(get_root_script()->path)) {
+		class_names = path.trim_prefix(get_root_script()->path).split("::");
+		result = get_root_script();


I'm not sure, but the condition looks as always true to me. p_qualified_name is not used at all in the branch, this is weird.

Thanks for looking at this! I tried to explain what I found while debugging here.

If you can provide more details, I can dive back in.

"The condition looks as always true" - Which condition?
"p_qualified_name is not used" - What exactly do you mean by "not used" and what is weird about it?

Which condition?

path.begins_with(get_root_script()->path). path is the script path, for inner classes it may additionally contain suffixes. get_root_script() returns the topmost script that owns the inner class scripts.

What exactly do you mean by "not used" and what is weird about it?

This method checks p_qualified_name, every other branch has p_qualified_name, but not this one, that's weird. I haven't tested this, but it might break the _owner != nullptr branch.

I haven't tested this, but it might break the _owner != nullptr branch.

How can I test for this?
Adding some inner classes to my MRP?
I run --test --test-suite="*GDScript*" and everything seems to be fine.

Successful tests do not guarantee the absence of regressions, since the tests do not cover all possible scenarios. I added this PR for discussion at the GDScript team meeting or maybe @vnen will review it offline.

It is odd to me as well. This is a function to find a class identified by p_qualified_name, so the branch not checking the argument cannot reliably find the requested class. As @dalexeev said, this will always be true (the path of an inner class will always start with the path of its root script), so the other conditions will never be checked.

Given that, I don't think this is a proper solution.

vnen

See comment

pirey0 · 2024-05-05T14:55:32Z

@vnen i've been talking with Kana and further analyzed the root cause for this issue.
(see #83542 (comment) <- could use your input here)
This problem will not be fixed by changes to GDScript::find_class as suggested originally.

Should we close this PR, and address the root problem in a new PR?
(Or use this PR to merge only the suggested ERR_FAIL_COND_V_MSG?)
I feel like addressing the root problem here would only cause further confusion since the issue will go in a completely different direction

vnen · 2024-05-06T12:43:26Z

@pirey0 if the ERR_FAIL is enough to fix the crash, then we can merge it here. But yes, if you want to propose a new change it might be better to close this and open a new PR.

Added error handling in `_prepare_compilation()` to address cases where the `base_type` cannot be found, preventing a crash.

KANAjetzt · 2024-05-07T06:25:34Z

@vnen @pirey0 I changed it to only include the ERR_FAIL. That prevents the crash and logs these two errors:

I hope we can find a proper fix for this 👍

akien-mga · 2024-05-07T09:15:11Z

This seems good as crash prevention, but the bug still exsits. Do we have an open bug report with a MRP keeping track of it?

Suggested changes done.

pirey0 · 2024-05-07T09:17:59Z

This seems good as crash prevention, but the bug still exsits. Do we have an open bug report with a MRP keeping track of it?

#83542 (if this is not what you meant, let me know :) )

akien-mga · 2024-05-07T11:54:47Z

Thanks! And congrats for your first merged Godot contribution 🎉

KANAjetzt requested a review from a team as a code owner October 29, 2023 16:11

AThousandShips added bug topic:core crash topic:gdscript cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed topic:core labels Oct 29, 2023

AThousandShips added this to the 4.3 milestone Oct 29, 2023

AThousandShips added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 29, 2023

KANAjetzt mentioned this pull request Nov 24, 2023

Godot 4.x support with class_names GodotModding/godot-mod-loader#315

Open

adamscott requested review from adamscott, vnen and dalexeev January 10, 2024 14:16

adamscott approved these changes Jan 10, 2024

View reviewed changes

akien-mga reviewed Feb 14, 2024

View reviewed changes

dalexeev reviewed Feb 14, 2024

View reviewed changes

KANAjetzt force-pushed the fix_crash_take_over_path_named_class branch from 87db92f to ddea67e Compare March 18, 2024 08:56

vnen previously requested changes Apr 24, 2024

View reviewed changes

KANAjetzt force-pushed the fix_crash_take_over_path_named_class branch from ddea67e to fd6fcc6 Compare April 25, 2024 14:47

pirey0 mentioned this pull request May 5, 2024

take_over_path and extending the same named class twice #83542

Open

Fix crash when extending taken-over named class

f4192aa

Added error handling in `_prepare_compilation()` to address cases where the `base_type` cannot be found, preventing a crash.

KANAjetzt force-pushed the fix_crash_take_over_path_named_class branch from fd6fcc6 to f4192aa Compare May 7, 2024 06:07

dalexeev approved these changes May 7, 2024

View reviewed changes

akien-mga merged commit 0404e3a into godotengine:master May 7, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when extending taken-over named class #84148

Fix crash when extending taken-over named class #84148

KANAjetzt commented Oct 29, 2023 •

edited

Loading

adamscott commented Jan 10, 2024 •

edited

Loading

adamscott left a comment

akien-mga Feb 14, 2024

dalexeev Feb 14, 2024

dalexeev Feb 14, 2024

KANAjetzt Feb 14, 2024

dalexeev Feb 14, 2024

KANAjetzt Mar 18, 2024

dalexeev Apr 23, 2024

vnen Apr 24, 2024

vnen left a comment

pirey0 commented May 5, 2024 •

edited

Loading

vnen commented May 6, 2024

KANAjetzt commented May 7, 2024

akien-mga commented May 7, 2024

pirey0 commented May 7, 2024 •

edited

Loading

akien-mga commented May 7, 2024

		@@ -2647,7 +2647,10 @@ Error GDScriptCompiler::_prepare_compilation(GDScript *p_script, const GDScriptP

		GDScriptDataType base_type = _gdtype_from_datatype(p_class->base_type, p_script, false);

		ERR_FAIL_COND_V_MSG(base_type.native_type == StringName(), ERR_BUG, vformat(R"(Failed to get base class for "%s")", p_script->path));

Fix crash when extending taken-over named class #84148

Fix crash when extending taken-over named class #84148

Conversation

KANAjetzt commented Oct 29, 2023 • edited Loading

adamscott commented Jan 10, 2024 • edited Loading

adamscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnen left a comment

Choose a reason for hiding this comment

pirey0 commented May 5, 2024 • edited Loading

vnen commented May 6, 2024

KANAjetzt commented May 7, 2024

akien-mga commented May 7, 2024

pirey0 commented May 7, 2024 • edited Loading

akien-mga commented May 7, 2024

KANAjetzt commented Oct 29, 2023 •

edited

Loading

adamscott commented Jan 10, 2024 •

edited

Loading

pirey0 commented May 5, 2024 •

edited

Loading

pirey0 commented May 7, 2024 •

edited

Loading