Skip to content

Commit a68be26

Browse files
emesarezznop
emesare
authored and
zznop
committed
Refactor BinaryViewBase::save
- Save the raw file contents in the default impl of `BinaryViewBase::save` - Pass the view and file accessor into the function so its actually usable - Add a simple test - Mark view safe functions as unsafe as they cannot be called without upholding the main thread synchronization separately
1 parent c5c7c41 commit a68be26

3 files changed

Lines changed: 145 additions & 15 deletions

File tree

‎rust/src/binary_view.rs‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@ use crate::data_buffer::DataBuffer;
3232
use crate::debuginfo::DebugInfo;
3333
use crate::disassembly::DisassemblySettings;
3434
use crate::external_library::{ExternalLibrary, ExternalLocation};
35-
use crate::file_accessor::{Accessor, FileAccessor};
35+
use crate::file_accessor::{
36+
raw_mut as raw_file_accessor, Accessor, BorrowedFileAccessor, FileAccessor, FileAccessorHandle,
37+
};
3638
use crate::file_metadata::FileMetadata;
3739
use crate::flowgraph::FlowGraph;
3840
use crate::function::{Function, FunctionViewType, Location, NativeBlock};
@@ -586,9 +588,17 @@ pub trait BinaryViewBase {
586588

587589
fn address_size(&self) -> usize;
588590

589-
// TODO: Needs to take file accessor?
590-
fn save(&self) -> bool {
591-
false
591+
/// Save the view to `file`.
592+
///
593+
/// The default implementation saves the parent view, which typically surfaces as saving the
594+
/// raw contents of the file (via the "Raw" root view).
595+
fn save(&self, view: &BinaryView, file: &mut BorrowedFileAccessor<'_>) -> bool {
596+
view.parent_view().is_some_and(|parent| {
597+
// SAFETY: This callback is invoked synchronously from an outer save whose caller is responsible for
598+
// satisfying the save preconditions. Those preconditions remain valid for this nested parent save, which
599+
// must call into core directly rather than dispatching another main thread action from inside the callback.
600+
unsafe { parent.save_to_accessor(file) }
601+
})
592602
}
593603
}
594604

@@ -825,7 +835,7 @@ impl BinaryView {
825835
///
826836
/// To avoid the above issue, use [`crate::main_thread::execute_on_main_thread_and_wait`] to verify there
827837
/// are no queued up main thread actions.
828-
pub fn save_to_path(&self, file_path: impl AsRef<Path>) -> bool {
838+
pub unsafe fn save_to_path(&self, file_path: impl AsRef<Path>) -> bool {
829839
let file = file_path.as_ref().to_cstr();
830840
unsafe { BNSaveToFilename(self.handle, file.as_ptr() as *mut _) }
831841
}
@@ -838,8 +848,8 @@ impl BinaryView {
838848
///
839849
/// To avoid the above issue, use [`crate::main_thread::execute_on_main_thread_and_wait`] to verify there
840850
/// are no queued up main thread actions.
841-
pub fn save_to_accessor<A: Accessor>(&self, file: &mut FileAccessor<A>) -> bool {
842-
unsafe { BNSaveToFile(self.handle, &mut file.raw) }
851+
pub unsafe fn save_to_accessor<A: FileAccessorHandle + ?Sized>(&self, file: &mut A) -> bool {
852+
unsafe { BNSaveToFile(self.handle, raw_file_accessor(file)) }
843853
}
844854

845855
pub fn file(&self) -> Ref<FileMetadata> {
@@ -3674,14 +3684,17 @@ where
36743684
})
36753685
}
36763686

3677-
extern "C" fn cb_save<C>(ctxt: *mut c_void, _file: *mut BNFileAccessor) -> bool
3687+
extern "C" fn cb_save<C>(ctxt: *mut c_void, file: *mut BNFileAccessor) -> bool
36783688
where
36793689
C: CustomBinaryView,
36803690
{
36813691
ffi_wrap!("BinaryViewBase::save", unsafe {
36823692
let context = &*(ctxt as *mut CustomBinaryViewContext<C>);
3683-
// TODO: Need to pass file accessor to save to.
3684-
// let file = FileAccessor::from_raw(file);
3685-
context.view.save()
3693+
let mut file = BorrowedFileAccessor::from_raw(file);
3694+
// SAFETY: The core view has been initialized by [`BinaryView::from_custom`], and saving can
3695+
// only occur after the custom view has been created.
3696+
context
3697+
.view
3698+
.save(context.core_view.assume_init_ref(), &mut file)
36863699
})
36873700
}

‎rust/src/file_accessor.rs‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,3 +117,82 @@ impl<A: Accessor> Drop for FileAccessor<A> {
117117
}
118118
}
119119
}
120+
121+
/// A non-owning wrapper around a file accessor.
122+
///
123+
/// This is passed to callbacks, such as [`crate::binary_view::BinaryViewBase::save`], where the
124+
/// core retains ownership of the underlying accessor.
125+
///
126+
/// Use [`FileAccessor`] instead when you are the owner of the underlying accessor.
127+
pub struct BorrowedFileAccessor<'a> {
128+
pub(crate) raw: &'a mut BNFileAccessor,
129+
}
130+
131+
impl<'a> BorrowedFileAccessor<'a> {
132+
pub(crate) unsafe fn from_raw(raw: *mut BNFileAccessor) -> Self {
133+
debug_assert!(!raw.is_null());
134+
Self {
135+
raw: unsafe { &mut *raw },
136+
}
137+
}
138+
139+
pub fn read(&self, addr: u64, len: usize) -> Result<Vec<u8>, ErrorKind> {
140+
let cb_read = self.raw.read.unwrap();
141+
let mut buf = vec![0; len];
142+
let read_len = unsafe { cb_read(self.raw.context, buf.as_mut_ptr() as *mut _, addr, len) };
143+
if read_len != len {
144+
return Err(ErrorKind::UnexpectedEof);
145+
}
146+
Ok(buf)
147+
}
148+
149+
pub fn write(&self, addr: u64, data: &[u8]) -> usize {
150+
let cb_write = self.raw.write.unwrap();
151+
unsafe {
152+
cb_write(
153+
self.raw.context,
154+
addr,
155+
data.as_ptr() as *const _,
156+
data.len(),
157+
)
158+
}
159+
}
160+
161+
pub fn length(&self) -> u64 {
162+
let cb_get_length = self.raw.getLength.unwrap();
163+
unsafe { cb_get_length(self.raw.context) }
164+
}
165+
}
166+
167+
/// Used to provide access to the underlying file accessor.
168+
mod private {
169+
use binaryninjacore_sys::BNFileAccessor;
170+
171+
pub trait Sealed {
172+
fn raw_mut(&mut self) -> &mut BNFileAccessor;
173+
}
174+
}
175+
176+
/// A file accessor that can be passed to core functions like [`crate::binary_view::BinaryView::save_to_accessor`].
177+
///
178+
/// This is implemented by both [`FileAccessor`] and [`BorrowedFileAccessor`].
179+
#[allow(private_bounds)]
180+
pub trait FileAccessorHandle: private::Sealed {}
181+
182+
impl<T: private::Sealed + ?Sized> FileAccessorHandle for T {}
183+
184+
impl<A: Accessor> private::Sealed for FileAccessor<A> {
185+
fn raw_mut(&mut self) -> &mut BNFileAccessor {
186+
&mut self.raw
187+
}
188+
}
189+
190+
impl private::Sealed for BorrowedFileAccessor<'_> {
191+
fn raw_mut(&mut self) -> &mut BNFileAccessor {
192+
self.raw
193+
}
194+
}
195+
196+
pub(crate) fn raw_mut<T: FileAccessorHandle + ?Sized>(accessor: &mut T) -> &mut BNFileAccessor {
197+
private::Sealed::raw_mut(accessor)
198+
}

