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

Distinguish between dynamic library not found and can't be opened. #86682

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Distinguish between dynamic library not found and can't be opened. #86682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions core/extension/gdextension.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,10 +715,8 @@ Error GDExtension::open_library(const String &p_path, const String &p_entry_symb
#endif

Error err = OS::get_singleton()->open_dynamic_library(abs_path, library, true, &library_path);
if (err != OK) {
ERR_PRINT("GDExtension dynamic library not found: " + abs_path);
return err;
}
ERR_FAIL_COND_V_MSG(err == ERR_FILE_NOT_FOUND, err, "GDExtension dynamic library not found: " + abs_path);
ERR_FAIL_COND_V_MSG(err != OK, err, "Can't open GDExtension dynamic library: " + abs_path);

#if defined(WINDOWS_ENABLED) && defined(TOOLS_ENABLED)
// If we copied the file, let's change the library path to point at the original,
Expand Down
2 changes: 2 additions & 0 deletions drivers/unix/os_unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,8 @@ Error OS_Unix::open_dynamic_library(const String p_path, void *&p_library_handle
path = get_executable_path().get_base_dir().path_join("../lib").path_join(p_path.get_file());
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);

p_library_handle = dlopen(path.utf8().get_data(), GODOT_DLOPEN_MODE);
ERR_FAIL_NULL_V_MSG(p_library_handle, ERR_CANT_OPEN, vformat("Can't open dynamic library: %s. Error: %s.", p_path, dlerror()));

Expand Down
2 changes: 2 additions & 0 deletions platform/android/os_android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ Error OS_Android::open_dynamic_library(const String p_path, void *&p_library_han
so_file_exists = false;
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this breaks dynamic library loading on Android since on that platform the original path may be inaccessible which requires moving the library around (see logic below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting the change for Android in #86792

Copy link
Member

Choose a reason for hiding this comment

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

Actually I reviewed this a bit fast, this may also fail on other platforms. So the whole PR might need to be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eep, sorry, guys! I thought it would be fine

Copy link
Contributor

@dsnopek dsnopek Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like we could report a different error code if the file doesn't exist on Android too, it would just need to be handled later, I guess as an else on the 2nd if (internal_so_file_exists) check?

EDIT: No, that doesn't make sense. I'm actually a little confused as to why the current version causes problems. The copying branch only happens if so_file_exists is true, so it doesn't sound like missing that branch is what's causing the problem? Is there some case where loading a library should work even when so_file_exists is false?

EDIT 2: Ah, looking at this PR #68362 I think I understand this better

Copy link
Member

Choose a reason for hiding this comment

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

So my worry here is that this function is currently only used for GDExtension, OpenXR (for Android), and .NET (using absolute paths), so we might be forgetting about the fact that dynamic libraries can be in C:\Windows\System32 or /usr/lib//usr/lib64.

We don't use it currently (Linux does a bunch of dynamic loading but doesn't seem to rely on this method, it uses dlopen directly), but in theory I would expect this function to succeed if I do OS::get_singleton()->open_dynamic_library("libSDL2.so", sdl_handle); on Linux to load /usr/lib64/libSDL2.so on my system (with /usr/lib64 being part of my distro's default library paths).

While I feel we're treating it more as a open_gdextension_library currently, and only taking this use case into account.

Am I missing something?

CC @bruvzg

Copy link
Member

Choose a reason for hiding this comment

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

Actually looking further at the OS_Unix implementation I see it's not the generic wrapper around dlopen I thought it would be. It does seem focused only on trying to load a library next to the binary or in the parent ../lib folder if it exists.

So I guess the other checks added for Unix/Windows are ok. But there should likely be an explicit _MSG that refers back to the original p_path and not just the generic error that would mention the modified path.

Because currently trying to open mylib.so if it doesn't exist will fail with an error referring to ../lib/mylib.so.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, (for GDExtension and, .NET) it is OK to assume we are using full path, but for something like #85741 it won't be, also GDExtension can try using it to load its dependencies.

Maybe we should check if path is absolute path and only do this check (and path substitution). So open_dynamic_library("res://libSDL2.so") will check do it and open_dynamic_library("libSDL2.so") will skip the checks and let system look for the lib.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, this breaks dynamic library loading on Android since on that platform the original path may be inaccessible which requires moving the library around (see logic below).

For this, we probably should try substituting path for the expected one, like on macOS it's trying to replace executable folder with ../Frameworks/ if lib is not found.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like PR #86794? It could certainly be improved with a message, but I think it should give the correct error code when the file doesn't exist.


p_library_handle = dlopen(path.utf8().get_data(), RTLD_NOW);
if (!p_library_handle && so_file_exists) {
// The library may be on the sdcard and thus inaccessible. Try to copy it to the internal
Expand Down
2 changes: 2 additions & 0 deletions platform/ios/os_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,8 @@ void register_dynamic_symbol(char *name, void *address) {
path = get_framework_executable(get_executable_path().get_base_dir().path_join("Frameworks").path_join(p_path.get_file().get_basename() + ".framework"));
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);

p_library_handle = dlopen(path.utf8().get_data(), RTLD_NOW);
ERR_FAIL_NULL_V_MSG(p_library_handle, ERR_CANT_OPEN, vformat("Can't open dynamic library: %s. Error: %s.", p_path, dlerror()));

Expand Down
2 changes: 2 additions & 0 deletions platform/macos/os_macos.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@
path = get_framework_executable(get_executable_path().get_base_dir().path_join("../Frameworks").path_join(p_path.get_file()));
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);

p_library_handle = dlopen(path.utf8().get_data(), RTLD_NOW);
ERR_FAIL_NULL_V_MSG(p_library_handle, ERR_CANT_OPEN, vformat("Can't open dynamic library: %s. Error: %s.", p_path, dlerror()));

Expand Down
2 changes: 2 additions & 0 deletions platform/windows/os_windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,8 @@ Error OS_Windows::open_dynamic_library(const String p_path, void *&p_library_han
path = get_executable_path().get_base_dir().path_join(p_path.get_file());
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);

typedef DLL_DIRECTORY_COOKIE(WINAPI * PAddDllDirectory)(PCWSTR);
typedef BOOL(WINAPI * PRemoveDllDirectory)(DLL_DIRECTORY_COOKIE);

Expand Down
Loading