Fix more UB in pointer calls, and leaks in general #275

lilizoey · 2023-05-14T20:52:23Z

Fixes 3 issues, all done together because the test for the first one doesn't pass on latest godot without the other two fixes.

UB in virtual method calls

Fixes UB caused by us dereferencing a pointer incorrectly. We initially assumed that in pointer calls, if an argument is an object then the pointer passed is either a Ref<T>* or a T*, but as it turns out it sometimes is T** instead. This happens whenever we're making a virtual call (which are always pointer calls) and T does not inherit from RefCounted.

Thus when we tried to dereference the pointer we'd be transmuting a T* into a T. Which is UB.

closes #267

Improper handling of refcount in virtual methods

We were previously not incrementing the refcount for objects we received in virtual methods. This is incorrect, it is our responsibility to increment the refcount. This seemed correct at the time because of the bug below.

Unnecessary increment of recount on passing values to Godot

When passing a refcounted object to godot we would increment the refcount before passing it along. But godot would assume we did not do this, thus leading to a leak as godot would not decrement the refcount for us.

This made it appear like godot incremented the refcount for us in the tests we had. However it was actually just that we had incremented the refcount and godot did not decrement it.

closes #257

GodotRust · 2023-05-14T20:58:14Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-275

lilizoey · 2023-05-14T21:50:14Z

bors try

bors · 2023-05-14T21:57:18Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bromeon

Thanks for the update!

There are now quite a few more types for the GDScript tests: GDScriptExecutableTestCase, GDScriptHardcodedTestCase, GDScriptTestCase (the abstract base), TestSuiteSpecial. I assume this is all due to the "skipping a physics frame", or any other reason?

Is it possible to inherit TestSuiteSpecial from TestSuite for the sake of code reuse?

itest/rust/src/virtual_methods_test.rs

godot-core/src/obj/as_arg.rs

godot-core/src/obj/gd.rs

Bromeon · 2023-05-21T15:14:18Z

itest/godot/SpecialTests.gd

+	# Ensure we run a full physics frame
+	await root.get_tree().physics_frame
+	await root.get_tree().physics_frame


This runs two frames, no? If that's truly needed, could you add an explanation to that? At the moment the comments ("one full physics frame") seem to contradict the code.

from my understanding, it is like this:
the first physics frame signal is emitted at the first physics frame, but at this point no physics has happened yet. the second physics frame signal is emitted at the second physics frame, at this point a full physics frame has been run.
i may be wrong though, but it didn't seem to consistently work with only one at least.

i made the test runner wait for a physics frame instead of a process frame for initialization. that way i only need to wait for one physics frame here

lilizoey · 2023-05-21T15:34:03Z

Thanks for the update!

There are now quite a few more types for the GDScript tests: GDScriptExecutableTestCase, GDScriptHardcodedTestCase, GDScriptTestCase (the abstract base), TestSuiteSpecial. I assume this is all due to the "skipping a physics frame", or any other reason?

Yeah, it should also be usable if we have another test that we just cant run with our standard itest api. GDScriptExecutableTestCase is the same as the original GDScriptTestCase (as in it has the same code). It's just split into two now, the abstract base and the concrete class.

Is it possible to inherit TestSuiteSpecial from TestSuite for the sake of code reuse?

I'll try, probably, but i think i'd end up rewriting everything in TestSuiteSpecial anyway, since i need the asserts to write errors to a buffer with this implementation. Though maybe a bit more refactoring could avoid some of the code duplication.

Bromeon · 2023-05-21T15:52:08Z

Don't worry too much about that refactoring, can always be cleaned up later 🙂

lilizoey · 2023-05-21T17:46:07Z

Don't worry too much about that refactoring, can always be cleaned up later slightly_smiling_face

turned out to be pretty simple

Bromeon · 2023-05-24T18:50:46Z

bors try

bors · 2023-05-24T19:17:23Z

try

Build failed:

full-ci

Bromeon · 2023-05-24T19:22:48Z

@lilizoey it looks like this no longer passes memory checks with Godot upstream changes and/or #284 🤔

See failed CI job.

Undefined behavior

=================================================================
==2177==ERROR: AddressSanitizer: heap-use-after-free on address 0x61c0000076b8 at pc 0x5560a40a0327 bp 0x7ffc8d442c60 sp 0x7ffc8d442c50
READ of size 8 at 0x61c0000076b8 thread T0
    #0 0x5560a40a0326 in SelfList<GDScript>::self() const core/templates/self_list.h:135
    #1 0x5560a4046552 in GDScriptLanguage::finish() modules/gdscript/gdscript.cpp:2009
    #2 0x5560bcafc66a in ScriptServer::finish_languages() core/object/script_language.cpp:229
    #3 0x5560a24d3166 in Main::cleanup(bool) main/main.cpp:3538
    #4 0x5560a21fc3dd in main platform/linuxbsd/godot_linuxbsd.cpp:75
    #5 0x7f38a93f3082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #6 0x5560a21fbdad in _start (/home/runner/work/_temp/godot_bin/godot.linuxbsd.editor.dev.x86_64.san+0x3ab9ddad)

