bevy_dynamic_plugin: Don't leak memory

[soqb]

Objective

• Currently bevy_dynamic_plugin::dynamically_load_plugin calls std::mem::forget on the libloading::Library instance. An escape hatch should be added to allow users to more easily handle the unloading of dynamic plugins.

Solution

• Add a DynamicPlugin struct which can be dropped manually.

• Add a new resource DynamicPluginLibraries which allows users to (unsafely) mark libraries for automatic unloading with DynamicPluginLibraries::mark_for_unloading. The library will be dropped when the associated DynamicPlugin is or when mark_for_unloading is called, whichever happens last.

---

Changelog

• Added DynamicPlugin.
• Added DynamicPluginLibraries.
• Changed the return type of dynamically_load_plugin to DynamicPlugin.
• Added bevy_ecs and bevy_utils dependencies for bevy_dynamic_plugin.

Migration Guide

• dynamically_load_plugin now returns a DynamicPlugin instead of a tuple. Call DynamicPlugin::into_raw_parts and Arc::try_unwrap on the resulting library if you need the old behavior.

[alice-i-cecile]

I like how much cleaner this is. And avoiding leaking memory could be useful for modding.

However, I'm definitely nervous about the long list of non-local invariants that must be upheld to avoid UB.

That's largely the story of this whole crate though, so my official stance on this PR is "neutral".

[soqb]

I'm definitely nervous about the long list of non-local invariants that must be upheld to avoid UB.

This is definitely an issue - as libloading states loading dynamic libraries is in theory always UB by definition but never in practice with a well-defined library that satisfies the relevant safety invariants.

The safety of the running an app with dynamic plugins relies on the safety calling dynamically_load_library. This PR fixes a memory leak (albeit intentional), but adds another safety invariant onto that function.

The safety notes on DynamicPlugin (ignoring that I now think they should be moved to dynamically_load_plugin for clarity and publicity) are unclear as they are now and could inadvertently cause UB. (e.g. An app defined in crate my_app loads a dynamic plugin defined in libdynamic.so which both depend on the my_dep crate. The dynamic plugin adds a resource defined in my_dep that contains a Box<dyn Fn()> and my_app retrieves it (because it's nameable through my_dep and may have the same TypeId if the same version of my_dep was built) and calls it after the app is dropped - inadvert UB but not a soundness hole.)

The only more complete solution i can conceive involves obfuscating dynamically-defined types and providing dynamic plugins with a more carefully controlled app scope.

I think for now I will change this PR to be leaky by default, with opt-in automatic dropping.

[soqb]

anyone know what this ci failure is about?

[alice-i-cecile]

Not your fault: there's duplicate dependencies in tree due to staggered updates. It won't block merging.

[inodentry]

Hmm. This is getting a little over my head now. I don't fully understand all the safety implications here. How is this intended to be used exactly?

If I'm understanding correctly, this PR adds a mechanism for unloading plugins. I cannot really imagine what that would look like in real user code.

[alice-i-cecile]

If this was used for modding, unloading plugins would be useful for disabling mods without a game restart.

[inodentry]

I understand the benefits of "unloading plugins" as a feature.

I meant that I don't understand how this particular API is to be used, and the safety implications of it.

[soqb]

I meant that I don't understand how this particular API is to be used, and the safety implications of it.

as it stands i'm not very happy with the api/implementation of this PR. i'm taking another look at the moment to see if i can come up with a better design.

[soqb]

I meant that I don't understand how this particular API is to be used, and the safety implications of it.

an example might help:

use bevy_dynamic_plugin::{DynamicPluginLibraries, DynamicPluginExt};
use bevy_app::App;
use bevy_ecs::system::ResMut;

const LIB_PATH: &str = "./libmy_dyn_plugin.so";

let mut app = App::new();
unsafe { app.load_plugin(LIB_PATH)
app.add_system(remove_library);

fn remove_library(mut libs: ResMut<DynamicPluginLibraries>) {
    // use arbitrary logic to decide whether to unload the library:
    if 1 != 2 {
        // SAFETY: No resources or components that contain function pointers remain after the app is dropped
        // and so code from the library cannot be called after the app is dropped.
        // Additionally, we have checked './libmy_dyn_plugin.so' and ensured it does not cause UB when unloaded.
        unsafe { libs.mark_for_unloading(LIB_PATH) };
        // library is still loaded at this point.
    }
}

app.run();

// library is no longer loaded.

[ItsDoot]

bevy_dynamic_plugin was deprecated in #13080 and subsequently removed in #14534.