Remove UnicornInner in Rust bindings

[bet4it]

Fix #1619

[afgTheCat]

Hey I started using the new binds, and I have problems mapping memory within a hook. Example:

use unicorn_engine::unicorn_const::{Arch, HookType, MemType, Mode, Permission};
use unicorn_engine::Unicorn;

pub fn page_fault_hook<D>(
    emu: &mut Unicorn<D>,
    _mem_type: MemType,
    address: u64,
    _mem_size: usize,
    _value: i64,
) -> bool {
    let page_start = address & 0xfffffffffffff000;
    emu.mem_map(page_start, 0x1000, Permission::READ | Permission::WRITE)
        .unwrap();
    true
}

fn main() {
    let mut unicorn = Unicorn::new(Arch::X86, Mode::MODE_64).unwrap();
    unicorn
        .add_mem_hook(HookType::MEM_INVALID, 0, u64::MAX, page_fault_hook)
        .expect("Could not add mem hook");
}

The compiler will tell me that the D may not live long enough. I could not change the hook's signature so that I escape all the lifetime errors, however changing the Unicorn struct the following way seems to solve my issues:

pub struct Unicorn<'a, D: 'a> {
    handle: uc_handle,
    ffi: bool,
    arch: Arch,
    /// to keep ownership over the hook for this uc instance's lifetime
    hooks: Vec<(ffi::uc_hook, Box<dyn ffi::IsUcHook<'a> + 'a>)>,
    /// To keep ownership over the mmio callbacks for this uc instance's lifetime
    mmio_callbacks: Vec<MmioCallbackScope<'a>>,
    data: D,
}

Is there a reason why Unicorn's data can't be restricted to 'a?

[bet4it]

I think you are right! Fixed.

[domenukk]

Hmm, aliasing, and later accessing, a &mut Unicorn borrow (via a raw ptr) is undefined behavior in Rust, AFAIK. I don't think the PR is sound as is(?)

[wtdcode]

@bet4it Could you check domenukk's comments?

[bet4it]

Hmm, aliasing, and later accessing, a &mut Unicorn borrow (via a raw ptr) is undefined behavior in Rust, AFAIK. I don't think the PR is sound as is(?)

I'm a newbie in Rust, and I really can't understand what @domenukk said is about.

[bet4it]

Some codes of this PR base on unicorn-rs.
For example: https://github.com/unicorn-rs/unicorn-rs/blob/125adc1cd7bfdcab9bfd1e37c8eb1456fea825eb/src/macros.rs#L39-L44

macro_rules! destructure_hook {
    ($hook_type:path, $hook:ident) => {{
        let $hook_type { unicorn, callback } = unsafe { &mut *$hook };
        (unsafe { &**unicorn }, callback)
    }};
}

[PhilippTakacs]

Hmm, aliasing, and later accessing, a &mut Unicorn borrow (via a raw ptr) is undefined behavior in Rust, AFAIK. I don't think the PR is sound as is(?)

I'm a newbie in Rust, and I really can't understand what @domenukk said is about.

I haven't found the code he is referencing, but what he means is following:

Somewhere in the code you create a raw pointer from a &mut Unicorn and access this raw pointer after the reference is out of scope. This is undefined in Rust because rust is allow to move or destroy the underling object. So you need to ensure that the object is not moved or destroyed.

[PhilippTakacs]

OK, I have understand this now, for the callbacks you create a raw pointer of self and add this to the user data of the callback. later in the callback-wrapper you access the pointer and cast it to a Unicorn object. This is undefined in rust. The old code used InnerUnicorn and UnsafeCell to fix this. But this code has no fix for this problem.

[wtdcode]

Thanks for your help @PhilippTakacs !

@bet4it A small tip on ub: https://github.com/rust-lang/miri . Using miri may be super helpful.

[bet4it]

OK, I have understand this now, for the callbacks you create a raw pointer of self and add this to the user data of the callback. later in the callback-wrapper you access the pointer and cast it to a Unicorn object. This is undefined in rust. The old code used InnerUnicorn and UnsafeCell to fix this. But this code has no fix for this problem.

Do you mean that we cast the pointer to an Unicorn object, but if the pointer doesn't point to an Unicorn object, the behavior is undefined? But the callback is only called by the Unicorn object, so we can assure the pointer is an Unicorn object in the callback. That's what we have done in C, right?

[PhilippTakacs]

OK, I have understand this now, for the callbacks you create a raw pointer of self and add this to the user data of the callback. later in the callback-wrapper you access the pointer and cast it to a Unicorn object. This is undefined in rust. The old code used InnerUnicorn and UnsafeCell to fix this. But this code has no fix for this problem.

Do you mean that we cast the pointer to an Unicorn object, but if the pointer doesn't point to an Unicorn object, the behavior is undefined?

Yes

But the callback is only called by the Unicorn object, so we can assure the pointer is an Unicorn object in the callback. That's what we have done in C, right?

But C is not Rust. The important difference here is that in C you are responsible to have correct references, in Rust the language is responsible. Problem now is a mutable reference might get moved in memory. For later access rust can ensure a correct reference. But rust can't change a raw pointer. Therefore is aliasing and accessing a raw pointer of a &mut undefined.

There is no legal way to obtain aliasing &mut

https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html

But exactly this do you do with UcHook. The current code creates a owned UnsafeCell<InnerUnicorn> to get the raw pointer. This is one way[0] to provide rust user data to a C ffi.

[0] probably be the only one

[bet4it]

So you think InnerUnicorn is necessary? Then this PR is meaningless and we can close it😅

[PhilippTakacs]

Yes