‎rust/tests/binary_view.rs‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use binaryninja::binary_view::{
44
CustomBinaryViewType, StringType,
55
};
66
use binaryninja::data_buffer::DataBuffer;
7+
use binaryninja::file_accessor::FileAccessor;
78
use binaryninja::file_metadata::{FileMetadata, SaveSettings};
89
use binaryninja::function::{Function, FunctionViewType};
910
use binaryninja::headless::Session;
@@ -14,6 +15,7 @@ use binaryninja::segment::SegmentBuilder;
1415
use binaryninja::symbol::{Symbol, SymbolBuilder, SymbolType};
1516
use binaryninja::Endianness;
1617
use std::collections::{BTreeMap, HashSet};
18+
use std::io::Cursor;
1719
use std::path::PathBuf;
1820

1921
#[test]
@@ -41,13 +43,16 @@ fn test_binary_saving() {
4143
let modified_contents = view.read_vec(contents_addr, 4);
4244
assert_eq!(modified_contents, [0xff, 0xff, 0xff, 0xff]);
4345

44-
// HACK: To prevent us from deadlocking in save_to_path, we wait for all main thread actions to finish.
45-
execute_on_main_thread_and_wait(|| {});
46-
4746
let temp_dir = tempfile::tempdir().expect("Failed to create temporary directory");
4847
let temp_path = temp_dir.path().join("atox.obj.new");
4948
// Save the modified file
50-
assert!(view.save_to_path(&temp_path));
49+
let save_view = view.clone();
50+
let save_path = temp_path.clone();
51+
execute_on_main_thread_and_wait(move || {
52+
// SAFETY: Running the save on the main thread ensures any previously queued main thread actions have
53+
// completed and no main thread action can run concurrently with the save.
54+
assert!(unsafe { save_view.save_to_path(&save_path) });
55+
});
5156
// Verify that the file exists and is modified.
5257
let new_view = binaryninja::load(temp_path).expect("Failed to load new view");
5358
assert_eq!(
@@ -284,4 +289,37 @@ fn test_custom_view() {
284289
vec![0x42, 0x42, 0x42, 0x42],
285290
"View not backed by the parent data"
286291
);
292+
293+
let temp_dir = tempfile::tempdir().expect("Failed to create temporary directory");
294+
let temp_path = temp_dir.path().join("custom_view.bin");
295+
let save_view = created_view.clone();
296+
let save_path = temp_path.clone();
297+
execute_on_main_thread_and_wait(move || {
298+
// SAFETY: Running the save on the main thread ensures any previously queued main thread actions have
299+
// completed and no main thread action can run concurrently with the save.
300+
assert!(
301+
unsafe { save_view.save_to_path(&save_path) },
302+
"Custom view did not save to path"
303+
);
304+
});
305+
assert_eq!(
306+
std::fs::read(temp_path).expect("Failed to read saved custom view"),
307+
[0x42, 0x42, 0x42, 0x42]
308+
);
309+
310+
// Verify that the custom view can be written to and the default save impl has handled it
311+
assert_eq!(created_view.write(1, &[0x10, 0x20]), 2);
312+
let save_view = created_view.clone();
313+
execute_on_main_thread_and_wait(move || {
314+
let mut saved_data = Cursor::new(Vec::new());
315+
let mut accessor = FileAccessor::new(&mut saved_data);
316+
// SAFETY: Running the save on the main thread ensures any previously queued main thread actions have
317+
// completed and no main thread action can run concurrently with the save.
318+
assert!(
319+
unsafe { save_view.save_to_accessor(&mut accessor) },
320+
"Custom view did not save to accessor"
321+
);
322+
drop(accessor);
323+
assert_eq!(saved_data.into_inner(), vec![0x42, 0x10, 0x20, 0x42]);
324+
});
287325
}

0 commit comments

Comments
 (0)