Introduce an owned insn type

[clubby789]

As mentioned in #121 and #122, cloning an Insn followed by dropping the Instructions slice it belongs to causes a use-after-free. This PR makes two changes

• Adding no_copy to bindgen This prevents the cs_insn structure as being marked safe to Copy. I also removed the Clone implementation as it used the buggy Copy

• Introducing OwnedInsn I refactored the functionality of Insn into an Instruction trait (added to the prelude) and added a method to convert an &Insn into an OwnedInsn. This copies the data onto the Rust heap, and has a drop implementation that frees it from the Rust heap

Closes #121

[clubby789]

I think the tests are failing since I removed the Insn::from_raw. I don't think there's a safe way to create an Insn out of a *cs_insn that wouldn't end up replacing being incorrectly freed.

Should this be replaced with OwnedInsn::from_raw?

Edit: One way to make it safe would be to copy only the plain data portion of cs_insn, and leave the detail pointer as null.

[codecov[bot]]

Codecov Report

Merging #123 (6af4190) into master (01a966d) will increase coverage by 0.05%. The diff coverage is 93.10%.

:exclamation: Current head 6af4190 differs from pull request most recent head 6bb7994. Consider uploading reports for the commit 6bb7994 to get more accurate results

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   94.59%   94.65%   +0.05%     
==========================================
  Files          22       22              
  Lines        2666     2692      +26     
  Branches     2627     2653      +26     
==========================================
+ Hits         2522     2548      +26     
  Misses        144      144              

Impacted Files	Coverage Δ
capstone-rs/src/arch/evm.rs	92.30% <0.00%> (ø)
capstone-rs/src/error.rs	59.52% <ø> (ø)
capstone-rs/src/instruction.rs	93.91% <95.00%> (+0.09%)	:arrow_up:
capstone-rs/src/test.rs	97.61% <100.00%> (+0.01%)	:arrow_up:
capstone-rs/examples/demo.rs	94.23% <0.00%> (+1.92%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 01a966d...6bb7994. Read the comment docs.

[tmfink]

Thanks for the carefully constructed PR! It will take a little time to review this carefully.

[tmfink]

I think the tests are failing since I removed the Insn::from_raw. I don't think there's a safe way to create an Insn out of a *cs_insn that wouldn't end up replacing being incorrectly freed.

Should this be replaced with OwnedInsn::from_raw?

Edit: One way to make it safe would be to copy only the plain data portion of cs_insn, and leave the detail pointer as null.

There should be no Drop impl for Isns, so there should be no issue. The Instructions Drop impl frees the instructions normally. If you provide an "orphan" cs_insn to Insn::from_raw(), then you are responsible for freeing the backing allocation.

It would be good to add that to the existing from_raw() doc comment:

/// ...
/// The caller is fully responsible for the backing allocations lifetime, including freeing.

[clubby789]

I think I've made all the changes you asked for, as well as adding a new test. Split into individual commits for easier review, feel free to squash them if you merge 🙂

[clubby789]

I'm not planning to implement it in this PR, but in the future it might help to have an OwnedInstructions type (pub struct Instructions<'a>(&'a mut [OwnedInstruction]);). An Instructions could be converted to OwnedInstructions by copying the whole block of memory into one large box, then copying each detail pointer into its own box.

With the current implementation, each Insn is boxed individually, which means far more allocations for a similar amount of memory.

EDIT: This is implemented here.