Building hooks that may conflict with embedder library dependencies

[simolus3]

I'm migrating package:sqlite3 to use hooks exclusively, and one issue I ran into are challenges loading SQLite with code assets when the app has already loaded SQLite before. I'm filing an issue here because the SQLite demo from this repository has the same issues, so I hope we can come up with possible solutions here.

Anyway, my hooks loading SQLite work just fine in dart test on Linux, but integrating them into a Flutter app causes crashes on Linux. This is reproducible with the SQLite example in this repository by:

• Adding bindings for sqlite3_initialize(), a supposedly harmless function that initializes the SQLite library if it hasn't already been initialized (typically one wouldn't call this directly, but it serves as a minimal repro and is called by other SQLite functions internally).
• Noticing that in unit tests, calling sqlite3_initialize() via @Native returns ok.
• Integrating the sqlite hook demo package into a Flutter app and calling sqlite3_initialize() there crashes the app because sqlite3 calls 0x0 as a function pointer.

You can find a setup ready to reproduce this on my flutter-sqlite-repro branch.

Note that when a Flutter app starts on Linux, the OS will already load libsqlite3.so from the system (on my machine, lddtree reveals libflutter_linux_gtk.so -> libgtk-3.so -> libtinysparql-3.0.so -> libsqlite3.so). So when Dart attempts to load the SQLite library provided by the hook, dlopen docs explain what's going on:

Symbol references in the shared object are resolved using (in order): symbols in the link map of objects loaded for the main program and its dependencies; symbols in shared objects (and their dependencies) that were previously opened with dlopen() using the RTLD_GLOBAL flag; and definitions in the shared object itself (and any dependencies that were loaded for that object).

So when we're loading SQLite, symbol references that have already been loaded (which will be nearly all of them) are pointing at the existing SQLite copy instead of the one we're trying to load. This then causes the new copy to not initialize properly and jump to a null pointer.

It looks like potential ways to fix this could be to

1. use RTLD_DEEPBIND which makes the loader prefer symbols in that shared object over symbols that have already been found.
2. (seems much more complicated) using dlmopen() to open the library in an independent namespace.
3. use static linking, which is what sqlite3_flutter_libs was doing on Linux before migrating to hooks.

Given the complexities of static linking, I wonder if the RTLD_DEEPBIND approach may be an option worth exploring here? Perhaps there could be an opt-in flag on DynamicLoadingBundled indicating that a hook would prefer an asset to get loaded with that flag?

[dcharkes]

Interesting, this is a conflict between the Dart package and the embedder.

