Repository navigation
Prefer OS-installed signed APKs over app-writable .so files when loading deferred libraries - #189145
Conversation
…omponent libs the native loader dlopens search paths from the end of the list first, but we were appending the app-writable getFilesDir .so paths last, so a loose .so in internal storage would win over the os-installed signed apk. flip the order so the signed apks are tried first and getFilesDir is only a fallback.
turns out the native loader iterating back to front is the actual bug: it contradicts the documented 'first to last' contract on FlutterJNI.loadDartDeferredLibrary. so fix it there (iterate front to back) rather than compensating in java, which fixes it for any custom manager too. revert PlayStoreDeferredComponentManager back to natural most-preferred-first order (bare name, signed apks, then getFilesDir fallback last). only changes behavior when multiple sources are loadable at once, so normal single-source installs and the getFilesDir-only download flow are unaffected.
pull the dlopen loop out of LoadDartDeferredLibrary into a named helper declared in the header, so the ordering is unit-testable without making the impl test-aware (no injected/fake dlopen). tests cover empty and no-match returning null, and -- using two distinct system libs as loadable stand-ins, so no build-time .so fixtures are needed -- that the earliest loadable path wins.
the system-lib version was fragile: platform ifdefs plus relying on libc/libm/libz being present and distinct on the runner. the property we actually care about is just the ordering (first-to-last, stop at first success), so add a testable overload of FindFirstLoadableLibrary that takes the loader as a param and assert order + short-circuit with a fake. production keeps the 1-arg signature and supplies dlopen. real load-path fidelity stays where it belongs, in deferred_components_test.
string/vector were already used transitively by the existing method sigs in this header; only functional (for std::function) is genuinely new.
drop the 1-arg overload; the sole caller in LoadDartDeferredLibrary passes the dlopen wrapper directly. one signature, loader injected, tests unchanged.
reverts the earlier "these come in transitively" call. the transitive includes do work, but include-what-you-use is the rule and the header names both types in its own declarations.
junit is (expected, actual), and every other test in the file follows that. also joins the signature back onto one line to match google-java-format.
the doc said it returns the first entry of search_paths, but it returns the handle for that entry.
platform_view_android_jni_impl.so files when loading deferred libraries
There was a problem hiding this comment.
Code Review
This pull request refactors the deferred component library loading logic to enforce and document search path priorities, ensuring that OS-installed signed APKs are preferred over loose .so files in app-writable internal storage for security. It introduces a helper function FindFirstLoadableLibrary in C++ to load libraries in descending priority order, and adds corresponding unit tests in both C++ and Java to verify this ordering behavior. There are no review comments, and I have no feedback to provide.
|
|
||
| // These paths are handed to the native loader in descending priority order: it tries them | ||
| // first to last and stops at the first that loads (see FlutterJNI.loadDartDeferredLibrary). | ||
| // Ordering matters for security -- OS-installed signed APKs must be preferred over loose .so |
There was a problem hiding this comment.
"Over loose .so"?
the phrase is used in many places in this pr but I am not sure the concept is well defined. Especially when reading this code outside of this pr.
There was a problem hiding this comment.
In this context I mean "Loose .so" as in sitting in the top level of the dir, as a standalone file, as opposed to say:
where it is inside an apk archive. I.e. "unbundled", or similar. But I can update it to say that more explicitly
There was a problem hiding this comment.
updated to have that wording
| doReturn(null).when(spyContext).getAssets(); | ||
|
|
||
| // A loose .so sitting in the app-writable internal storage dir (getFilesDir()). | ||
| String soTestPath = "test/path/libapp.so-123.part.so"; |
There was a problem hiding this comment.
for my own understanding why are non signed so files in app writable storage allowed as a load path at all?
There was a problem hiding this comment.
I don't know why they are, we could consider investigating/deprecating if not needed. It would be an independent change though
| } | ||
|
|
||
| @Test | ||
| public void searchPathsPrioritizeSignedApksOverInternalStorageSo() throws NameNotFoundException { |
There was a problem hiding this comment.
I find this test hard to read.
There are somethings that are odd like assertEquals(1, jni.loadDartDeferredLibraryCalled); why is something "called" being compared to a number.
There is also some string duplication around "libapp.so-123.part.so" which itself is kind of a hard to read/parse file to look for.
Overall I would ask if you could try to make this test easier to understand for someone debugging it later.
There was a problem hiding this comment.
Hmm I'd prefer to keep this test in line stylistically with the others in the file. jni.loadDartDeferredLibraryCalled is an integer invocation counter defined on the existing TestFlutterJNI mock in this test suite, which is already used in all the other tests. Similarly the other tests in this file, and most of the java tests in the embedder, follow the pattern of duplicating strings per test to make them self contained. I could change if you feel strongly, but I actually prefer it that way because of the different highlighting I get in IDE for strings vs variables which I find much more readable in a wall of asserts:
There was a problem hiding this comment.
ok I still find the test hard to read but this change is not the place to change the style of jni testing.
|
How do we need to communicate this change to our users. In theory there could be developers who will get different native code loaded. |
…prefer-signed-apks' into deferred-component-prefer-signed-apks
Yes, there could be. It would only break those who are assuming the opposite of the public contract though. I think we should add a note to the flutter release blog that we fixed a bug in loading order of native libraries when using deferred components |
I mean they are not assuming the opposite of the public contract. They are assuming the behavior as observed especially if there is a reason they have multiple so files and want to find one before another. That said this is a security problem and we are doing the opposite of our public contract. I just want to make sure that if we break users they have a way of finding out what changed. |
Yeah it's possible some are going based on observation. Regardless, I think we should add a note to our release, but as it's the opposite of the contract, and the contract behavior is more secure, I think we should land the fix |
|
I think the vast vast majority won't notice a difference though - in either case a |
LoadDartDeferredLibraryinplatform_view_android_jni_impl.ccwalked itssearch paths back to front (
search_paths.back()/pop_back()), which isthe opposite of the contract documented in three places:
FlutterJNI.loadDartDeferredLibrary: "Paths will be tried first to last andends when a library is successfully found."
DeferredComponentManager.loadDartLibrary: "Each search path will beattempted in order until a shared library is found."
PlayStoreDeferredComponentManagerreading// Add the bare filename as the first search path— which was in fact triedlast.
PlayStoreDeferredComponentManagerbuilds the list as[bare filename, signed split APKs..., loose .so files from getFilesDir()...],so reversing it meant the app-writable
getFilesDir()copies were consultedfirst and the OS-installed signed splits only as a fallback, which is exactly
backwards from the intended trust ordering.
This PR makes the native side honor the documented order. The Java-side list
construction is unchanged; only comments were added there to record why the
ordering matters.