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

Fix Android dynamic library loading, while still distinguishing between not found and can't open #86794

Closed
wants to merge 1 commit into from
Closed

Fix Android dynamic library loading, while still distinguishing between not found and can't open #86794

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
3 changes: 1 addition & 2 deletions platform/android/os_android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,6 @@ 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);

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 Expand Up @@ -199,6 +197,7 @@ Error OS_Android::open_dynamic_library(const String p_path, void *&p_library_han
}
}

ERR_FAIL_COND_V(!p_library_handle && !so_file_exists, 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.

@dsnopek I think this check should be within the if block above if internal_so_file_exists is false.

The logic in this function is used to open shared libraries that are bundled with the apk / aab and shared libraries that are on the device but 'external' to the apk / aab binary.

In the first scenario, the path is irrelevant as (1) it doesn't match the location of the shared library embedded in the apk / aab (not sure if there's an actual usable path for it) and (2) the apk / aab is able to open and load the embedded shared library given only its name.
The loading can still fail however for example if the shared library doesn't match the device architecture (e.g: x86 library on arm device). With your current update, the function would return ERR_FILE_NOT_FOUND for that scenario.

In the second scenario, the 'external' path should point to the shared library, but Android security mechanisms prevent from loading external shared libraries. We work around that by copying the shared library to an internal path before attempting to open and load it (this is how the Android editor loads and uses GDExtension plugins).
For that case, we could return the ERR_FILE_NOT_FOUND error if the copy attempt is unsuccessful (unless there's another error that matches better).

ps: feel free to add this comment to the function for documentation / clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this check should be within the if block above if internal_so_file_exists is false.

I don't think we could ever get to that if block if so_file_exists is false, because the outer most if block would only be entered if so_file_exists is true. That whole section will only run if the file does exist, whereas we only want to report ERR_FILE_NOT_FOUND if it doesn't.

Although, I may be misunderstanding, because this is quite confusing :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

No that sounds correct. It may just mean this error cannot be used for the Android platform:

  • If we enter the if block, it means that the file exists
  • If we don't enter the if block, it only tells us the file is not available externally from the apk. The file could exist and be embedded within the apk, or could be missing from the apk as well..

The Android api (System.loadLibrary(...)) typically specifies whether the library is available but invalid, or missing.. any idea if dlopen provides such capabilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I figured if we got all the way past that block, and we failed to load it and initial file didn't exist... it probably doesn't exist? That's what the line I added is attempting to do. :-)

If that logic isn't sound, yeah, I guess maybe we can't use this error on Android, and your PR is the way to go!

The Android api (System.loadLibrary(...)) typically specifies whether the library is available but invalid, or missing.. any idea if dlopen provides such capabilities?

Not that I'm aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I figured if we got all the way past that block, and we failed to load it and initial file didn't exist... it probably doesn't exist? That's what the line I added is attempting to do. :-)

There is one edge case where the given path is not valid because the file is embedded in the apk, so the file exists, but the embedded version doesn't match the architecture the device is running on (arm file for an x86 device).
In that case, we'd get past that block, failed to load, and the so_file_exists would report the given path doesn't exist, but the failure would be because of the mismatched architecture.

If that logic isn't sound, yeah, I guess maybe we can't use this error on Android, and your PR is the way to go!

Yes, sounds like we should go with #86792 for the time-being, especially to avoid introducing a regression in the dev2 build.

Can you approve the other PR?

cc @akien-mga

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you approve the other PR?

Sure, I can do that.

ERR_FAIL_NULL_V_MSG(p_library_handle, ERR_CANT_OPEN, vformat("Can't open dynamic library: %s. Error: %s.", p_path, dlerror()));

if (r_resolved_path != nullptr) {
Expand Down
Loading