(If it was a conflict between two packages, I'd have said this needs to be solved with making a shared package that both packages depend on. The SQLite3 should be API compatible with the newest versions. If Flutter would have been a Dart package instead of an embedder...)

1. use RTLD_DEEPBIND which makes the loader prefer symbols in that shared object over symbols that have already been found.

That already seems to be the default semantics for Android, iOS, MacOS, and Windows from what I can gather online. So it might make sense to simply align the semantics. (We also aligned some other semantics: https://dart-review.googlesource.com/c/sdk/+/260760. But we did not align semantics here: dart-lang/sdk#50105.)

One issue with this approach is that it might break things such as ASAN because you can no longer use LD_PRELOAD to override functions like malloc or free for that library. (Sanitizers are not supported yet, tracking in: #840)

2. using dlmopen() to open the library in an independent namespace.

This seems quite heavy weight indeed.

3. use static linking, which is what sqlite3_flutter_libs was doing on Linux before migrating to hooks.

Tracked in dart-lang/sdk#47718

Perhaps there could be an opt-in flag on DynamicLoadingBundled indicating that a hook would prefer an asset to get loaded with that flag?

Yes, if we want to make things configurable, doing it in the link mode is the right place.

Some other directions for solutions:

4. Tell embedders to not expose any symbols. Though, I'm not sure how feasible this is.

5. Let the SDK provide a list of dynamic libraries that are available in the process and if you see sqlite is already loaded by the embedder, use link mode LookupInProcess.

cc @goderbauer. And maybe @mraleph or @mkustermann have an opinion about what the right solution is here.

[simolus3]

Interesting, this is a conflict between the Dart package and the embedder.

A weird aspect of this is that the embedder doesn't even load / depend on libsqlite3 consciously, it just happens to be a transitive dependency somewhere. So this is even more platform-specific than it seems (maybe on some distros an important platform library has been changed to include additional items in its .dynamic section, which then causes more libraries to break if dlopened).

So I believe RTLD_DEEPBIND would be a better default, but I see that it needs to be customizable for asan and it would probably be too breaking to change it now.

The SQLite3 should be API compatible with the newest versions.

It's not just a question of API compatibility, but in case of SQLite also opt-in features/behaviors which depend on compilation options. It's not guaranteed all distributions use sensible defaults.

Yes, if we want to make things configurable, doing it in the link mode is the right place.

FWIW I would be happy to experiment with this / open PRs if that's the direction we want to go in. I'm not sure what an API would look like this since this is platform specific? Maybe something like this to allow a hook to specify a preference without giving a guarantee that the embedder will respect it:

abstract final class DynamicLoading extends LinkMode {
  // When set to true, pass RTLD_GLOBAL on platforms that support it.
  final bool? globalVisibilityPreference;
  // When set to true, pass RTLD_DEEPBIND on Linux. Ignored on other platforms.
  final bool? deepBindPreference;
  // When set to true, pass RTLD_FIRST on macOS. Ignored on other platforms.
  final bool? onlySearchInSelfPreference;

  DynamicLoading._({...}) : super._();
}

4. Tell embedders to not expose any symbols. Though, I'm not sure how feasible this is.

As in, tell embedders to load libraries for code assets in such a way that they don't see previously loaded symbols? Isn't that essentially the same as the namespaced loading?

5. Let the SDK provide a list of dynamic libraries that are available in the process and if you see sqlite is already loaded by the embedder, use link mode LookupInProcess.

Wouldn't I have to make the decision to use LookupInProcess in the build hook before I could possibly see which libraries are loaded? (I guess I could have two Dart files with equal @Native bindings, configure hooks to use process-lookups for one and bundled lookups for another and then use runtime checks to see which one I'd use. But that feels horrible).

If Flutter would have been a Dart package instead of an embedder...

[dcharkes]

So I believe RTLD_DEEPBIND would be a better default, but I see that it needs to be customizable for asan and it would probably be too breaking to change it now.

I don't believe it would be breaking at this point. I hope no-one has written any hooks that rely on libraries loaded by the embedder/SDK!

It's not just a question of API compatibility, but in case of SQLite also opt-in features/behaviors which depend on compilation options. It's not guaranteed all distributions use sensible defaults.

Ah right 👍 Yes, that rules out option 5 then. 😄

FWIW I would be happy to experiment with this / open PRs if that's the direction we want to go in. I'm not sure what an API would look like this since this is platform specific? Maybe something like this to allow a hook to specify a preference without giving a guarantee that the embedder will respect it:

abstract final class DynamicLoading extends LinkMode {
// When set to true, pass RTLD_GLOBAL on platforms that support it.
final bool? globalVisibilityPreference;
// When set to true, pass RTLD_DEEPBIND on Linux. Ignored on other platforms.
final bool? deepBindPreference;
// When set to true, pass RTLD_FIRST on macOS. Ignored on other platforms.
final bool? onlySearchInSelfPreference;

DynamicLoading.({...}) : super.();
}

Yeah, I was thinking something similar. We'd also need to pass those things into the Dart standalone and Flutter backend. Flutter uses an asset manifest json flutter/flutter#159322, and Dart standalone embeds the data inside a kernel snapshot. And we'd need to update the embedder API that Dart standalone and Flutter calls https://dart-review.googlesource.com/c/sdk/+/361881. So this is a non-trivial change. I think we can postpone it until we want to support ASAN.

If you want, you can make a PR for the Flutter engine to change the dlopen logic. Flutter engine implements this API in the Dart embedder API:

typedef struct {
  Dart_NativeAssetsDlopenCallback dlopen_absolute;
  Dart_NativeAssetsDlopenCallback dlopen_relative;
  Dart_NativeAssetsDlopenCallback dlopen_system;
  Dart_NativeAssetsDlopenCallbackNoPath dlopen_process;
  Dart_NativeAssetsDlopenCallbackNoPath dlopen_executable;
  Dart_NativeAssetsDlsymCallback dlsym;
  Dart_NativeAssetsDlopenAssetId dlopen;
  Dart_NativeAssetsAvailableAssets available_assets;
} NativeAssetsApi;

It's the dlopen callbacks that need to be changed. You can find it here: https://github.com/flutter/flutter/blob/673023b2e0a3e2a9902c1abe77afa28b13292e2c/engine/src/flutter/runtime/dart_isolate.cc#L1202 Or maybe that calls into a Dart helper lib again https://github.com/dart-lang/sdk/blob/b668fc8d3f3db7cd1439f5fd2bb58fb4a5cdcc27/runtime/bin/native_assets_api_impl.cc#L98

Do you know how to build the Flutter engine with a custom Dart?

[simolus3]

I've already started working on making this opt-in (also adding RTLD_GLOBAL as an option since you've mentioned a related issue), I should have a CL ready shortly 🫡

My approach would break the NativeAssetsApi ABI to add these flags though, I'm not sure how painful it is to deal with that.

So I can also just change the default to be RTLD_DEEPBIND too, but:

I don't believe it would be breaking at this point. I hope no-one has written any hooks that rely on libraries loaded by the embedder/SDK!

It would probably be quite rare, but I can see someone running into this. E.g. if I write a Flutter plugin for some Linux UI library using FFI and native assets, and I then specifically want to rely on say libgtk already being available.

So we could make RTLD_DEEPBIND the default and then allow disabling it, but another thing that just occurred to me is that RTLD_DEEPBIND is quite a niche option. So if we break something by making this the default, users will likely have a harder time figuring out what happened.

[dcharkes]

Is it breaking if we add a Dart_NativeAssetsDlopenAssetId dlopen2; to the struct? Then we can check which one is nullptr or not.

But I don't think we have to go through the trouble right now, I think we can shelf it until we start adding things such as ASAN. I presume with the sanitizers we will find more corners where we need to add support, so we can do a holistic design then.

It would probably be quite rare, but I can see someone running into this. E.g. if I write a Flutter plugin for some Linux UI library using FFI and native assets, and I then specifically want to rely on say libgtk already being available.

I don't think users should be relying on Flutter-specific libraries with build hooks and releasing it as a Dart standalone package! Also, it should still work right? It's just the order that is changing, not whether it's available at all.

So we could make RTLD_DEEPBIND the default and then allow disabling it, but another thing that just occurred to me is that RTLD_DEEPBIND is quite a niche option. So if we break something by making this the default, users will likely have a harder time figuring out what happened.

I think we should document this on the dartdoc of DynamicLoadingBundled and friends for which this applies.

I think until ASAN, we don't have a use case to not use deepbind, so we should keep the API simple for users.

[simolus3]

I've opened https://dart-review.googlesource.com/c/sdk/+/457620 to use RTLD_DEEPBIND when loading native assets, without making this configurable.

Since Flutter calls those methods, I think this doesn't need a Flutter PR and would just be covered by an SDK update into Flutter?

Do you know how to build the Flutter engine with a custom Dart?

This took a bit of trial and error, but I ended up symlinking my Dart checkout into flutter/engine/src/flutter/third_party/dart and that worked. Is there a better way to do that with gclient?

Anyway, I managed to get SQLite loaded with that CL in a Flutter application on Linux.

[dcharkes]

I've opened https://dart-review.googlesource.com/c/sdk/+/457620 to use RTLD_DEEPBIND when loading native assets, without making this configurable.

Hm, the sanitizer tests start failing immediately.

I wonder if we should consider option 4. cc-ing some Flutter folks who might have an opinion on Linux Desktop: @loic-sharma @knopp @stuartmorgan-g

Since Flutter calls those methods, I think this doesn't need a Flutter PR and would just be covered by an SDK update into Flutter?

👍

This took a bit of trial and error, but I ended up symlinking my Dart checkout into flutter/engine/src/flutter/third_party/dart and that worked. Is there a better way to do that with gclient?

Nope, I usually just ferry patches between that Dart checkout and another Dart checkout. 🙈

[loic-sharma]

@robert-ancell (who works on Flutter Linux at Canonical), what do you think of option 4? Here's a quick summary of the relevant bits of the conversation:

4. Tell embedders to not expose any symbols.
   ... when a Flutter app starts on Linux, the OS will already load libsqlite3.so from the system (on my machine, lddtree reveals libflutter_linux_gtk.so -> libgtk-3.so -> libtinysparql-3.0.so -> libsqlite3.so).

I'll bubble this conversation up in our sync with Canonical next week :)

[mraleph]

I think using RTLD_DEEPBIND is probably fine.

That being said: I think using multiple variants of SQLite library in one process can lead to DB corruption - if you are concurrently writing to the same DB using two different instances of the library the file locking will not work properly because it relies on static state. So you need to be rather careful about it.

[robert-ancell]

For multi-window we are relying on the GTK symbols being exposed as we access them via FFI to build windows - can we choose which symbols are exposed at a library level?

[simolus3]

For multi-window we are relying on the GTK symbols being exposed as we access them via FFI to build windows

Are you using LookupInProcess for that? My understanding is that if you dlopen a system library that has already been loaded into the process (either due to a prior dlopen or because of a normal dynamic-linking dependency), you'd get the existing loaded instance. So perhaps that FFI logic could use DynamicLoadingSystem('libgtk.so') to not depend on the embedder?