0x61c0000076b8 is located 1592 bytes inside of 1720-byte region [0x61c000007080,0x61c000007738)
freed by thread T0 here:
    #0 0x7f38aa1b340f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x5560bb29a104 in Memory::free_static(void*, bool) core/os/memory.cpp:168
    #2 0x5560a2514cb1 in void memdelete<GDScript>(GDScript*) core/os/memory.h:112
    #3 0x5560a250b045 in Ref<GDScript>::unref() core/object/ref_counted.h:210
    #4 0x5560a24fbac6 in Ref<GDScript>::~Ref() core/object/ref_counted.h:222
    #5 0x5560a40343a3 in GDScript::~GDScript() modules/gdscript/gdscript.cpp:1449
    #6 0x5560a2514ca0 in void memdelete<GDScript>(GDScript*) core/os/memory.h:109
    #7 0x5560a250b045 in Ref<GDScript>::unref() core/object/ref_counted.h:210
    #8 0x5560a24fbac6 in Ref<GDScript>::~Ref() core/object/ref_counted.h:222
    #9 0x5560a4046eb0 in GDScriptLanguage::finish() modules/gdscript/gdscript.cpp:2009
    #10 0x5560bcafc66a in ScriptServer::finish_languages() core/object/script_language.cpp:229
    #11 0x5560a24d3166 in Main::cleanup(bool) main/main.cpp:3538
    #12 0x5560a21fc3dd in main platform/linuxbsd/godot_linuxbsd.cpp:75
    #13 0x7f38a93f3082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

previously allocated by thread T0 here:
    #0 0x7f38aa1b3808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x5560bb2990a9 in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:75
    #2 0x5560bb298fba in operator new(unsigned long, char const*) core/os/memory.cpp:40
    #3 0x5560a40b5dfb in Ref<GDScript>::instantiate() core/object/ref_counted.h:216
    #4 0x5560a42800ed in GDScriptCache::get_shallow_script(String const&, Error&, String const&) modules/gdscript/gdscript_cache.cpp:254
    #5 0x5560a42ab0a8 in GDScriptCompiler::_gdtype_from_datatype(GDScriptParser::DataType const&, GDScript*) modules/gdscript/gdscript_compiler.cpp:127
    #6 0x5560a4326c0c in GDScriptCompiler::_populate_class_members(GDScript*, GDScriptParser::ClassNode const*, bool) modules/gdscript/gdscript_compiler.cpp:2434
    #7 0x5560a43362cd in GDScriptCompiler::compile(GDScriptParser const*, GDScript*, bool) modules/gdscript/gdscript_compiler.cpp:2832
    #8 0x5560a4016ea4 in GDScript::reload(bool) modules/gdscript/gdscript.cpp:783
    #9 0x5560a4281432 in GDScriptCache::get_full_script(String const&, Error&, String const&, bool) modules/gdscript/gdscript_cache.cpp:301
    #10 0x5560a4282065 in GDScriptCache::finish_compiling(String const&) modules/gdscript/gdscript_cache.cpp:333
    #11 0x5560a4336730 in GDScriptCompiler::compile(GDScriptParser const*, GDScript*, bool) modules/gdscript/gdscript_compiler.cpp:2847
    #12 0x5560a4016ea4 in GDScript::reload(bool) modules/gdscript/gdscript.cpp:783
    #13 0x5560a4281432 in GDScriptCache::get_full_script(String const&, Error&, String const&, bool) modules/gdscript/gdscript_cache.cpp:301
    #14 0x5560a405919e in ResourceFormatLoaderGDScript::load(String const&, String const&, Error*, bool, float*, ResourceFormatLoader::CacheMode) modules/gdscript/gdscript.cpp:2598
    #15 0x5560bbab7541 in ResourceLoader::_load(String const&, String const&, String const&, ResourceFormatLoader::CacheMode, Error*, bool, float*) core/io/resource_loader.cpp:260
    #16 0x5560bbab8f71 in ResourceLoader::_thread_load_function(void*) core/io/resource_loader.cpp:311
    #17 0x5560bbabc516 in ResourceLoader::_load_start(String const&, String const&, ResourceLoader::LoadThreadMode, ResourceFormatLoader::CacheMode) core/io/resource_loader.cpp:487
    #18 0x5560b5b5c604 in ResourceLoaderText::load() scene/resources/resource_format_text.cpp:466
    #19 0x5560b5b9a8cf in ResourceFormatLoaderText::load(String const&, String const&, Error*, bool, float*, ResourceFormatLoader::CacheMode) scene/resources/resource_format_text.cpp:1640
    #20 0x5560bbab7541 in ResourceLoader::_load(String const&, String const&, String const&, ResourceFormatLoader::CacheMode, Error*, bool, float*) core/io/resource_loader.cpp:260
    #21 0x5560bbab8f71 in ResourceLoader::_thread_load_function(void*) core/io/resource_loader.cpp:311
    #22 0x5560bbabc516 in ResourceLoader::_load_start(String const&, String const&, ResourceLoader::LoadThreadMode, ResourceFormatLoader::CacheMode) core/io/resource_loader.cpp:487
    #23 0x5560bbabb4ef in ResourceLoader::load(String const&, String const&, ResourceFormatLoader::CacheMode, Error*) core/io/resource_loader.cpp:404
    #24 0x5560a24c4de1 in Main::start() main/main.cpp:3183
    #25 0x5560a21fc236 in main platform/linuxbsd/godot_linuxbsd.cpp:71
    #26 0x7f38a93f3082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

