Fix a use-after-free in the Rust bindings

[ProfFan]

Fixes #2175

[wtdcode]

Thanks for your contributions but you should submit this to https://github.com/unicorn-engine/unicorn-engine-rs

The rust bindings were already removed in the dev branch

[ProfFan]

It will need uc.c‎

[wtdcode]

Then you could submit both =)

[ProfFan]

Done @wtdcode

[wtdcode]

@amaanq What's your opinion on this?

Or could you elaborate why this solves the issue as it is not that straightforward.

[ProfFan]

The hook callback will be called twice, once when the callback is first called, and once in the next round when the simulation resumes, because the cleanup of the hooks linked list is lazy. At the 2nd time the Rust wrapper will try to access the context pointer which already got cleaned up because the Rust wrapper wrongfully assumed that the hook cleanup happens at the same time.

[PhilippTakacs]

The hook callback will be called twice, once when the callback is first called, and once in the next round when the simulation resumes

Can you point at the code where this happens. The cleanup clearly sets to_delete true and each hook loop check to_delete and skips these hooks. Also when there where a later call of the hook, this would also be a bug in hook system of unicorn.

because the cleanup of the hooks linked list is lazy. At the 2nd time the Rust wrapper will try to access the context pointer which already got cleaned up because the Rust wrapper wrongfully assumed that the hook cleanup happens at the same time.

I can't follow this observation by the code. Yes, hook cleanup is lazy, but the user_data is never accessed by unicorn after a hook_del().

Can you at least provide a test demonstrating that user_data can be used after a hook was deleted?

I'm not against this change, but it should be correct explained what it's for.

[ProfFan]

@PhilippTakacs I did not do very extensive testing tbh :)

You just need to run the original reproducer in #2175 in a debugger and put a breakpoint on the Rust callback. It hits twice which should not happen.

[PhilippTakacs]

I did not do very extensive testing tbh

Next time be more clear about such things. It's much simpler to understand problems with more information about the problem. Also put as much information about changes in your PR and your commits as possible. The change only describe what happens not why something happens. Therefor we have comments, commit messages and discussion in issues/PRs.

You just need to run the original reproducer in #2175 in a debugger and put a breakpoint on the Rust callback. It hits twice which should not happen.

Try to understand a bug with a reproducer which causes a segfault is a bit challenging. Also having multiple layers (unicorn and the rust bindings) in the reproducer doesn't help. You claimed to have a fix for the bug, therefor you should have had a better understanding of the bug. So I though it would be easy for you to provide a better reproducer.

A bit more about your fix: In many cases a use after free (and a nullptr deref) is not the bug itself. It's only the symptom of the bug. So just set stuff to NULL and add a NULL check isn't the best solution. Also this bug was noticed with the rust bindings, but isn't a bug in the rust bindings. You only looked at the rust bindings with your fix. Have you thought about the other bindings (no I don't want an answer, just think about it for yourself)?

[ProfFan]

Hi @PhilippTakacs I am sorry for the lack of details. I'm not a regular user of unicorn, this bug is what I discovered in my very first use of unicorn and I just made a quick patch in case this is useful for you. Yes, I know this is a very hacky solution but this is the best I can do given my 0 background in unicorn.

On the bug itself, on the Rust side it happens at user_data.uc.upgrade() at 2nd hook call.

  * frame #0: 0x0000000100003760 hook`alloc::rc::Weak<T,A>::upgrade [inlined] core::cell::Cell<T>::get(self=0x0000000000000000) at cell.rs:546:18
    frame #1: 0x0000000100003760 hook`alloc::rc::Weak<T,A>::upgrade [inlined] alloc::rc::RcInnerPtr::strong(self=0x000000016fdfde18) at rc.rs:3554:27
    frame #2: 0x0000000100003758 hook`alloc::rc::Weak<T,A>::upgrade(self=0x00000001006a5c30) at rc.rs:3346:18
    frame #3: 0x0000000100003b64 hook`unicorn_engine::hook::code_hook_proxy(uc=0x0000000a66c00000, address=4097, size=5, user_data=0x00000001006a5c30) at hook.rs:73:29
    frame #4: 0x0000000300000244
    frame #5: 0x000000010117b050 libunicorn.2.dylib`cpu_exec_x86_64 + 1348
    frame #6: 0x000000010113ce3c libunicorn.2.dylib`resume_all_vcpus_x86_64 + 168
    frame #7: 0x000000010112b070 libunicorn.2.dylib`uc_emu_start + 756
    frame #8: 0x00000001000020d8 hook`unicorn_engine::Unicorn<D>::emu_start(self=0x000000016fdfe728, begin=4096, until=4108, timeout=10000000, count=0) at lib.rs:1236:18
    frame #9: 0x0000000100000d7c hook`hook::main at hook.rs:20:16