From 01a224425d2a90e660bbfffe8d1cbc6b93da24bd Mon Sep 17 00:00:00 2001 From: Mason Reed Date: Mon, 27 Jan 2025 16:56:15 -0500 Subject: 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 :/ --- rust/src/binary_view.rs | 16 +++++++++++++++- rust/src/collaboration/remote.rs | 8 ++++++++ rust/src/headless.rs | 40 ++++++++++++++++++++-------------------- rust/src/platform.rs | 4 ++-- rust/src/type_container.rs | 10 +++++----- 5 files changed, 50 insertions(+), 28 deletions(-) (limited to 'rust/src') diff --git a/rust/src/binary_view.rs b/rust/src/binary_view.rs index b508fa8b..43e913d5 100644 --- a/rust/src/binary_view.rs +++ b/rust/src/binary_view.rs @@ -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) -> 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) } } diff --git a/rust/src/collaboration/remote.rs b/rust/src/collaboration/remote.rs index 382d1ca2..21f148e9 100644 --- a/rust/src/collaboration/remote.rs +++ b/rust/src/collaboration/remote.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(()) diff --git a/rust/src/headless.rs b/rust/src/headless.rs index 2642b2b0..78c04a96 100644 --- a/rust/src/headless.rs +++ b/rust/src/headless.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() { diff --git a/rust/src/platform.rs b/rust/src/platform.rs index 62c61297..e732152c 100644 --- a/rust/src/platform.rs +++ b/rust/src/platform.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(&self, name: T) -> Array { diff --git a/rust/src/type_container.rs b/rust/src/type_container.rs index f5689acf..947c6912 100644 --- a/rust/src/type_container.rs +++ b/rust/src/type_container.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() } } -- cgit v1.3.1