From 1b4fd27a6d82bb10f2ddaccf42dd72412682f434 Mon Sep 17 00:00:00 2001 From: Mason Reed Date: Wed, 6 Aug 2025 21:07:02 -0400 Subject: [PATCH] [WARP] Move symbol and comment application to when matched functions are inserted Instead of applying symbols and comments in the applier step, we will do it when the matched function is identified. This has the side effect that if you turn off the apply activity names and comments will still be applied, more work to be done later. --- plugins/warp/src/cache/function.rs | 27 ++++++++++++++++++++++++- plugins/warp/src/plugin/ffi/function.rs | 4 ++-- plugins/warp/src/plugin/workflow.rs | 17 +++------------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/plugins/warp/src/cache/function.rs b/plugins/warp/src/cache/function.rs index 2fef05fdab..a953f51453 100644 --- a/plugins/warp/src/cache/function.rs +++ b/plugins/warp/src/cache/function.rs @@ -1,17 +1,42 @@ +use crate::convert::{comment_to_bn_comment, to_bn_symbol_at_address}; +use binaryninja::binary_view::BinaryViewExt; use binaryninja::function::{Function as BNFunction, FunctionUpdateType}; +use binaryninja::symbol::SymbolType; use warp::signature::function::Function; -/// Inserts a function match into the cache. +// TODO: Rename this? +/// Inserts a function match into the cache. This also has the side effect of setting persisted function +/// information, such as the matched function symbol. /// /// IMPORTANT: This will mark the function as needing updates, if you intend to fill in functions with /// no match (i.e. `None`), then you must change this function to prevent marking that as needing updates. /// However, it's perfectly valid to remove a match and need to update the function still, so be careful. pub fn insert_cached_function_match(function: &BNFunction, matched_function: Option) { + let view = function.view(); + let function_start = function.start(); // NOTE: If we expect to run match_function multiple times on a function, we should move this elsewhere. // Mark the function as needing updates so that reanalysis occurs on the function, and we apply the match. function.mark_updates_required(FunctionUpdateType::FullAutoFunctionUpdate); + if let Some(auto_sym) = view.symbol_by_address(function_start) { + // TODO: If we ever create non library function symbols we will need to remove this check (see: `to_bn_symbol_at_address`). + if auto_sym.sym_type() == SymbolType::LibraryFunction { + // NOTE: This will also mark for full auto function update, one thing to note is that the + // requirement to call this is that this function not be called in the associated function's analysis. + view.undefine_auto_symbol(&auto_sym); + } + } match matched_function { Some(matched_function) => { + // Define the new matched function symbol, this can safely be done here as the symbol itself + // will be persisted, unlike function type information or variable information. + let new_sym = to_bn_symbol_at_address(&view, &matched_function.symbol, function_start); + view.define_auto_symbol(&new_sym); + // TODO: How to clear the comments? They are just persisted. + // TODO: Also they generate an undo action, i hate implicit undo actions so much. + for comment in &matched_function.comments { + let bn_comment = comment_to_bn_comment(&function, comment.clone()); + function.set_comment_at(bn_comment.addr, &bn_comment.comment); + } function.store_metadata("warp_matched_function", &matched_function.to_bytes(), false); } None => { diff --git a/plugins/warp/src/plugin/ffi/function.rs b/plugins/warp/src/plugin/ffi/function.rs index a7ec6593ac..7563bd969f 100644 --- a/plugins/warp/src/plugin/ffi/function.rs +++ b/plugins/warp/src/plugin/ffi/function.rs @@ -67,7 +67,7 @@ pub unsafe extern "C" fn BNWARPFunctionApply( let analysis_function = Function::from_raw(analysis_function); match function.is_null() { false => { - // Set the matched function to `function` and return previous. + // Set the matched function to `function`. let matched_function = ManuallyDrop::new(Arc::from_raw(function)); insert_cached_function_match( &analysis_function, @@ -75,7 +75,7 @@ pub unsafe extern "C" fn BNWARPFunctionApply( ) } true => { - // We are removing the previous match and returning it. + // We are removing the previous match. insert_cached_function_match(&analysis_function, None) } }; diff --git a/plugins/warp/src/plugin/workflow.rs b/plugins/warp/src/plugin/workflow.rs index 624fe0ef96..2a8f6287eb 100644 --- a/plugins/warp/src/plugin/workflow.rs +++ b/plugins/warp/src/plugin/workflow.rs @@ -3,9 +3,7 @@ use crate::cache::{ cached_function_guid, insert_cached_function_match, try_cached_function_guid, try_cached_function_match, }; -use crate::convert::{ - comment_to_bn_comment, platform_to_target, to_bn_symbol_at_address, to_bn_type, -}; +use crate::convert::{platform_to_target, to_bn_type}; use crate::matcher::{Matcher, MatcherSettings}; use crate::{get_warp_tag_type, relocatable_regions}; use binaryninja::architecture::RegisterId; @@ -192,16 +190,13 @@ pub fn run_matcher(view: &BinaryView) { } pub fn insert_workflow() { + // TODO: Note: because of symbol persistence function symbol is applied in `insert_cached_function_match`. + // TODO: Comments are also applied there, they are "user" like, persisted and make undo actions. // "Hey look, it's a plier" ~ Josh 2025 let apply_activity = |ctx: &AnalysisContext| { let view = ctx.view(); let function = ctx.function(); if let Some(matched_function) = try_cached_function_match(&function) { - view.define_auto_symbol(&to_bn_symbol_at_address( - &view, - &matched_function.symbol, - function.symbol().address(), - )); // core.function.propagateAnalysis will assign user type info to auto, so we must not apply // otherwise we will wipe over user type info. if !function.has_user_type() { @@ -209,12 +204,6 @@ pub fn insert_workflow() { function.set_auto_type(&to_bn_type(&function.arch(), func_ty)); } } - // TODO: How to clear the comments? They are just persisted. - // TODO: Also they generate an undo action, i hate implicit undo actions so much. - for comment in matched_function.comments { - let bn_comment = comment_to_bn_comment(&function, comment); - function.set_comment_at(bn_comment.addr, &bn_comment.comment); - } if let Some(mlil) = ctx.mlil_function() { for variable in matched_function.variables { let decl_addr = ((function.start() as i64) + variable.offset) as u64;