Added set_skipdata method

[mothran]

Added the new method set_skipdata() to the Capstone type. Capstone option: CS_OPT_SKIPDATA

I also added a simple test with two x86 instructions, one decoding to '.byte'.

[codecov[bot]]

Codecov Report

Merging #55 into master will increase coverage by 1.07%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #55      +/-   ##
=========================================
+ Coverage   93.82%   94.9%   +1.07%     
=========================================
  Files          15      15              
  Lines        1896    2455     +559     
=========================================
+ Hits         1779    2330     +551     
- Misses        117     125       +8

Impacted Files	Coverage Δ
src/capstone.rs	96.53% <100%> (+2.01%)	:arrow_up:
src/test.rs	97% <100%> (+0.72%)	: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 fab2a4a...baba964. Read the comment docs.

[tmfink]

Thanks for the PR! Sorry for the late response.

I like the idea, but I'm not comfortable merging the PR as-is:

• The only way for a user to check if bytes were returned is to check if the mnemonic is ".byte"
• A user could gets an instruction out, which would not make sense for a byte

Alternatives:

• Make the Capstone::disasm() family of functions return an enum that includes variants for instructions and bytes
  • This introduces complexity and run-time overhead having to check the variant for each instruction
• Add an Instruction::is_skipped_byte() -> CsResult<bool> method that allows users to check whether bytes were skipped without checking for ".byte"
  • A little bit janky, but does not negatively impact those who don't use skipdata.

Also, I'm not sure about the skipdata feature itself. You would use it on bytes where you don't know what is code or data. But, data bytes that happen to be a valid instruction could be mistakenly interpreted as code. With architectures like X86 that have variable length instruction, you could hit alignment issues where capstone could return an instruction that overlaps with code and data.

I'm open to suggestions.

[mothran]

I am more familiar with the raw C library for capstone but I was under the impression that upon a failed decomp during skipdata = True mode, the cs_insn->id would be *_INVALID.

I also suggested the feature because it is a useful mode for capstone when searching for things like ROP gadgets that require lots of exploration of dubious alignment instruction steams (for x86 at least).

I suggested the pull request because since this is a binding it would be nice to provide as much of the functionality of raw capstone as is possible.

[tmfink]

@mothran if you could add some tests (or add to your existing test) that the id is *_INVALID for skipped bytes, then I'll merge this PR.

[tmfink]

Looks like Criterion does not support Rust 1.24.1 anymore.

You can bump the minimum required version. See fab2a4a7e0f9ba6848be678028fd0a559231014e for reference.

[mothran]

Sure thing, I am not familiar with Criterion, where can I find its minimum supported version currently?

[tmfink]

I recommend you use rustup to install earlier touching to test before committing again.

https://github.com/bheisler/criterion.rs/blob/master/README.md#compatibility-policy

On Mon, Jan 28, 2019, 01:04 Parker Thompson <notifications@github.com wrote:

Sure thing, I am not familiar with Criterion, where can I find its minimum supported version currently?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/capstone-rust/capstone-rs/pull/55#issuecomment-458008342, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYuCUXgaIN10rvtYCHeKuk3D4AdFpvks5vHpLZgaJpZM4Z88oc .

[mothran]

Sorry for the delay but I have a minor update:

I attempted to replicate the problem with building Criterion but was unable to on my machine:

active toolchain
----------------

1.24.1-x86_64-unknown-linux-gnu (default)
rustc 1.24.1 (d3ae9a9e0 2018-02-27)

Using 1.24.1 I was able to run 'cargo test' and 'cargo bench' just fine. Is it possible there is something else I am missing to validate the error on my end?

[tmfink]

Using 1.24.1 I was able to run 'cargo test' and 'cargo bench' just fine. Is it possible there is something else I am missing to validate the error on my end?

Your Cargo.lock could be referencing an older version of Criterion. To replicate, try deleting your Cargo.lock the testing.

Another way to solve this problem could be updating Cargo.toml to using a "maximum" version constraint with <=. You should be able to say something like >= 0.2.0, <= 0.2.7. That way, we will not need to bump the minimum required Rust toolchain version.

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-requirements

Restricting versions could cause problems with dependency graphs normally, but Criterion is only a dev-dependency, so it should not cause dependency resolution errors in downstream users (since capstone-rs will be used as a normal dependency).

[tmfink]

replaced with #65