Implement enters target for guarding execution from double-mutability #2878
Conversation
| state: &'a mut S, | ||
| mgr: &'a mut EM, | ||
| input: &'a I, | ||
| ) -> Self::Guard<'a> { | ||
| unsafe { | ||
| let data = &raw mut GLOBAL_STATE; |
There was a problem hiding this comment.
Add a read_volatile check here that current_input_ptr is ptr::null() please
And add the potential race condition to the # Safety comment
There was a problem hiding this comment.
Correct me if I'm wrong, but I don't think there's a race threat here unless there are multiple executors running simultaneously. If there is, then signal handling is already broken, since this would apply at a thread group level.
There was a problem hiding this comment.
Yes if you call this function twice you have a race condition. Ergo, document.
Also, return Err or None if it's already set
There was a problem hiding this comment.
I'm not saying it's how we use it, but it's how the doc should be
There was a problem hiding this comment.
Ah but you can't call this function twice, because you'll hold the guard.
There was a problem hiding this comment.
And, if you're not holding the guard, then the guard is dropped and it's already restored the guarded state.
| OT: ObserversTuple<I, S>, | ||
| /// A simple guard, to be used with [`GenericInProcessExecutorInner`]. | ||
| #[derive(Debug)] | ||
| pub struct InProcessGuard<'a> { |
There was a problem hiding this comment.
Shouldn't/coudn't this guy have the type of the generics used? As it is now you can still use it to call the methods with a different type, right?
There was a problem hiding this comment.
You would always be able to call the methods with a different type, but there are other ways that this is restricted (i.e. the harness gets moved out of the executor when it's in use, then restored after). There is no real way to restrict this further, without holding EM/Z in phantom data.
…ms in global data
|
While this would be nice to have, we need to severely revisit executors in general before we could use this effectively. Closing. |
|
Why? |
This will almost guaranteed break QEMU, because it was doing something necessarily unsound (double-mutable access of state). This needs to be resolved before this is merged.