proc scheme: Rewrite try_stop_context

src/arch/x86_64/interrupt/exception.rs

@@ -20,9 +20,9 @@ interrupt_stack!(debug, stack, {
 
     let guard = ptrace::set_process_regs(stack);
 
-    // Disable singlestep before their is a breakpoint, since the
-    // breakpoint handler might end up setting it again but unless it
-    // does we want the default to be false.
+    // Disable singlestep before there is a breakpoint, since the breakpoint
+    // handler might end up setting it again but unless it does we want the
+    // default to be false.
     let had_singlestep = stack.iret.rflags & (1 << 8) == 1 << 8;
     stack.set_singlestep(false);
 

src/arch/x86_64/interrupt/syscall.rs

@@ -22,10 +22,10 @@ macro_rules! with_interrupt_stack {
         unsafe fn $wrapped(stack: *mut InterruptStack) {
             let _guard = ptrace::set_process_regs(stack);
 
-            let thumbs_up = ptrace::breakpoint_callback(PTRACE_STOP_PRE_SYSCALL, None)
+            let allowed = ptrace::breakpoint_callback(PTRACE_STOP_PRE_SYSCALL, None)
                 .and_then(|_| ptrace::next_breakpoint().map(|f| !f.contains(PTRACE_FLAG_IGNORE)));
 
-            if thumbs_up.unwrap_or(true) {
+            if allowed.unwrap_or(true) {
                 // If syscall not ignored
                 let $stack = &mut *stack;
                 $stack.scratch.rax = $code;

src/ptrace.rs

@@ -71,15 +71,6 @@ struct Session {
     tracee: WaitCondition,
     tracer: WaitCondition,
 }
-impl Session {
-    fn send_event(&self, event: PtraceEvent) {
-        // Add event to queue
-        self.data.lock().add_event(event);
-
-        // Alert blocking tracers
-        self.tracer.notify();
-    }
-}
 
 type SessionMap = BTreeMap<ContextId, Arc<Session>>;
 
@@ -95,8 +86,8 @@ fn sessions_mut() -> RwLockWriteGuard<'static, SessionMap> {
     SESSIONS.call_once(init_sessions).write()
 }
 
-/// Try to create a new session, but fail if one already exists for
-/// this process
+/// Try to create a new session, but fail if one already exists for this
+/// process
 pub fn try_new_session(pid: ContextId, file_id: usize) -> bool {
     let mut sessions = sessions_mut();
 
@@ -159,14 +150,17 @@ pub fn send_event(event: PtraceEvent) -> Option<()> {
 
     let sessions = sessions();
     let session = sessions.get(&context.id)?;
-    let data = session.data.lock();
+    let mut data = session.data.lock();
     let breakpoint = data.breakpoint.as_ref()?;
 
     if event.cause & breakpoint.flags != event.cause {
         return None;
     }
 
-    session.send_event(event);
+    // Add event to queue
+    data.add_event(event);
+    // Notify tracer
+    session.tracer.notify();
 
     Some(())
 }
@@ -215,14 +209,19 @@ pub fn cont(pid: ContextId) {
 
 /// Create a new breakpoint for the specified tracee, optionally with
 /// a sysemu flag. Panics if the session is invalid.
-pub fn set_breakpoint(pid: ContextId, flags: PtraceFlags) {
+pub fn set_breakpoint(pid: ContextId, flags: PtraceFlags, should_continue: bool) {
     let sessions = sessions_mut();
     let session = sessions.get(&pid).expect("proc (set_breakpoint): invalid session");
     let mut data = session.data.lock();
+
     data.breakpoint = Some(Breakpoint {
         reached: false,
         flags
     });
+
+    if should_continue {
+        session.tracee.notify();
+    }
 }
 
 /// Wait for the tracee to stop. If an event occurs, it returns a copy

src/scheme/proc.rs

@@ -50,40 +50,34 @@ fn with_context_mut<F, T>(pid: ContextId, callback: F) -> Result<T>
 fn try_stop_context<F, T>(pid: ContextId, restart_after: bool, mut callback: F) -> Result<T>
     where F: FnMut(&mut Context) -> Result<T>
 {
-    let mut first = true;
-    let mut was_stopped = false; // will never be read
+    // Stop process
+    let (was_stopped, mut running) = with_context_mut(pid, |context| {
+        let was_stopped = context.ptrace_stop;
+        context.ptrace_stop = true;
 
-    loop {
-        if !first {
-            // We've tried this before, so lets wait before retrying
-            unsafe { context::switch(); }
-        }
-        first = false;
+        Ok((was_stopped, context.running))
+    })?;
 
-        let contexts = context::contexts();
-        let context = contexts.get(pid).ok_or(Error::new(ESRCH))?;
-        let mut context = context.write();
-        if let Status::Exited(_) = context.status {
-            return Err(Error::new(ESRCH));
-        }
+    // Wait until stopped
+    while running {
+        unsafe { context::switch(); }
 
-        // Stop the process until we've done our thing
-        if first {
-            was_stopped = context.ptrace_stop;
-        }
-        context.ptrace_stop = true;
+        running = with_context(pid, |context| {
+            Ok(context.running)
+        })?;
+    }
 
-        if context.running {
-            // Process still running, wait until it has stopped
-            continue;
-        }
+    with_context_mut(pid, |context| {
+        assert!(!context.running, "process can't have been restarted, we stopped it!");
 
-        let ret = callback(&mut context);
+        let ret = callback(context);
 
-        context.ptrace_stop = restart_after && was_stopped;
+        if !was_stopped || restart_after {
+            context.ptrace_stop = false;
+        }
 
-        break ret;
-    }
+        ret
+    })
 }
 
 #[derive(Clone, Copy, PartialEq, Eq)]
@@ -154,6 +148,7 @@ impl Handle {
     fn continue_ignored_children(&mut self) -> Option<()> {
         let data = self.data.trace_data()?;
         let contexts = context::contexts();
+
         for pid in data.clones.drain(..) {
             if ptrace::is_traced(pid) {
                 continue;
@@ -222,8 +217,8 @@ impl Scheme for ProcScheme {
                     return Err(Error::new(EPERM));
                 }
 
-                // Is it a subprocess of us? In the future, a capability
-                // could bypass this check.
+                // Is it a subprocess of us? In the future, a capability could
+                // bypass this check.
                 match contexts.anchestors(target.ppid).find(|&(id, _context)| id == current.id) {
                     Some((id, context)) => {
                         // Paranoid sanity check, as ptrace security holes
@@ -240,8 +235,8 @@ impl Scheme for ProcScheme {
 
         if let Operation::Trace { .. } = operation {
             if !ptrace::try_new_session(pid, id) {
-                // There is no good way to handle id being occupied
-                // for nothing here, is there?
+                // There is no good way to handle id being occupied for nothing
+                // here, is there?
                 return Err(Error::new(EBUSY));
             }
 
@@ -347,7 +342,7 @@ impl Scheme for ProcScheme {
                         let fx = context.arch.get_fx_regs().unwrap_or_default();
                         Ok((Output { float: fx }, mem::size_of::<FloatRegisters>()))
                     })?,
-                    RegsKind::Int => try_stop_context(info.pid, true, |context| match unsafe { ptrace::regs_for(&context) } {
+                    RegsKind::Int => try_stop_context(info.pid, false, |context| match unsafe { ptrace::regs_for(&context) } {
                         None => {
                             println!("{}:{}: Couldn't read registers from stopped process", file!(), line!());
                             Err(Error::new(ENOTRECOVERABLE))
@@ -448,7 +443,7 @@ impl Scheme for ProcScheme {
                         *(buf as *const _ as *const IntRegisters)
                     };
 
-                    try_stop_context(info.pid, true, |context| match unsafe { ptrace::regs_for_mut(context) } {
+                    try_stop_context(info.pid, false, |context| match unsafe { ptrace::regs_for_mut(context) } {
                         None => {
                             println!("{}:{}: Couldn't read registers from stopped process", file!(), line!());
                             Err(Error::new(ENOTRECOVERABLE))
@@ -472,21 +467,13 @@ impl Scheme for ProcScheme {
                 let op = u64::from_ne_bytes(bytes);
                 let op = PtraceFlags::from_bits(op).ok_or(Error::new(EINVAL))?;
 
-                if !op.contains(PTRACE_FLAG_WAIT) || op.intersects(PTRACE_STOP_MASK) {
-                    ptrace::cont(info.pid);
-                }
-                if op.intersects(PTRACE_STOP_MASK) {
-                    ptrace::set_breakpoint(info.pid, op);
-                }
+                let should_continue = !op.contains(PTRACE_FLAG_WAIT) || op.intersects(PTRACE_STOP_MASK);
 
                 if op.contains(PTRACE_STOP_SINGLESTEP) {
-                    // try_stop_context with `false` will
-                    // automatically disable ptrace_stop
-                    try_stop_context(info.pid, false, |context| {
+                    // `true` to `try_stop_context` means we restart after, no
+                    // matter if it was or wasn't stopped before
+                    try_stop_context(info.pid, true, |context| {
                         match unsafe { ptrace::regs_for_mut(context) } {
-                            // If another CPU is running this process,
-                            // await for it to be stopped and in such
-                            // a way the registers can be read!
                             None => {
                                 println!("{}:{}: Couldn't read registers from stopped process", file!(), line!());
                                 Err(Error::new(ENOTRECOVERABLE))
@@ -505,6 +492,12 @@ impl Scheme for ProcScheme {
                     })?;
                 }
 
+                // Set next breakpoint, and potentially restart tracee
+                if op.intersects(PTRACE_STOP_MASK) {
+                    ptrace::set_breakpoint(info.pid, op, should_continue);
+                }
+
+                // And await the tracee, if requested to
                 if op.contains(PTRACE_FLAG_WAIT) || info.flags & O_NONBLOCK != O_NONBLOCK {
                     ptrace::wait(info.pid)?;
                 }