Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GDScript: Fix segfault on invalid script #92035

Merged
merged 1 commit into from
May 19, 2024
Merged

GDScript: Fix segfault on invalid script #92035

merged 1 commit into from
May 19, 2024
+53 −1

Conversation

rune-scape
Copy link
Contributor

gdscript compiler sets script as not valid if GDScriptCache::finish_compiling() fails
various GDScript and GDScriptInstance functions now have checks to avoid calling any gdscipt function on an invalid gdscript instance, bc theres many ways to get a hold of an invalid gdscript and many places where the validity of a gdscript isnt checked
fixes #92021

@rune-scape rune-scape requested a review from a team as a code owner May 17, 2024 02:46
@dalexeev dalexeev added this to the 4.3 milestone May 17, 2024
@akien-mga
Copy link
Member

Haven't tested the MRP of the linked issue yet, but tested briefly on another project I had locally with a parse error, and it adds another IMO unnecessary error message to the pre-existing error spam:

ERROR: Condition "!valid" is true.
   at: _get_property_list (./modules/gdscript/gdscript.cpp:1032)
SCRIPT ERROR: Parse Error: Preload file "res://script_a.gd" does not exist.
          at: GDScript::reload (res://main.gd:3)
SCRIPT ERROR: Parse Error: Cannot infer the type of "ScriptA" constant because the value doesn't have a set type.
          at: GDScript::reload (res://main.gd:3)
ERROR: Failed to load script "res://main.gd" with error "Parse error".
   at: load (./modules/gdscript/gdscript.cpp:2941)

A non-descriptive error like Condition "!valid" is true. should only ever be shown to the user if the engine is broken, not if it encounters an invalid script during normal usage of the editor. The other parse errors are already descriptive enough about what the problem is so this new error is IMO unnecessary.

@akien-mga
Copy link
Member

Tested with the MRP from #92021, I confirm this fixes the segfault on start, but I notice there's a segfault on exit:

Thread 1 "New Game Projec" received signal SIGSEGV, Segmentation fault.
0x0000000006e81b78 in OS::is_stdout_verbose (this=0x0) at ./core/os/os.cpp:180
180             return _verbose_stdout;

(gdb) bt
#0  0x0000000006e81b78 in OS::is_stdout_verbose (this=0x0) at ./core/os/os.cpp:180
#1  0x000000000743eb9c in is_print_verbose_enabled () at ./core/string/print_string.cpp:199
#2  0x0000000004374931 in GDScript::~GDScript (this=0xa70a250, __in_chrg=<optimized out>) at ./modules/gdscript/gdscript.cpp:1613
#3  0x00000000041d67fe in memdelete<RefCounted> (p_class=0xa70a250) at ./core/os/memory.h:116
#4  0x00000000070d5787 in Variant::_clear_internal (this=0xc168e08) at ./core/variant/variant.cpp:1389
#5  0x00000000039ec69b in Variant::clear (this=0xc168e08) at ./core/variant/variant.h:309
#6  0x00000000039ec708 in Variant::~Variant (this=0xc168e08, __in_chrg=<optimized out>) at ./core/variant/variant.h:803
#7  0x00000000073f4e80 in Object::~Object (this=0xc168d90, __in_chrg=<optimized out>) at ./core/object/object.cpp:2153
#8  0x0000000003a6dd90 in RefCounted::~RefCounted (this=0xc168d90, __in_chrg=<optimized out>) at ./core/object/ref_counted.h:53
#9  0x0000000004191308 in ResourceFormatLoader::~ResourceFormatLoader (this=0xc168d90, __in_chrg=<optimized out>) at ./core/io/resource_loader.h:91
#10 0x000000000418ea8c in memdelete<ResourceFormatLoader> (p_class=0xc168d90) at ./core/os/memory.h:116
#11 0x000000000418e8dc in Ref<ResourceFormatLoader>::unref (this=0x7937b80 <ResourceLoader::loader>) at ./core/object/ref_counted.h:210
#12 0x000000000418e772 in Ref<ResourceFormatLoader>::~Ref (this=0x7937b80 <ResourceLoader::loader>, __in_chrg=<optimized out>) at ./core/object/ref_counted.h:223
#13 0x0000000006fc7a1d in __tcf_0 () at ./core/io/resource_loader.cpp:50
#14 0x00007ffff7d0dbb1 in __run_exit_handlers () from /lib64/libc.so.6
#15 0x00007ffff7d0dc7e in exit () from /lib64/libc.so.6
#16 0x00007ffff7cf508f in __libc_start_call_main () from /lib64/libc.so.6
#17 0x00007ffff7cf514b in __libc_start_main_impl () from /lib64/libc.so.6
#18 0x00000000039eb865 in _start ()

Additionally, the project doesn't work once exported, even though it works in the editor. This might be a separate issue from #92021 but there's still more work needed (here or in another PR), if a project works in the editor it ought to work once exported too.

@rune-scape
Copy link
Contributor Author

i see, ill take out the error print
the editors always segfaulted on exit for me .... i just assumed it happened so late it didnt affect anythign so nobody cared about it
and ya thats definitely a separate issue that may have triggered this issue and could actually be caused by #90601
ill look into it

its funny i actually partially made this fix a while ago for my own branch and just didnt have the motivation to make a PR until now
hopefully this fixes other weird crashes too

vnen
vnen approved these changes May 18, 2024
View reviewed changes
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akien-mga akien-mga merged commit c63383f into godotengine:master May 19, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@rune-scape rune-scape deleted the rune-gdscript-invalid branch May 19, 2024 20:13
@lyuma
Copy link
Contributor

lyuma commented May 30, 2024

This PR causes a regression in projects which access inner classes from `const foo: GDScript = preload("./foo.gd"):

MRP: GDScriptPreloadBug.zip

Steps to reproduce:

  1. Open the project. There are errors.
  2. Change test.gd from : GDScript = to := to infer type
  3. Errors go away. Uncheck and check the plugin and it should print Hello World
  4. Revert the change made in step 2.
  5. There are still no errors?!?!?!

This issue does not reproduce when this PR is backed out. The MRP works fine in Godot 4.0.3 through May 17 bd2300d.

@lyuma lyuma mentioned this pull request May 30, 2024
Closed
@dalexeev dalexeev mentioned this pull request May 30, 2024
Merged
@rune-scape rune-scape mentioned this pull request Jun 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnen vnen vnen approved these changes

Assignees
No one assigned
Labels
bug crash regression topic:gdscript
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Godot 4.3 Pull Request 90601 broke addon compatibility and segfaults
5 participants
@rune-scape @akien-mga @lyuma @vnen @dalexeev