regomne / ilhook-rs

A library that provides methods to inline hook binary codes in x86 and x86_64 architecture
MIT License
69 stars 10 forks source link

feat: make CallbackOption send and sync #12

Closed plusls closed 2 months ago

regomne commented 2 months ago

Thank you. However, if the Send and Sync trait is added into the Box, no structs without the traits could be put into it. Is there any better solution?

plusls commented 2 months ago

Thank you. However, if the Send and Sync trait is added into the Box, no structs without the traits could be put into it. Is there any better solution?

Any example?

In my opinion, If a struct not support Sync, we can use newtype to wrap a Mutex to make it support Sync, for Send, maybe we need some example?

regomne commented 2 months ago

For Send, it seems that we can only use unsafe impl Send for XXX to make it support.

Any example?

struct MyThreadCb {
  myptr: *const i32 // A raw pointer make the struct not support Send.
}

impl ThreadCallback for MyThreadCb {
  //...
}

// error, as Send is not satisfied for MyThreadCb
let cb_opt = CallbackOption::Some(Box::new(MyThreadCb{ ... }));
plusls commented 2 months ago

For Send, it seems that we can only use unsafe impl Send for XXX to make it support.

Any example?

struct MyThreadCb {
  myptr: *const i32 // A raw pointer make the struct not support Send.
}

impl ThreadCallback for MyThreadCb {
  //...
}

// error, as Send is not satisfied for MyThreadCb
let cb_opt = CallbackOption::Some(Box::new(MyThreadCb{ ... }));

I have no idea about it, but I think we should make HookPoint Sync + Send?

In my use case, I will inject the DLL into the target program and wish to hold onto the HookPoint throughout the entire lifecycle of the DLL.

plusls commented 2 months ago

Perhaps having HookPoint automatically unhook upon drop is a bad idea? After all, the unhooking process should be considered unsafe.

regomne commented 2 months ago

In fact, I don't know how to let the HookPoint support Sync without breaking the compatibility, either.

Perhaps having HookPoint automatically unhook upon drop is a bad idea? After all, the unhooking process should be considered unsafe.

But for this problem, the easiest way is to std::mem::forget ?

plusls commented 2 months ago

In fact, I don't know how to let the HookPoint support Sync without breaking the compatibility, either.

Perhaps having HookPoint automatically unhook upon drop is a bad idea? After all, the unhooking process should be considered unsafe.

But for this problem, the easiest way is to std::mem::forget ?

It's too dirty, I think the best way is hold it and manual unhook it when DLL unload

plusls commented 2 months ago

But for this problem, the easiest way is to std::mem::forget ?

I current wrap HookPoint with newtype and impl Send and Sync for it.

In fact, I don't know how to let the HookPoint support Sync without breaking the compatibility, either.

And may be we should make some change to do some break change?

I think some API design has some problem.

Such as we can hook a addr thread safety, by use OpenProcess and WriteMemory API in windows, And direct write /proc/pid/mem in linux

In my opinion we can pass a custom callback to write memory, user can implement self write_memory function to make it thread safety

regomne commented 2 months ago

But you are right. The Drop trait does not and can not have an unsafe version, so maybe the unhook should always be manually?

Another way is to wrap the HookPoint and unsafe impl Send for it and use Mutex to make it Sync. Not so elegant, either.

I would try to find a way not to constraint the ThreadCallback forcibly. And would not accept this PR until it.

regomne commented 2 months ago

And may be we should make some change to do some break change? I think some API design has some problem.

Yes, breaking changes is allowed with the minor or major version upgraded, but I don't want to constraint the input trait, unless necessarily.