From 078dd061a7a34040b8e0e17fb20ccddd5971e164 Mon Sep 17 00:00:00 2001
From: BD103 <59022059+BD103@users.noreply.github.com>
Date: Mon, 12 Feb 2024 10:06:00 -0500
Subject: [PATCH] bevy_dynamic_plugin: fix `unsafe_op_in_unsafe_fn` lint
 (#11622)

# Objective

- Part of #11590.

## Solution

- Fix `unsafe_op_in_unsafe_fn` for `bevy_dynamic_plugin`.

---

## Changelog

- Added further restrictions to the safety requirements of
`bevy_dynamic_plugin::dynamically_load_plugin`.

---

I had a few issues, specifically with the safety comment on
`dynamically_load_plugin`. There are three different unsafe functions
called within the function body, and they all need their own
justification / message.

Also, would it be unsound to call `dynamically_load_plugin` multiple
times on the same file? I feel the documentation needs to be more clear.
---
 crates/bevy_dynamic_plugin/src/lib.rs    |  2 --
 crates/bevy_dynamic_plugin/src/loader.rs | 27 ++++++++++++++++++------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/crates/bevy_dynamic_plugin/src/lib.rs b/crates/bevy_dynamic_plugin/src/lib.rs
index 0a86b83b8a..961ae5a19f 100644
--- a/crates/bevy_dynamic_plugin/src/lib.rs
+++ b/crates/bevy_dynamic_plugin/src/lib.rs
@@ -1,5 +1,3 @@
-// FIXME(11590): remove this once the lint is fixed
-#![allow(unsafe_op_in_unsafe_fn)]
 // FIXME(3492): remove once docs are ready
 #![allow(missing_docs)]
 
diff --git a/crates/bevy_dynamic_plugin/src/loader.rs b/crates/bevy_dynamic_plugin/src/loader.rs
index 9e57daaced..656e377849 100644
--- a/crates/bevy_dynamic_plugin/src/loader.rs
+++ b/crates/bevy_dynamic_plugin/src/loader.rs
@@ -21,14 +21,28 @@ pub enum DynamicPluginLoadError {
 /// The specified plugin must be linked against the exact same libbevy.so as this program.
 /// In addition the `_bevy_create_plugin` symbol must not be manually created, but instead created
 /// by deriving `DynamicPlugin` on a unit struct implementing [`Plugin`].
+///
+/// Dynamically loading plugins is orchestrated through dynamic linking. When linking against foreign
+/// code, initialization routines may be run (as well as termination routines when the program exits).
+/// The caller of this function is responsible for ensuring these routines are sound. For more
+/// information, please see the safety section of [`libloading::Library::new`].
 pub unsafe fn dynamically_load_plugin<P: AsRef<OsStr>>(
     path: P,
 ) -> Result<(Library, Box<dyn Plugin>), DynamicPluginLoadError> {
-    let lib = Library::new(path).map_err(DynamicPluginLoadError::Library)?;
-    let func: Symbol<CreatePlugin> = lib
-        .get(b"_bevy_create_plugin")
-        .map_err(DynamicPluginLoadError::Plugin)?;
-    let plugin = Box::from_raw(func());
+    // SAFETY: Caller must follow the safety requirements of Library::new.
+    let lib = unsafe { Library::new(path).map_err(DynamicPluginLoadError::Library)? };
+
+    // SAFETY: Loaded plugins are not allowed to specify `_bevy_create_plugin` symbol manually, but must
+    // instead automatically generate it through `DynamicPlugin`.
+    let func: Symbol<CreatePlugin> = unsafe {
+        lib.get(b"_bevy_create_plugin")
+            .map_err(DynamicPluginLoadError::Plugin)?
+    };
+
+    // SAFETY: `func` is automatically generated and is guaranteed to return a pointer created using
+    // `Box::into_raw`.
+    let plugin = unsafe { Box::from_raw(func()) };
+
     Ok((lib, plugin))
 }
 
@@ -41,7 +55,8 @@ pub trait DynamicPluginExt {
 
 impl DynamicPluginExt for App {
     unsafe fn load_plugin<P: AsRef<OsStr>>(&mut self, path: P) -> &mut Self {
-        let (lib, plugin) = dynamically_load_plugin(path).unwrap();
+        // SAFETY: Follows the same safety requirements as `dynamically_load_plugin`.
+        let (lib, plugin) = unsafe { dynamically_load_plugin(path).unwrap() };
         std::mem::forget(lib); // Ensure that the library is not automatically unloaded
         plugin.build(self);
         self