RFC: define safe shared handle for Unicorn

[Jvlegod]

Unicorn currently implements Clone through Rc<UnsafeCell<UnicornInner<D>>>.

The design is useful: the cloned handle points to the same Unicorn engine.

• https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#aliasing-rules

(My understanding is that &D and &mut D cannot coexist.)

However, it can exec in unicorn-rs.

let emu = Unicorn::new_with_data(Arch::X86, Mode::MODE_64, 0u32)?;
let mut another = emu.clone();

let read_only: &u32 = emu.get_data();
let writable: &mut u32 = another.get_data_mut();

*writable = 42;
println!("{read_only}");

However, the current API exposes:

unicorn-engine-rs/crates/unicorn/src/lib.rs

Line 266 in e8ca055

pub fn get_data(&self) -> &D {

pub fn get_data(&self) -> &D;
pub fn get_data_mut(&mut self) -> &mut D;

Two cloned handles can therefore create &D and &mut D for the same D at the same time.

I think maybe this violates Rust's aliasing rules(see above).

Do we want to preserve Unicorn::clone() as a supported shared handle API?

[Jvlegod]

Hello @wtdcode, what are your thoughts on this?

[mlgiraud]

I agree that this is undefined behavior. As you already stated the main issue comes from being able to clone the Unicorn instance (and thus the inner Rc). This then violates the aliasing rule. I am unsure why we would want to be able to clone the Unicorn instance in the first place. If we want multiple references to the same instance, i think the cleaner solution would be to simply have the user wrap it in Rc or Rc<RefCell<..>>.

Currently, the inner Rc is required for the hooks (they need a weak ref) as far as i can tell, but this can also quickly become unsound, so maybe there is a better solution for the hooks. I would have to invest some more time (of which i have little at the moment sadly). Maybe @PhilippTakacs can take a closer look. Do we have a dedicated maintainer for the Unicorn rust bindings?

[wtdcode]

Oh yes, this is exactly unsound. Removing Clone is a quick fix but in generall I think we should remove Rc inside the Unicorn, hmmm.

[Jvlegod]

Oh yes, this is exactly unsound. Removing Clone is a quick fix but in generall I think we should remove Rc inside the Unicorn, hmmm.

I agree. Is anyone in the org currently doing this? If not, I’ll have a try.

[wtdcode]

Oh yes, this is exactly unsound. Removing Clone is a quick fix but in generall I think we should remove Rc inside the Unicorn, hmmm.

I agree. Is anyone in the org currently doing this? If not, I’ll try to have a try.

Not yet.