summaryrefslogtreecommitdiff
path: root/rust
diff options
context:
space:
mode:
Diffstat (limited to 'rust')
-rw-r--r--rust/Cargo.toml3
-rw-r--r--rust/src/binary_view.rs16
-rw-r--r--rust/src/collaboration/remote.rs8
-rw-r--r--rust/src/headless.rs36
-rw-r--r--rust/src/platform.rs4
-rw-r--r--rust/src/type_container.rs10
-rw-r--r--rust/tests/binary_view.rs5
-rw-r--r--rust/tests/collaboration.rs22
-rw-r--r--rust/tests/platform.rs1
9 files changed, 73 insertions, 32 deletions
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]