capstone-rust / capstone-rs

high-level Capstone system bindings for Rust
220 stars 78 forks source link

Implementing Clone for the Insn and cs_insn Structures #165

Closed c3rb3ru5d3d53c closed 1 week ago

c3rb3ru5d3d53c commented 1 month ago

Typically, when writing more complex code using capstone we can collect cs_insn structures on their own.

For example, we can create a list of cs_insn structures in Python without them going out of scope.

Currently this is not appearing to be possible in capstone-rs because the Insn and cs_insn structures only being accessed by reference when contained in Instructions iterator.

Most of the work on implementing the clone functionality has already been done for cs_detail structure and others.

This PR simply implements clone for both cs_insn and Insn structures, so it is possible to store these for later use in more complex projects.

Here is an example of using this functionality:

pub fn disassemble_instruction(&self, address: u64) -> Result<Insn, Error> {
        let instructions = match self.disassemble_instructions(address, 1) {
            Ok(instructions) => instructions,
            Err(error) => return Err(Error::new(ErrorKind::Other, "failed to disassemble instruction")),
        };
        let instruction: Insn<'_> = instructions.iter().next().unwrap().clone();
        return Ok(instruction);
    }

It also helps for when we need to iterate a single instruction at a time collecting them for later post-processing.

This becomes an issue when having to unpack from the Instructions iterator and only getting references without the possibility of cloning for later use.

This should not break any other features, it simply continues what was already done with cs_detail and other places clone has already been implemented in the capstone-rs project.

That being said, this is my first Rust-based PR, so proceed with caution.

Thank you :smile:

c3rb3ru5d3d53c commented 1 month ago

Nightly only appeared to have failed due to an unrelated cargo update segmentation fault, not sure why that is the case, all other tests appear to pass.

tmfink commented 1 week ago

@c3rb3ru5d3d53c thanks for your PR! Sorry I have not had time to review the PR (yet). Also, I allowed the rest of the tests to run--there are some tests failing.

tmfink commented 1 week ago

I think what you want is provided by the OwnedInsn type which avoids holding a reference.

You can see an example in the test_owned_insn() function: https://github.com/capstone-rust/capstone-rs/blob/62c7ec570451b5cc0cdf8761ebb0fb95b60f8a64/capstone-rs/src/test.rs#L3319-L3332

I'll close this PR for now, but feel free to comment on it if you think it should be re-opened.