2shady4u / godot-sqlite

GDExtension wrapper for SQLite (Godot 4.x+)
MIT License
895 stars 79 forks source link

[gd-extension] release version crashes exported projects #109

Closed Dfred closed 1 year ago

Dfred commented 1 year ago

Environment:

Issue description: When exporting projects, release versions that use godot-sqlite successfully load the shared library but crash with 'free(): invalid size' when calling open_db(). Same exact code works when exported as debug.

Steps to reproduce: create a new project & scene with addon/godot-sqlite compiled for your godot4 version. Copy&paste below code. Export project as debug - see it runs as expected, then export project as release. Run and see that last print is never displayed and a core has been dumped (if your system allows it).

Minimal reproduction project:

var db := SQLite.new()
db.path = "/home/user/work/database.sql"
print("opening")
print("opened" if db.open() else "failed to open")

Additional context tested with godot4-beta[1-6], confirmed issue with valgrind (which preserves a little bit further the stack):

==1948736== Invalid write of size 8                                                                                                                                                                                
==1948736==    at 0x1243C4FA: godot::String::utf8() const (in /home/user/work/addons/godot-sqlite/bin/libgdsqlite.linux.template_release.x86_64.so)           
==1948736==    by 0x1236A2AD: godot::SQLite::open_db() (in /home/user/work/addons/godot-sqlite/bin/libgdsqlite.linux.template_release.x86_64.so)

==1948736==  Address 0x291c6ad8 is 8 bytes before a block of size 60 alloc'd
==1948736==    at 0x4C4D899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1948736==    by 0x2ED92B8: ??? (in /home/user/work/Perso/godot-sqlite issues.x86_64)
==1948736==    by 0x1243C4EC: godot::String::utf8() const (in /home/user/work/addons/godot-sqlite/bin/libgdsqlite.linux.template_release.x86_64.so)
==1948736==    by 0x1236A2AD: godot::SQLite::open_db() (in /home/user/work/addons/godot-sqlite/bin/libgdsqlite.linux.template_release.x86_64.so)
2shady4u commented 1 year ago

Confirmed on Godot 4.0 Beta 7

I'll try to figure out if this is an actual godot-sqlite issue or if it is a generic godot-cpp issue instead.

Also is this related to #102 ?

Dfred commented 1 year ago

Also is this related to #102 ?

not to my knowledge but it could be.. #102 was specific to downloaded Windows artifacts, and this one also occurs when I compile the lib, so I can't say.

2shady4u commented 1 year ago

I've seemingly pinpointed the issue to the following line:

void SQLite::do_something(String some_string)
{
    const CharString dummy_path = some_string.utf8();
}

Calling this method works fine when exporting a debug build, but crashes when exporting to a release build.

This is also confirmed by the Valgrind crash dump.

2shady4u commented 1 year ago

There's already an issue describing this exact crash in the main godot repository here: https://github.com/godotengine/godot/issues/66846

We'll have to wait until that issue gets resolved before this issue is fixable 😢

Dfred commented 1 year ago

I've seemingly pinpointed the issue to the following line:

void SQLite::do_something(String some_string)
{
    const CharString dummy_path = some_string.utf8();
}

Calling this method works fine when exporting a debug build, but crashes when exporting to a release build.

This is also confirmed by the Valgrind crash dump.

Alright this makes a lot of sense :)

Good work pinning this down, let's hope this gets fixed soon. However I'm curious why it's still there as it is reported since Sep. and affects both 3.5.1 and 4-beta versions. Also, dropping here link to godot-cpp#842 that centralizes most related issues.

cridenour commented 1 year ago

Oh so this is why my release builds have been broken. I've just been assuming it was GDScript 2.0 parsing bugs still.

Applying https://github.com/godotengine/godot-cpp/compare/master...lightyears1998:godot-cpp:fix-heap-corruption to godot-cpp allows it to build. Unknown consequences for godot-cpp as a whole, but it does allow you to export your project in the mean time.

Dfred commented 1 year ago

thanks for the info Chris. Much appreciated.

Dfred commented 1 year ago

@2shady4u good news! godot-cpp/#842 has just been closed. It is quite likely that this issue will be closed soon too ;-)

2shady4u commented 1 year ago

@Dfred @cridenour I have recompiled and tested the demo project for Godot 4.0 rc3 and am pleased to announce that the release build isn't crashing immediately anymore! 😄

I have some new issue that I'll have to check which happens in both debug and release builds: image

But I think we can close this issue 🥳

2shady4u commented 1 year ago

I've been able to fix the issue above by removing the flush()-method call in the VFS and replacing it with a normal close()-call. Flushing a packaged file seems to return a non-crashing error in Godot 4.0

cridenour commented 1 year ago

I'll take full credit for lamenting about the issue in the Godot RocketChat :joy: