From 9adf6a60b2496c7892615abc98d65b5ffd412d46 Mon Sep 17 00:00:00 2001 From: Mason Reed Date: Wed, 15 Jan 2025 00:00:04 -0500 Subject: [PATCH] More rust cleanup - Make workflow cloning explicit - Add workflow tests - Add missing property string list getting for settings - Remove IntoActivityName (see https://github.com/Vector35/binaryninja-api/pull/6257) --- plugins/warp/src/plugin/workflow.rs | 6 +- rust/README.md | 3 +- rust/examples/workflow.rs | 3 +- rust/src/settings.rs | 22 ++++ rust/src/workflow.rs | 149 +++++++++++++++------------- rust/tests/workflow.rs | 90 +++++++++++++++++ 6 files changed, 200 insertions(+), 73 deletions(-) create mode 100644 rust/tests/workflow.rs diff --git a/plugins/warp/src/plugin/workflow.rs b/plugins/warp/src/plugin/workflow.rs index bcd19f479..8906bdd82 100644 --- a/plugins/warp/src/plugin/workflow.rs +++ b/plugins/warp/src/plugin/workflow.rs @@ -78,7 +78,8 @@ pub fn insert_workflow() { } }; - let function_meta_workflow = Workflow::new_from_copy("core.function.metaAnalysis"); + let old_function_meta_workflow = Workflow::instance("core.function.metaAnalysis"); + let function_meta_workflow = old_function_meta_workflow.clone("core.function.metaAnalysis"); let guid_activity = Activity::new_with_action(GUID_ACTIVITY_CONFIG, guid_activity); function_meta_workflow .register_activity(&guid_activity) @@ -86,7 +87,8 @@ pub fn insert_workflow() { function_meta_workflow.insert("core.function.runFunctionRecognizers", [GUID_ACTIVITY_NAME]); function_meta_workflow.register().unwrap(); - let module_meta_workflow = Workflow::new_from_copy("core.module.metaAnalysis"); + let old_module_meta_workflow = Workflow::instance("core.module.metaAnalysis"); + let module_meta_workflow = old_module_meta_workflow.clone("core.module.metaAnalysis"); let matcher_activity = Activity::new_with_action(MATCHER_ACTIVITY_CONFIG, matcher_activity); module_meta_workflow .register_activity(&matcher_activity) diff --git a/rust/README.md b/rust/README.md index 2d075cbf1..727a820c5 100644 --- a/rust/README.md +++ b/rust/README.md @@ -34,7 +34,6 @@ fn main() { println!("{}:", func.symbol().full_name()); } } - ``` ## Getting Started @@ -61,7 +60,7 @@ binaryninjacore-sys = { git = "https://github.com/Vector35/binaryninja-api.git", ``` `build.rs`: -```rust +```doctestinjectablerust fn main() { let link_path = std::env::var_os("DEP_BINARYNINJACORE_PATH").expect("DEP_BINARYNINJACORE_PATH not specified"); diff --git a/rust/examples/workflow.rs b/rust/examples/workflow.rs index ec46a233c..a7695a095 100644 --- a/rust/examples/workflow.rs +++ b/rust/examples/workflow.rs @@ -50,7 +50,8 @@ pub fn main() { binaryninja::headless::Session::new().expect("Failed to initialize session"); println!("Registering workflow..."); - let meta_workflow = Workflow::new_from_copy("core.function.metaAnalysis"); + let old_meta_workflow = Workflow::instance("core.function.metaAnalysis"); + let meta_workflow = old_meta_workflow.clone("core.function.metaAnalysis"); let activity = Activity::new_with_action(RUST_ACTIVITY_CONFIG, example_activity); meta_workflow.register_activity(&activity).unwrap(); meta_workflow.insert("core.function.runFunctionRecognizers", [RUST_ACTIVITY_NAME]); diff --git a/rust/src/settings.rs b/rust/src/settings.rs index f37a5a348..2462de83d 100644 --- a/rust/src/settings.rs +++ b/rust/src/settings.rs @@ -215,6 +215,28 @@ impl Settings { } } + pub fn get_property_string_list( + &self, + key: S, + property: S, + ) -> Array { + let key = key.into_bytes_with_nul(); + let property = property.into_bytes_with_nul(); + let mut size: usize = 0; + unsafe { + Array::new( + BNSettingsQueryPropertyStringList( + self.handle, + key.as_ref().as_ptr() as *mut _, + property.as_ref().as_ptr() as *mut _, + &mut size, + ) as *mut *mut c_char, + size, + (), + ) + } + } + pub fn get_json( &self, key: S, diff --git a/rust/src/workflow.rs b/rust/src/workflow.rs index f8042ccc7..6c0a3d821 100644 --- a/rust/src/workflow.rs +++ b/rust/src/workflow.rs @@ -236,22 +236,6 @@ unsafe impl RefCountable for Activity { } } -pub trait IntoActivityName { - fn activity_name(self) -> BnString; -} - -impl IntoActivityName for &Activity { - fn activity_name(self) -> BnString { - self.name() - } -} - -impl IntoActivityName for S { - fn activity_name(self) -> BnString { - BnString::new(self) - } -} - // TODO: We need to hide the JSON here behind a sensible/typed API. #[repr(transparent)] pub struct Workflow { @@ -269,7 +253,7 @@ impl Workflow { /// Create a new unregistered [Workflow] with no activities. /// - /// To get a copy of an existing registered [Workflow] use [Workflow::new_from_copy]. + /// To get a copy of an existing registered [Workflow] use [Workflow::clone]. pub fn new(name: S) -> Self { let name = name.into_bytes_with_nul(); let result = unsafe { BNCreateWorkflow(name.as_ref().as_ptr() as *const c_char) }; @@ -280,8 +264,8 @@ impl Workflow { /// /// * `name` - the name for the new [Workflow] #[must_use] - pub fn new_from_copy(name: S) -> Workflow { - Self::new_from_copy_with_root(name, "") + pub fn clone(&self, name: S) -> Workflow { + self.clone_with_root(name, "") } /// Make a new unregistered [Workflow], copying all activities, within `root_activity`, and the execution strategy. @@ -289,19 +273,17 @@ impl Workflow { /// * `name` - the name for the new [Workflow] /// * `root_activity` - perform the clone operation with this activity as the root #[must_use] - pub fn new_from_copy_with_root( + pub fn clone_with_root( + &self, name: S, root_activity: A, ) -> Workflow { - let raw_name = name.clone().into_bytes_with_nul(); - let activity = root_activity.activity_name(); - // I can't think of a single reason as to why we should let users pass a workflow handle into this. - // To prevent warning being emitted we default to the name. - let placeholder_workflow = Workflow::instance(name); + let raw_name = name.into_bytes_with_nul(); + let activity = root_activity.into_bytes_with_nul(); unsafe { Self::from_raw( NonNull::new(BNWorkflowClone( - placeholder_workflow.handle.as_ptr(), + self.handle.as_ptr(), raw_name.as_ref().as_ptr() as *const c_char, activity.as_ref().as_ptr() as *const c_char, )) @@ -371,14 +353,16 @@ impl Workflow { ) -> Result where I: IntoIterator, - I::Item: IntoActivityName, + I::Item: BnStrCompatible, { - let subactivities_raw: Vec = subactivities + let subactivities_raw: Vec<_> = subactivities .into_iter() - .map(|x| x.activity_name()) + .map(|x| x.into_bytes_with_nul()) + .collect(); + let mut subactivities_ptr: Vec<*const _> = subactivities_raw + .iter() + .map(|x| x.as_ref().as_ptr() as *const c_char) .collect(); - let mut subactivities_ptr: Vec<*const _> = - subactivities_raw.iter().map(|x| x.as_ptr()).collect(); let result = unsafe { BNWorkflowRegisterActivity( self.handle.as_ptr(), @@ -392,17 +376,30 @@ impl Workflow { } /// Determine if an Activity exists in this [Workflow]. - pub fn contains(&self, activity: A) -> bool { - unsafe { BNWorkflowContains(self.handle.as_ptr(), activity.activity_name().as_ptr()) } + pub fn contains(&self, activity: A) -> bool { + unsafe { + BNWorkflowContains( + self.handle.as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + ) + } + } + + /// Retrieve the configuration as an adjacency list in JSON for the [Workflow]. + pub fn configuration(&self) -> BnString { + self.configuration_with_activity("") } /// Retrieve the configuration as an adjacency list in JSON for the - /// [Workflow], or if specified just for the given `activity`. + /// [Workflow], just for the given `activity`. /// - /// `activity` - if specified, return the configuration for the `activity` - pub fn configuration(&self, activity: A) -> BnString { + /// `activity` - return the configuration for the `activity` + pub fn configuration_with_activity(&self, activity: A) -> BnString { let result = unsafe { - BNWorkflowGetConfiguration(self.handle.as_ptr(), activity.activity_name().as_ptr()) + BNWorkflowGetConfiguration( + self.handle.as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + ) }; assert!(!result.is_null()); unsafe { BnString::from_raw(result) } @@ -433,12 +430,12 @@ impl Workflow { /// specified just for the given `activity`. /// /// * `activity` - if specified, return the roots for the `activity` - pub fn activity_roots(&self, activity: A) -> Array { + pub fn activity_roots(&self, activity: A) -> Array { let mut count = 0; let result = unsafe { BNWorkflowGetActivityRoots( self.handle.as_ptr(), - activity.activity_name().as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, &mut count, ) }; @@ -450,7 +447,7 @@ impl Workflow { /// /// * `activity` - if specified, return the direct children and optionally the descendants of the `activity` (includes `activity`) /// * `immediate` - whether to include only direct children of `activity` or all descendants - pub fn subactivities( + pub fn subactivities( &self, activity: A, immediate: bool, @@ -459,7 +456,7 @@ impl Workflow { let result = unsafe { BNWorkflowGetSubactivities( self.handle.as_ptr(), - activity.activity_name().as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, immediate, &mut count, ) @@ -474,20 +471,23 @@ impl Workflow { /// * `activities` - the list of Activities to assign pub fn assign_subactivities(&self, activity: A, activities: I) -> bool where - A: IntoActivityName, + A: BnStrCompatible, I: IntoIterator, - I::Item: IntoActivityName, + I::Item: BnStrCompatible, { - let mut input_list: Vec = - activities.into_iter().map(|a| a.activity_name()).collect(); - // SAFETY: this works because BnString and *mut ffi::c_char are - // transmutable - let input_list_ptr = input_list.as_mut_ptr() as *mut *const c_char; + let input_list: Vec<_> = activities + .into_iter() + .map(|a| a.into_bytes_with_nul()) + .collect(); + let mut input_list_ptr: Vec<*const _> = input_list + .iter() + .map(|x| x.as_ref().as_ptr() as *const c_char) + .collect(); unsafe { BNWorkflowAssignSubactivities( self.handle.as_ptr(), - activity.activity_name().as_ptr(), - input_list_ptr, + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + input_list_ptr.as_mut_ptr(), input_list.len(), ) } @@ -504,35 +504,43 @@ impl Workflow { /// * `activities` - the list of Activities to insert pub fn insert(&self, activity: A, activities: I) -> bool where - A: IntoActivityName, + A: BnStrCompatible, I: IntoIterator, - I::Item: IntoActivityName, + I::Item: BnStrCompatible, { - let mut input_list: Vec = - activities.into_iter().map(|a| a.activity_name()).collect(); - // SAFETY: this works because BnString and *mut ffi::c_char are - // transmutable - let input_list_ptr = input_list.as_mut_ptr() as *mut *const c_char; + let input_list: Vec<_> = activities + .into_iter() + .map(|a| a.into_bytes_with_nul()) + .collect(); + let mut input_list_ptr: Vec<*const _> = input_list + .iter() + .map(|x| x.as_ref().as_ptr() as *const c_char) + .collect(); unsafe { BNWorkflowInsert( self.handle.as_ptr(), - activity.activity_name().as_ptr(), - input_list_ptr, + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + input_list_ptr.as_mut_ptr(), input_list.len(), ) } } /// Remove the specified `activity` - pub fn remove(&self, activity: A) -> bool { - unsafe { BNWorkflowRemove(self.handle.as_ptr(), activity.activity_name().as_ptr()) } + pub fn remove(&self, activity: A) -> bool { + unsafe { + BNWorkflowRemove( + self.handle.as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + ) + } } /// Replace the specified `activity`. /// /// * `activity` - the Activity to replace /// * `new_activity` - the replacement Activity - pub fn replace( + pub fn replace( &self, activity: A, new_activity: N, @@ -540,8 +548,8 @@ impl Workflow { unsafe { BNWorkflowReplace( self.handle.as_ptr(), - activity.activity_name().as_ptr(), - new_activity.activity_name().as_ptr(), + activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, + new_activity.into_bytes_with_nul().as_ref().as_ptr() as *const c_char, ) } } @@ -550,15 +558,20 @@ impl Workflow { /// /// * `activity` - if specified, generate the Flowgraph using `activity` as the root /// * `sequential` - whether to generate a **Composite** or **Sequential** style graph - pub fn graph( + pub fn graph( &self, activity: A, sequential: Option, ) -> Option { let sequential = sequential.unwrap_or(false); - let activity_name = activity.activity_name(); - let graph = - unsafe { BNWorkflowGetGraph(self.handle.as_ptr(), activity_name.as_ptr(), sequential) }; + let activity_name = activity.into_bytes_with_nul(); + let graph = unsafe { + BNWorkflowGetGraph( + self.handle.as_ptr(), + activity_name.as_ref().as_ptr() as *const c_char, + sequential, + ) + }; if graph.is_null() { return None; } diff --git a/rust/tests/workflow.rs b/rust/tests/workflow.rs new file mode 100644 index 000000000..a234dcfa8 --- /dev/null +++ b/rust/tests/workflow.rs @@ -0,0 +1,90 @@ +use binaryninja::headless::Session; +use binaryninja::settings::Settings; +use binaryninja::workflow::Workflow; +use rstest::*; + +#[fixture] +#[once] +fn session() -> Session { + Session::new().expect("Failed to initialize session") +} + +#[rstest] +fn test_workflow_clone(_session: &Session) { + let original_workflow = Workflow::new("core.function.baseAnalysis"); + let mut cloned_workflow = original_workflow.clone("clone_workflow"); + + assert_eq!(cloned_workflow.name().as_str(), "clone_workflow"); + assert_eq!( + cloned_workflow.configuration(), + original_workflow.configuration() + ); + + cloned_workflow = original_workflow.clone(""); + assert_eq!( + cloned_workflow.name().as_str(), + "core.function.baseAnalysis" + ); +} + +#[rstest] +fn test_workflow_registration(_session: &Session) { + // Validate NULL workflows cannot be registered + let workflow = Workflow::new("null"); + assert_eq!(workflow.name().as_str(), "null"); + assert!(!workflow.registered()); + workflow + .register() + .expect_err("Re-registration of null is allowed"); + + // Validate new workflows can be registered + let test_workflow = Workflow::instance("core.function.baseAnalysis").clone("test_workflow"); + + assert_eq!(test_workflow.name().as_str(), "test_workflow"); + assert!(!test_workflow.registered()); + test_workflow + .register() + .expect("Failed to register workflow"); + assert!(test_workflow.registered()); + assert_eq!( + test_workflow.size(), + Workflow::instance("core.function.baseAnalysis").size() + ); + Workflow::list() + .iter() + .find(|w| w.name() == test_workflow.name()) + .expect("Workflow not found in list"); + Settings::new("") + .get_property_string_list("analysis.workflows.functionWorkflow", "enum") + .iter() + .find(|&w| w == "test_workflow") + .expect("Workflow not found in settings"); + + // Validate that registered workflows are immutable + let immutable_workflow = Workflow::instance("test_workflow"); + assert!(!immutable_workflow.clear()); + assert!(immutable_workflow.contains("core.function.advancedAnalysis")); + assert!(!immutable_workflow.remove("core.function.advancedAnalysis")); + assert!(!Workflow::instance("core.function.baseAnalysis").clear()); + + // Validate re-registration of baseAnalysis is not allowed + let base_workflow_clone = Workflow::instance("core.function.baseAnalysis").clone(""); + + assert_eq!( + base_workflow_clone.name().as_str(), + "core.function.baseAnalysis" + ); + assert!(!base_workflow_clone.registered()); + base_workflow_clone + .register() + .expect_err("Re-registration of baseAnalysis is allowed"); + assert_eq!( + base_workflow_clone.size(), + Workflow::instance("core.function.baseAnalysis").size() + ); + assert_eq!( + base_workflow_clone.configuration(), + Workflow::instance("core.function.baseAnalysis").configuration() + ); + assert!(!base_workflow_clone.registered()); +}