Skip to content

/capstone-rs

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lifetime of Capstone, and adjust self mutablity for disasm #49

Merged
merged 2 commits into from Oct 5, 2018
Merged

Remove lifetime of Capstone, and adjust self mutablity for disasm #49

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f16c110
Remove lifetime of Capstone, and make the disasm methods require only…
earthengine Sep 27, 2018
4a90528
Remove mut keywords in tests and elide lifetime for code slice, to re…
earthengine Sep 27, 2018
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+12 −15
Diff settings

Always

Just for now

Copy path View file
2 examples/objdump.rs
@@ -8,7 +8,7 @@ use std::io::Read;
use std::process;

fn main() {
let mut cs = Capstone::new()
let cs = Capstone::new()
.x86()
.mode(arch::x86::ArchMode::Mode64)
.build()
Copy path View file
4 src/arch/mod.rs
@@ -126,7 +126,7 @@ macro_rules! define_arch_builder {
self
}

fn build<'a>(self) -> CsResult<Capstone<'a>> {
fn build(self) -> CsResult<Capstone> {
let mode = match self.mode {
Some(mode) => mode,
None => {
@@ -288,7 +288,7 @@ pub trait BuildsCapstone<ArchMode> {
fn detail(self, enable_detail: bool) -> Self;

/// Get final `Capstone`
fn build<'a>(self) -> CsResult<Capstone<'a>>;
fn build<'a>(self) -> CsResult<Capstone>;
}

/// Implies that a `CapstoneBuilder` architecture has extra modes
Copy path View file
17 src/capstone.rs
@@ -12,7 +12,7 @@ use std::os::raw::{c_int, c_uint, c_void};

/// An instance of the capstone disassembler
#[derive(Debug)]
pub struct Capstone<'cs> {
pub struct Capstone {
/// Opaque handle to cs_engine
/// Stored as a pointer to ensure `Capstone` is `!Send`/`!Sync`
csh: *mut c_void,
@@ -39,8 +39,6 @@ pub struct Capstone<'cs> {

/// Architecture
arch: Arch,

_marker: PhantomData<&'cs mut c_void>,
}

/// Defines a setter on `Capstone` that speculatively changes the arch-specific mode (which
@@ -93,7 +91,7 @@ impl Iterator for EmptyExtraModeIter {
}
}

impl<'cs> Capstone<'cs> {
impl Capstone {
/// Create a new instance of the decompiler using the builder pattern interface.
/// This is the recommended interface to `Capstone`.
///
@@ -118,7 +116,7 @@ impl<'cs> Capstone<'cs> {
mode: Mode,
extra_mode: T,
endian: Option<Endian>,
) -> CsResult<Capstone<'cs>> {
) -> CsResult<Capstone> {
let mut handle: csh = 0;
let csarch: cs_arch = arch.into();
let csmode: cs_mode = mode.into();
@@ -146,7 +144,6 @@ impl<'cs> Capstone<'cs> {
detail_enabled,
raw_mode,
arch,
_marker: PhantomData,
};
cs.update_raw_mode();
Ok(cs)
@@ -157,7 +154,7 @@ impl<'cs> Capstone<'cs> {

/// Disassemble all instructions in buffer
pub fn disasm_all<'a>(
&mut self,

This comment has been minimized.

Sign in to view
Copy link
@tmfink

tmfink Sep 27, 2018

Member

I'm worried about removing the mut requirement for self on the disasm() methods since the cs_disasm() function mutates the underlying handle:

https://github.com/capstone-rust/capstone-sys/blob/master/capstone/cs.c#L646-L650

However, maybe this is safe to do since:

  • Capstone is not Send or Sync
  • Capstone is not Clone (so the handle should be unique to a given Capstone instance)

This comment has been minimized.

Sign in to view
Copy link
@earthengine

earthengine Sep 27, 2018

Author Contributor

I think this is like a UnsafeCell hidden behind FFI... So if we can prove that it is not possible to have two disasm call in progress simultaneously, we should be safe.

This comment has been minimized.

Sign in to view
Copy link
@earthengine

earthengine Sep 27, 2018

Author Contributor

On the other hand, if this turn out not safe, then the csh method will need to accept &mut self, as the semantic requires csh works like a *mut c_void.

This comment has been minimized.

Sign in to view
Copy link
@earthengine

earthengine Sep 27, 2018

Author Contributor

To ensure safety, we need to prove that is is not possible to perform two operation that mutate the underlying handle simultaneously.

If it is not safe, we need to change csh method to accept &mut self, because it returns something that semantically equal to *mut c_void.

This comment has been minimized.

Sign in to view
Copy link
@earthengine

earthengine Sep 27, 2018

Author Contributor

As Send or Sync is not an issue, unwinding is become the main focus as this is another way to run code simutaneously (i.e. mutation in one piece of code panic, and unwind calls another piece of mutating code).

So your unsafe advanture will arrive the next stop: unwind safety. The following is from Nomicon:

Rust's unwinding strategy is not specified to be fundamentally compatible with any other language's unwinding. As such, unwinding into Rust from another language, or unwinding into another language from Rust is Undefined Behavior. You must absolutely catch any panics at the FFI boundary! What you do at that point is up to you, but something must be done. If you fail to do this, at best, your application will crash and burn. At worst, your application won't crash and burn, and will proceed with completely clobbered state.

This comment has been minimized.

Sign in to view
Copy link
@earthengine

earthengine Sep 27, 2018

Author Contributor

So, unfortunately there is no easy solutions, you need to be ready to create a wrapper in C and making sure all exceptions are handled in the wrapper. Or, if you are sure the C function is already exception free, you can just trust it.

This comment has been minimized.

Sign in to view
Copy link
@tmfink

tmfink Sep 28, 2018

Member

In this library, the Rust code calls into the C code; the C code never calls into the Rust (or any other language).
From that point of view, we should also be safe (since C has no exceptions or stack unwinding).
&'a self,
code: &[u8],
addr: u64,
) -> CsResult<Instructions<'a>> {
@@ -166,7 +163,7 @@ impl<'cs> Capstone<'cs> {

/// Disassemble `count` instructions in `code`
pub fn disasm_count<'a>(
&mut self,
&'a self,
code: &[u8],
addr: u64,
count: usize,
@@ -180,7 +177,7 @@ impl<'cs> Capstone<'cs> {
/// Disassembles a `&[u8]` full of instructions.
///
/// Pass `count = 0` to disassemble all instructions in the buffer.
fn disasm<'a>(&mut self, code: &[u8], addr: u64, count: usize) -> CsResult<Instructions<'a>> {
fn disasm<'a>(&'a self, code: &[u8], addr: u64, count: usize) -> CsResult<Instructions<'a>> {
let mut ptr: *mut cs_insn = unsafe { mem::zeroed() };
let insn_count = unsafe {
cs_disasm(
@@ -395,7 +392,7 @@ impl<'cs> Capstone<'cs> {
}
}

impl<'cs> Drop for Capstone<'cs> {
impl Drop for Capstone {
fn drop(&mut self) {
unsafe { cs_close(&mut self.csh()) };
}
Copy path View file
4 src/test.rs
@@ -16,7 +16,7 @@ const IRET: cs_group_type::Type = cs_group_type::CS_GRP_IRET;
#[test]
fn test_x86_simple() {
match Capstone::new().x86().mode(x86::ArchMode::Mode64).build() {
Ok(mut cs) => match cs.disasm_all(X86_CODE, 0x1000) {
Ok(cs) => match cs.disasm_all(X86_CODE, 0x1000) {
Ok(insns) => {
assert_eq!(insns.len(), 2);
let is: Vec<_> = insns.iter().collect();
@@ -60,7 +60,7 @@ fn test_arm_simple() {

#[test]
fn test_arm64_none() {
let mut cs = Capstone::new()
let cs = Capstone::new()
.arm64()
.mode(arm64::ArchMode::Arm)
.build()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.