Fix some rust race conditions and comment some likely suspects

FYI The binary view saving stuff can deadlock right now, so we document that now :/

Author: emesare

Cargo.lock

@@ -39,6 +39,10 @@ fn get_warp_tag_type(view: &BinaryView) -> Ref<TagType> {
 // TODO: Rename to markup_function or something.
 pub fn on_matched_function(function: &Function, matched: &WarpFunction) {
     let view = function.view();
+    // TODO: Using user symbols here is problematic
+    // TODO: For one they queue up a bunch of main thread actions
+    // TODO: Secondly by queueing up those main thread actions if you attempt to save the file
+    // TODO: Before the undo actions are done completing
     view.define_user_symbol(&to_bn_symbol_at_address(
         &view,
         &matched.symbol,

plugins/warp/src/plugin.rs

@@ -20,4 +20,5 @@ thiserror = "2.0"
 
 [dev-dependencies]
 rstest = "0.24"
-tempfile = "3.15"
+tempfile = "3.15"
+serial_test = "3.2"

rust/Cargo.toml

@@ -1580,7 +1580,7 @@ pub trait BinaryViewExt: BinaryViewBase {
         let type_container_ptr =
             NonNull::new(unsafe { BNGetAnalysisUserTypeContainer(self.as_ref().handle) });
         // NOTE: I have no idea how this isn't a UAF, see the note in `TypeContainer::from_raw`
-        unsafe { TypeContainer::from_raw(type_container_ptr.unwrap()) }
+        unsafe { TypeContainer::from_raw(type_container_ptr.unwrap()) }.clone()
     }
 
     /// Type container for auto types in the Binary View.
@@ -1878,12 +1878,26 @@ impl BinaryView {
     }
 
     /// Save the original binary file to the provided `file_path` along with any modifications.
+    ///
+    /// WARNING: Currently there is a possibility to deadlock if the analysis has queued up a main thread action
+    /// that tries to take the [`FileMetadata`] lock of the current view, and is executed while we
+    /// are executing in this function.
+    ///
+    /// To avoid the above issue use [`crate::main_thread::execute_on_main_thread_and_wait`] to verify there
+    /// are no queued up main thread actions.
     pub fn save_to_path(&self, file_path: impl AsRef<Path>) -> bool {
         let file = file_path.as_ref().into_bytes_with_nul();
         unsafe { BNSaveToFilename(self.handle, file.as_ptr() as *mut _) }
     }
 
     /// Save the original binary file to the provided [`FileAccessor`] along with any modifications.
+    ///
+    /// WARNING: Currently there is a possibility to deadlock if the analysis has queued up a main thread action
+    /// that tries to take the [`FileMetadata`] lock of the current view, and is executed while we
+    /// are executing in this function.
+    ///
+    /// To avoid the above issue use [`crate::main_thread::execute_on_main_thread_and_wait`] to verify there
+    /// are no queued up main thread actions.
     pub fn save_to_accessor(&self, file: &mut FileAccessor) -> bool {
         unsafe { BNSaveToFile(self.handle, &mut file.api_object) }
     }

rust/src/binary_view.rs

@@ -192,6 +192,10 @@ impl Remote {
     /// Connects to the Remote, loading metadata and optionally acquiring a token.
     ///
     /// Use [Remote::connect_with_opts] if you cannot otherwise automatically connect using enterprise.
+    /// 
+    /// WARNING: This is currently **not** thread safe, if you try and connect/disconnect to a remote on
+    /// multiple threads you will be subject to race conditions. To avoid this wrap the [`Remote`] in
+    /// a synchronization primitive, and pass that to your threads. Or don't try and connect on multiple threads.
     pub fn connect(&self) -> Result<(), ()> {
         // TODO: implement SecretsProvider
         if self.is_enterprise()? && enterprise::is_server_authenticated() {
@@ -234,6 +238,10 @@ impl Remote {
     }
 
     /// Disconnects from the remote.
+    /// 
+    /// WARNING: This is currently **not** thread safe, if you try and connect/disconnect to a remote on
+    /// multiple threads you will be subject to race conditions. To avoid this wrap the [`Remote`] in
+    /// a synchronization primitive, and pass that to your threads. Or don't try and connect on multiple threads.
     pub fn disconnect(&self) -> Result<(), ()> {
         let success = unsafe { BNRemoteDisconnect(self.handle.as_ptr()) };
         success.then_some(()).ok_or(())

rust/src/collaboration/remote.rs

@@ -148,26 +148,26 @@ impl Default for InitializationOptions {
 /// This initializes the core with the given [`InitializationOptions`].
 pub fn init_with_opts(options: InitializationOptions) -> Result<(), InitializationError> {
     // If we are the main thread that means there is no main thread, we should register a main thread handler.
-    if options.register_main_thread_handler
-        && is_main_thread()
-        && MAIN_THREAD_HANDLE.lock().unwrap().is_none()
-    {
-        let (sender, receiver) = std::sync::mpsc::channel();
-        let main_thread = HeadlessMainThreadSender::new(sender);
-
-        // This thread will act as our main thread.
-        let main_thread_handle = std::thread::Builder::new()
-            .name("HeadlessMainThread".to_string())
-            .spawn(move || {
-                // We must register the main thread within said thread.
-                main_thread.register();
-                while let Ok(action) = receiver.recv() {
-                    action.execute();
-                }
-            })?;
-
-        // Set the static MAIN_THREAD_HANDLER so that we can close the thread on shutdown.
-        *MAIN_THREAD_HANDLE.lock().unwrap() = Some(main_thread_handle);
+    if options.register_main_thread_handler && is_main_thread() {
+        let mut main_thread_handle = MAIN_THREAD_HANDLE.lock().unwrap();
+        if main_thread_handle.is_none() {
+            let (sender, receiver) = std::sync::mpsc::channel();
+            let main_thread = HeadlessMainThreadSender::new(sender);
+
+            // This thread will act as our main thread.
+            let join_handle = std::thread::Builder::new()
+                .name("HeadlessMainThread".to_string())
+                .spawn(move || {
+                    // We must register the main thread within said thread.
+                    main_thread.register();
+                    while let Ok(action) = receiver.recv() {
+                        action.execute();
+                    }
+                })?;
+
+            // Set the static MAIN_THREAD_HANDLER so that we can close the thread on shutdown.
+            *main_thread_handle = Some(join_handle);
+        }
     }
 
     match crate::product().as_str() {

rust/src/headless.rs

@@ -90,7 +90,7 @@ impl Platform {
             if res.is_null() {
                 None
             } else {
-                Some(Ref::new(Self { handle: res }))
+                Some(Self::ref_from_raw(res))
             }
         }
     }
@@ -176,7 +176,7 @@ impl Platform {
         // NOTE: I have no idea how this isn't a UAF, see the note in `TypeContainer::from_raw`
         // TODO: We are cloning here for platforms but we dont need to do this for [BinaryViewExt::type_container]
         // TODO: Why does this require that we, construct a TypeContainer, duplicate the type container, then drop the original.
-        unsafe { TypeContainer::from_raw(type_container_ptr.unwrap()).clone() }
+        unsafe { TypeContainer::from_raw(type_container_ptr.unwrap()) }
     }
 
     pub fn get_type_libraries_by_name<T: BnStrCompatible>(&self, name: T) -> Array<TypeLibrary> {

rust/src/platform.rs

@@ -36,11 +36,10 @@ impl TypeContainer {
         // NOTE: but this is how the C++ and Python bindings operate so i guess its fine?
         // TODO: I really dont get how some of the usage of the TypeContainer doesnt free the underlying container.
         // TODO: So for now we always duplicate the type container
-        // let cloned_ptr = NonNull::new(BNDuplicateTypeContainer(handle.as_ptr()));
-        // Self {
-        //     handle: cloned_ptr.unwrap(),
-        // }
-        Self { handle }
+        let cloned_ptr = NonNull::new(BNDuplicateTypeContainer(handle.as_ptr()));
+        Self {
+            handle: cloned_ptr.unwrap(),
+        }
     }
 
     /// Get an id string for the Type Container. This will be unique within a given
@@ -417,6 +416,7 @@ impl Debug for TypeContainer {
             .field("name", &self.name())
             .field("container_type", &self.container_type())
             .field("is_mutable", &self.is_mutable())
+            .field("type_names", &self.type_names().unwrap().to_vec())
             .finish()
     }
 }

rust/src/type_container.rs

@@ -3,6 +3,7 @@ use binaryninja::headless::Session;
 use binaryninja::symbol::{SymbolBuilder, SymbolType};
 use rstest::*;
 use std::path::PathBuf;
+use binaryninja::main_thread::execute_on_main_thread_and_wait;
 
 #[fixture]
 #[once]
@@ -31,6 +32,10 @@ fn test_binary_saving(_session: &Session) {
     // Verify that we modified the binary
     let modified_contents = view.read_vec(0x1560, 4);
     assert_eq!(modified_contents, [0xff, 0xff, 0xff, 0xff]);
+
+    // HACK: To prevent us from deadlocking in save_to_path we wait for all main thread actions to finish.
+    execute_on_main_thread_and_wait(|| {});
+
     // Save the modified file
     assert!(view.save_to_path(out_dir.join("atox.obj.new")));
     // Verify that the file exists and is modified.