SUMMARY: AddressSanitizer: heap-use-after-free core/templates/self_list.h:135 in SelfList<GDScript>::self() const
Shadow bytes around the buggy address:
  0x0c387fff8e80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c387fff8e90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c387fff8ea0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c387fff8eb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c387fff8ec0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c387fff8ed0: fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd
  0x0c387fff8ee0: fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c387fff8ef0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c387fff8f00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c387fff8f10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c387fff8f20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==2177==ABORTING

Fix incorrect incrementing of refcount when calling in to godot Fix refcount not being incremented when we receive a refcounted object in virtual methods

Bromeon · 2023-05-25T21:08:47Z

bors try

bors · 2023-05-25T21:16:22Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

lilizoey · 2023-05-25T21:17:47Z

bors r+

bors · 2023-05-25T21:26:22Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

lilizoey changed the title ~~Fix UB in virtual calls with non-refcounted object arguments~~ Fix more UB and leaks in pointer calls May 14, 2023

lilizoey changed the title ~~Fix more UB and leaks in pointer calls~~ Fix more UB in pointer calls, and leaks in general May 14, 2023

bors bot added a commit that referenced this pull request May 14, 2023

Try #275:

6a4ec1a

lilizoey mentioned this pull request May 17, 2023

Replace Deref/DerefMut with traits using supertraits for subtyping #278

Closed

lilizoey force-pushed the fix/virtual-call-object-ub branch 3 times, most recently from 485589c to e30d82c Compare May 18, 2023 15:22

lilizoey marked this pull request as ready for review May 21, 2023 15:04

lilizoey requested a review from Bromeon May 21, 2023 15:04

Bromeon reviewed May 21, 2023

View reviewed changes

lilizoey force-pushed the fix/virtual-call-object-ub branch from e30d82c to 3b0bff9 Compare May 21, 2023 17:44

lilizoey mentioned this pull request May 23, 2023

Regressions from PR #1044 and #1045 godotengine/godot-cpp#1119

Closed

bors bot added a commit that referenced this pull request May 24, 2023

Try #275:

29c62fa

lilizoey added 2 commits May 25, 2023 23:05

Fix UB in virtual method calls that take objects

8b52bfb

Fix incorrect incrementing of refcount when calling in to godot Fix refcount not being incremented when we receive a refcounted object in virtual methods

fix UB caused by preload weirdness

5b03781

lilizoey force-pushed the fix/virtual-call-object-ub branch from 3b0bff9 to 5b03781 Compare May 25, 2023 21:05

bors bot added a commit that referenced this pull request May 25, 2023

Try #275:

3a38054

Bromeon approved these changes May 25, 2023

View reviewed changes

bors bot merged commit 7dbd80f into godot-rust:master May 25, 2023

Bromeon added bug c: ffi Low-level components and interaction with GDExtension API labels May 25, 2023

Bromeon added the ub Undefined behavior label Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix more UB in pointer calls, and leaks in general #275

Fix more UB in pointer calls, and leaks in general #275

lilizoey commented May 14, 2023 •

edited

Loading

GodotRust commented May 14, 2023

lilizoey commented May 14, 2023

bors bot commented May 14, 2023

Bromeon left a comment

Bromeon May 21, 2023

lilizoey May 21, 2023

lilizoey May 21, 2023

lilizoey commented May 21, 2023

Bromeon commented May 21, 2023

lilizoey commented May 21, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

Bromeon commented May 24, 2023

Bromeon commented May 25, 2023

bors bot commented May 25, 2023

lilizoey commented May 25, 2023

bors bot commented May 25, 2023

Fix more UB in pointer calls, and leaks in general #275

Fix more UB in pointer calls, and leaks in general #275

Conversation

lilizoey commented May 14, 2023 • edited Loading

UB in virtual method calls

Improper handling of refcount in virtual methods

Unnecessary increment of recount on passing values to Godot

GodotRust commented May 14, 2023

lilizoey commented May 14, 2023

bors bot commented May 14, 2023

try

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon May 21, 2023

Choose a reason for hiding this comment

lilizoey May 21, 2023

Choose a reason for hiding this comment

lilizoey May 21, 2023

Choose a reason for hiding this comment

lilizoey commented May 21, 2023

Bromeon commented May 21, 2023

lilizoey commented May 21, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

try

Bromeon commented May 24, 2023

Bromeon commented May 25, 2023

bors bot commented May 25, 2023

try

lilizoey commented May 25, 2023

bors bot commented May 25, 2023

lilizoey commented May 14, 2023 •

edited

Loading