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

Conversation

Projects
None yet
2 participants
@earthengine
Contributor

earthengine commented Sep 27, 2018

Discussed in the following post:

https://www.reddit.com/r/rust/comments/9hwr5r/a_rust_ffi_adventure_in_unsafety/

Conclusion:

  • There is no need to introduce lifetime for Capstone to ensure Instructions's lifetime constrainted.
  • The disasm methods might not actually need self to be mutable, as it only access immutable methods.
  • The disasm methods uses unbound lifetime, which is not desired here. Bound to self instead. However, as the semantic is eager evaluation, there is no need to also bound the code slice.
  • Tests fixed to represent that disasm is no longer mutating Capstone.

This change compiles all tests, but I didn't run the tests, please confirm with your own test before merge.

earthengine added some commits Sep 27, 2018

Remove mut keywords in tests and elide lifetime for code slice, to re…
…present the fact that this is a eager iterator
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 27, 2018

Codecov Report

Merging #49 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   93.85%   94.07%   +0.21%     
==========================================
  Files          15       15              
  Lines        2019     2094      +75     
==========================================
+ Hits         1895     1970      +75     
  Misses        124      124
Impacted Files Coverage Δ
src/arch/mod.rs 93% <100%> (ø) ⬆️
src/capstone.rs 96.38% <100%> (+1.85%) ⬆️
src/test.rs 96.27% <100%> (ø) ⬆️

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 53c1235...4a90528. Read the comment docs.

codecov bot commented Sep 27, 2018

Codecov Report

Merging #49 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   93.85%   94.07%   +0.21%     
==========================================
  Files          15       15              
  Lines        2019     2094      +75     
==========================================
+ Hits         1895     1970      +75     
  Misses        124      124
Impacted Files Coverage Δ
src/arch/mod.rs 93% <100%> (ø) ⬆️
src/capstone.rs 96.38% <100%> (+1.85%) ⬆️
src/test.rs 96.27% <100%> (ø) ⬆️

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 53c1235...4a90528. Read the comment docs.

@@ -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.

@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)
@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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@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).

@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).

@earthengine

It looks like that Capstone is also not UnwindSafe, because it contains *mut _, this is a good thing as this only prevents you to return a Capstone from catch_unwind, and this is the safety garantee that the underline handle can never being called simultaneously.

@@ -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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@@ -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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@@ -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.

@earthengine

earthengine Sep 27, 2018

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.

@earthengine

earthengine Sep 27, 2018

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.

@tmfink tmfink merged commit 4a4d1b8 into capstone-rust:master Oct 5, 2018

4 checks passed

codecov/patch 100% of diff hit (target 93.85%)
Details
codecov/project 94.07% (+0.21%) compared to 53c1235
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tmfink

This comment has been minimized.

Show comment
Hide comment
@tmfink

tmfink Oct 5, 2018

Member

I think that remove the mut requirement for &self from the disasm() methods is safe because Capstone is not Send/Sync/Clone, so no simultaneous access can occur.
I believe this should be safe to use even though disasm() can mutate the underlying Capstone handle.
Also, the Capstone C code cannot panic or throw an exception.

@earthengine thanks for your help!

Member

tmfink commented Oct 5, 2018

I think that remove the mut requirement for &self from the disasm() methods is safe because Capstone is not Send/Sync/Clone, so no simultaneous access can occur.
I believe this should be safe to use even though disasm() can mutate the underlying Capstone handle.
Also, the Capstone C code cannot panic or throw an exception.

@earthengine thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment