adding disasm_iter functionality #114
Conversation
capstone-rs/Cargo.toml
Outdated
| @@ -18,6 +18,9 @@ travis-ci = { repository = "capstone-rust/capstone-rs" } | |||
| capstone-sys = { path = "../capstone-sys", version = "0.14.0" } | |||
| libc = { version = "0.2", default-features = false } | |||
|
|
|||
| [dependencies.object] | |||
There was a problem hiding this comment.
I'd like to avoid bringing in this dependency. Is it used? Could you make it a "dev-dependency" instead?
| let cs = if let Ok(cs) = Capstone::new() | ||
| .x86() | ||
| .mode(arch::x86::ArchMode::Mode64) | ||
| .detail(true) |
There was a problem hiding this comment.
please run cargo fmt to format the code
capstone-rs/src/capstone.rs
Outdated
| @@ -165,6 +165,28 @@ impl Capstone { | |||
| } | |||
| } | |||
|
|
|||
| pub fn iter_malloc<'a>(&'a self) -> *mut cs_insn { | |||
| unsafe { | |||
| cs_malloc(self.csh()) | |||
There was a problem hiding this comment.
this allocates memory but never frees it; this is a memory leak
capstone-rs/examples/recursive.rs
Outdated
| let mut addr_queue: VecDeque<u64> = VecDeque::new(); | ||
| let mut addr_seen: HashMap<u64, bool> = HashMap::new(); | ||
|
|
||
| let cs_ins = cs.iter_malloc(); |
There was a problem hiding this comment.
I think this is undefined behavior (instantiating an Rust struct with initialized memory).
To work with uninitialized data, look into the MaybeUninit type.
There was a problem hiding this comment.
I will double check this. However the iter_malloc does allocate memory for 1 instruction, and that is a raw pointer:
*mut cs_insn.
I just used this fn: https://docs.rs/capstone-sys/0.5.0/capstone_sys/fn.cs_malloc.html
|
Thanks for the PR! What is the purpose of |
|
I didn't run into any issues. I needed to do recursive disassembly for obfuscated instructions. |
Sorry I wasn't clear with my question. What did this crate not allow you to do before that this new function |
|
Oh, recursive disassembly is good for use cases where data and code are mixed or when x86 instructions are obfuscated. |
There was a problem hiding this comment.
I'm happy to add an example program that does recursive disassembly, but I'm not inclined to accept the new disasm_iter() function in the current state since it allows users to violate memory safety.
One of the biggest advantages of Rust is the memory safety, so we want to make sure we preserve that.
You can see from #1 that I've previously tried to add support for cs_disasm_iter() unsuccessfully.
My recommendation is that you remove the disasm_iter() feature from this PR and just add the recursive disassembly example. If you want to in a follow-up PR, you can try to add disasm_iter() in a memory-safe way.
| } | ||
| } | ||
|
|
||
| pub fn disasm_iter<'a>( |
There was a problem hiding this comment.
What is the purpose of adding this function? Why are the existing disasm_all() or disasm_count() not sufficient?
This function dereferences raw pointers and is hence unsafe.
There was a problem hiding this comment.
I need to think more about what the disasm_iter() function interface should be.
Instead of a C-like interface, we want a "Rusty" interface that:
- Does not require the user to use unsafe
a. Does not take any pointers - Does not require manual malloc/free functions to avoid memory leaks, double frees, etc.
- Is easy to use
a. Does not return a tuple (an enum/struct instead)
There was a problem hiding this comment.
I agree. Let me close this PR. I can do a followup with these guidelines
capstone-rs/src/capstone.rs
Outdated
|
|
||
| pub fn disasm_iter<'a>( | ||
| &'a self, | ||
| code: *mut *const u8, |
There was a problem hiding this comment.
Avoid exposing raw pointers in the public API. We want to expose a memory-safe interface.
There was a problem hiding this comment.
Thank you for all your feedback. I really appreciate it.
I will take a look and see if I can clean even more the API. Feel free to decline this PR.
I needed the recursive disasm functionality.
I haven't done extensive testing although I've been using this code for some time now.