diff options
| -rw-r--r-- | Cargo.lock | 76 | ||||
| -rw-r--r-- | plugins/warp/src/plugin.rs | 4 | ||||
| -rw-r--r-- | rust/Cargo.toml | 3 | ||||
| -rw-r--r-- | rust/src/binary_view.rs | 16 | ||||
| -rw-r--r-- | rust/src/collaboration/remote.rs | 8 | ||||
| -rw-r--r-- | rust/src/headless.rs | 36 | ||||
| -rw-r--r-- | rust/src/platform.rs | 4 | ||||
| -rw-r--r-- | rust/src/type_container.rs | 10 | ||||
| -rw-r--r-- | rust/tests/binary_view.rs | 5 | ||||
| -rw-r--r-- | rust/tests/collaboration.rs | 22 | ||||
| -rw-r--r-- | rust/tests/platform.rs | 1 |
11 files changed, 153 insertions, 32 deletions
@@ -358,6 +358,7 @@ dependencies = [ "log", "rayon", "rstest", + "serial_test", "tempfile", "thiserror 2.0.11", ] @@ -1134,12 +1135,28 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a06f77d526c1a601b7c4cdd98f54b5eaabffc14d5f2f0296febdc7f357c6d3ba" [[package]] +name = "futures" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "65bc07b1a8bc7c85c5f2e110c476c7389b4554ba72af57d8445ea63a576b0876" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + +[[package]] name = "futures-channel" version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2dff15bf788c671c1934e366d07e30c1814a8ef514e1af724a602e8a2fbe1b10" dependencies = [ "futures-core", + "futures-sink", ] [[package]] @@ -1149,6 +1166,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "05f29059c0c2090612e8d742178b0580d2dc940c837851ad723096f87af6663e" [[package]] +name = "futures-executor" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e28d1d997f585e54aebc3f97d39e72338912123a67330d723fdbb564d646c9f" +dependencies = [ + "futures-core", + "futures-task", + "futures-util", +] + +[[package]] name = "futures-io" version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -1179,6 +1207,12 @@ dependencies = [ ] [[package]] +name = "futures-sink" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e575fab7d1e0dcb8d0c7bcf9a63ee213816ab51902e6d244a95819acacf1d4f7" + +[[package]] name = "futures-task" version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -1196,9 +1230,11 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9fa08315bb612088cc391249efdc3bc77536f16c91f6cf495e6fbe85b20a4a81" dependencies = [ + "futures-channel", "futures-core", "futures-io", "futures-macro", + "futures-sink", "futures-task", "memchr", "pin-project-lite", @@ -2457,6 +2493,15 @@ dependencies = [ ] [[package]] +name = "scc" +version = "2.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "28e1c91382686d21b5ac7959341fcb9780fa7c03773646995a87c950fa7be640" +dependencies = [ + "sdd", +] + +[[package]] name = "scoped-tls" version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -2495,6 +2540,12 @@ dependencies = [ ] [[package]] +name = "sdd" +version = "3.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "478f121bb72bbf63c52c93011ea1791dca40140dfe13f8336c4c5ac952c33aa9" + +[[package]] name = "semver" version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -2544,6 +2595,31 @@ dependencies = [ ] [[package]] +name = "serial_test" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b258109f244e1d6891bf1053a55d63a5cd4f8f4c30cf9a1280989f80e7a1fa9" +dependencies = [ + "futures", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] name = "sha1_smol" version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" diff --git a/plugins/warp/src/plugin.rs b/plugins/warp/src/plugin.rs index 86c4bc7b..b84173f7 100644 --- a/plugins/warp/src/plugin.rs +++ b/plugins/warp/src/plugin.rs @@ -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, diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 0a17730c..29170b6d 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -20,4 +20,5 @@ thiserror = "2.0" [dev-dependencies] rstest = "0.24" -tempfile = "3.15"
\ No newline at end of file +tempfile = "3.15" +serial_test = "3.2"
\ No newline at end of file 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<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) } } 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); + 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 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(); - } - })?; + // 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.lock().unwrap() = Some(main_thread_handle); + // 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<T: BnStrCompatible>(&self, name: T) -> Array<TypeLibrary> { 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() } } diff --git a/rust/tests/binary_view.rs b/rust/tests/binary_view.rs index 65f6b45b..0ee10d87 100644 --- a/rust/tests/binary_view.rs +++ b/rust/tests/binary_view.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. diff --git a/rust/tests/collaboration.rs b/rust/tests/collaboration.rs index 56b357e9..a8c0d52c 100644 --- a/rust/tests/collaboration.rs +++ b/rust/tests/collaboration.rs @@ -6,6 +6,7 @@ use binaryninja::headless::Session; use binaryninja::symbol::{SymbolBuilder, SymbolType}; use rstest::*; use std::path::PathBuf; +use serial_test::serial; #[fixture] #[once] @@ -14,13 +15,21 @@ fn session() -> Session { } // TODO: This cannot run in CI, as headless does not have collaboration, we should gate this. +// TODO: Why cant we create_project for the same project name? why does that fail. -fn temp_project_scope<T: Fn(&RemoteProject)>(remote: &Remote, cb: T) { +// TODO: Remote connection / disconnection is NOT thread safe, the core needs to lock on each. +// TODO: Because of this we run these tests serially, this isnt _really_ an issue for real code, as +// TODO: Real code shouldnt be trying to connect to the same remote on multiple threads. + +fn temp_project_scope<T: Fn(&RemoteProject)>(remote: &Remote, project_name: &str, cb: T) { if !remote.is_connected() { - remote.connect().expect("Failed to connect to remote"); + // TODO: Because connecting is not thread safe we wont check the error here, this is because we might already + // TODO: be connecting in some other thread and will error out on this thread. But we _probably_ will + // TODO: have connected by the time this errors out. Maybe? + let _ = remote.connect(); } let project = remote - .create_project("Test Project", "Test project for test purposes") + .create_project(project_name, "Test project for test purposes") .expect("Failed to create project"); project.open().expect("Failed to open project"); assert!(project.is_open(), "Project was not opened"); @@ -48,6 +57,7 @@ fn temp_project_scope<T: Fn(&RemoteProject)>(remote: &Remote, cb: T) { } #[rstest] +#[serial] fn test_connection(_session: &Session) { if !has_collaboration_support() { eprintln!("No collaboration support, skipping test..."); @@ -63,6 +73,7 @@ fn test_connection(_session: &Session) { } #[rstest] +#[serial] fn test_project_creation(_session: &Session) { if !has_collaboration_support() { eprintln!("No collaboration support, skipping test..."); @@ -70,7 +81,7 @@ fn test_project_creation(_session: &Session) { } let remotes = binaryninja::collaboration::known_remotes(); let remote = remotes.iter().next().expect("No known remotes!"); - temp_project_scope(&remote, |project| { + temp_project_scope(&remote, "test_creation", |project| { // Create the file than verify it by opening and checking contents. let created_file = project .create_file( @@ -143,6 +154,7 @@ fn test_project_creation(_session: &Session) { } #[rstest] +#[serial] fn test_project_sync(_session: &Session) { if !has_collaboration_support() { eprintln!("No collaboration support, skipping test..."); @@ -150,7 +162,7 @@ fn test_project_sync(_session: &Session) { } let remotes = binaryninja::collaboration::known_remotes(); let remote = remotes.iter().next().expect("No known remotes!"); - temp_project_scope(&remote, |project| { + temp_project_scope(&remote, "test_sync", |project| { // Open a view so that we can upload it. let out_dir = env!("OUT_DIR").parse::<PathBuf>().unwrap(); let view = binaryninja::load(out_dir.join("atox.obj")).expect("Failed to create view"); diff --git a/rust/tests/platform.rs b/rust/tests/platform.rs index c6c861fb..530f97b0 100644 --- a/rust/tests/platform.rs +++ b/rust/tests/platform.rs @@ -15,6 +15,7 @@ fn test_platform_lifetime(_session: &Session) { let platform_1 = Platform::by_name("windows-x86_64").expect("windows-x86_64 exists"); let platform_types_1 = platform_1.types(); assert_eq!(platform_types_0.len(), platform_types_1.len()); + assert_ne!(platform_types_1.len(), 0); } #[rstest] |